fix: remove unused informers #698
Conversation
britaniar
changed the title
britaniar
changed the title
britaniar
force-pushed
the
removeUnusedInformers
branch
3 times, most recently
from
58404fa
to
d826166
Compare
britaniar
force-pushed
the
removeUnusedInformers
branch
from
0834f2e
to
0f153f5
Compare
| impl.AddDynamicResources([]APIResourceMeta{}, handler, true) | ||
|
|
||
| // verify the map. Check that the dynamic resource was removed from the informer manager | ||
| _, ok = impl.apiResources[dynResources[0].GroupVersionKind] |
There was a problem hiding this comment.
we need to test more than if the imp has it, we don't need IT to test that.
| @@ -1,17 +1,17 @@ | |||
| module go.goms.io/fleet | |||
|
|
|||
| go 1.20 | |||
| go 1.21 | |||
There was a problem hiding this comment.
Hi Britania! Is this Go + deps version bump intentional?
There was a problem hiding this comment.
I updated the controller-runtime package I guess I updated the other as well with it.
britaniar
force-pushed
the
removeUnusedInformers
branch
2 times, most recently
from
0e9ca75
to
721e4f7
Compare
There was a problem hiding this comment.
We are trying out to switch from dynamic informer factory to informer cache to be able to remove unused informers. There is currently some errors that I have been running into.
Arvind helped me look into it further and some links that may be helpful:
- there are no tests which test the case of adding dynamic resources in this file
- This commit that shows it allows dynamic addition/removal of informers not the ability to add informers for dynamic resources
| } | ||
| //if !r.InformerManager.IsInformerSynced(gvr) { | ||
| // return nil, controller.NewExpectedBehaviorError(fmt.Errorf("informer cache for %+v is not synced yet", gvr)) | ||
| //} |
There was a problem hiding this comment.
Had to change to use GVK to be able to get the informer but GVK does not used in this function
| if err != nil { | ||
| klog.V(2).ErrorS(err, "Could not get object of a disappeared resource", "res", dynRes) | ||
| } | ||
| err = s.cache.RemoveInformer(s.ctx, obj.(client.Object)) |
There was a problem hiding this comment.
Added RemoveInformer() function here.
| if !exist { | ||
| newRes.isPresent = true | ||
| // TODO: how to add GVK to scheme? |
There was a problem hiding this comment.
The old method (using dynamic informer factory) automatically added the dynamic resources to the scheme, but informer cache does not and adds only known types. Currently, running into an issue where the not how to add the dynamic resources to the scheme using only the GVK?
Current errors I see:
E0320 01:57:50.893982 1 informer/informermanager.go:148] "Failed to create informer for resource" err="no kind \"CloneSet\" is registered for version \"apps.kruise.io/v1alpha1\" in scheme \"pkg/runtime/scheme.go:100\"" gvk="apps.kruise.io/v1alpha1, Kind=CloneSet" err="no kind \"CloneSet\" is registered for version \"apps.kruise.io/v1alpha1\" in scheme \"pkg/runtime/scheme.go:100\"" E0320 01:57:50.894143 1 informer/informermanager.go:148] "Failed to create informer for resource" err="no kind \"VolumeSnapshot\" is registered for version \"snapshot.storage.k8s.io/v1\" in scheme \"pkg/runtime/scheme.go:100\"" gvk="snapshot.storage.k8s.io/v1, Kind=VolumeSnapshot" err="no kind \"VolumeSnapshot\" is registered for version \"snapshot.storage.k8s.io/v1\" in scheme \"pkg/runtime/scheme.go:100\"" E0320 01:57:50.894305 1 informer/informermanager.go:148] "Failed to create informer for resource" err="no kind \"VolumeSnapshotContent\" is registered for version \"snapshot.storage.k8s.io/v1\" in scheme \"pkg/runtime/scheme.go:100\"" gvk="snapshot.storage.k8s.io/v1, Kind=VolumeSnapshotContent" err="no kind \"VolumeSnapshotContent\" is registered for version \"snapshot.storage.k8s.io/v1\" in scheme \"pkg/runtime/scheme.go:100\"" E0320 01:57:50.894578 1 informer/informermanager.go:148] "Failed to create informer for resource" err="no kind \"VolumeSnapshotClass\" is registered for version \"snapshot.storage.k8s.io/v1\" in scheme \"pkg/runtime/scheme.go:100\"" gvk="snapshot.storage.k8s.io/v1, Kind=VolumeSnapshotClass" err="no kind \"VolumeSnapshotClass\" is registered for version \"snapshot.storage.k8s.io/v1\" in scheme \"pkg/runtime/scheme.go:100\""
ryanzhang-oss
force-pushed
the
removeUnusedInformers
branch
from
721e4f7
to
8b92dd7
Compare
Description of your changes
Removed unused informers from the informer manager
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer