fix(propeller): Replace SharedIndexInformer with Informer #5129
Conversation
dosubot
bot
added
size:L
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5129 +/- ##
==========================================
+ Coverage 59.06% 59.93% +0.86%
==========================================
Files 646 463 -183
Lines 55714 39030 -16684
==========================================
- Hits 32910 23394 -9516
+ Misses 20208 13792 -6416
+ Partials 2596 1844 -752
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| counts := r.countList(ctx, objects) | ||
|
|
||
| pods := &corev1.PodList{} | ||
| if err := r.kubeClient.GetClient().List(ctx, pods); err != nil { |
There was a problem hiding this comment.
I am uncertain about the change here. Originally, sharedInformer fetches the resources it listens to, but now it changes to only fetches pods. Also, the code seems be metrics collecting and is from four years ago. I am not sure if the code is still relevant. cc @wild-endeavor
There was a problem hiding this comment.
Agreed, it is not safe to update this to a PodList because it will not capture other k8s resource types for other Flyte plugins (ex. Spark, Ray, TF, PyTorch, etc).
| @@ -668,16 +668,10 @@ func getPluginGvk(resourceToWatch runtime.Object) (schema.GroupVersionKind, erro | |||
| return kinds[0], nil | |||
| } | |||
|
|
|||
| func getPluginSharedInformer(ctx context.Context, kubeClient pluginsCore.KubeClient, resourceToWatch client.Object) (cache.SharedIndexInformer, error) { | |||
| func getPluginSharedInformer(ctx context.Context, kubeClient pluginsCore.KubeClient, resourceToWatch client.Object) (cache.Informer, error) { | |||
There was a problem hiding this comment.
nit:
| func getPluginSharedInformer(ctx context.Context, kubeClient pluginsCore.KubeClient, resourceToWatch client.Object) (cache.Informer, error) { | |
| func getPluginInformer(ctx context.Context, kubeClient pluginsCore.KubeClient, resourceToWatch client.Object) (cache.Informer, error) { |
MortalHappiness
force-pushed
the
bugfix/#5087-limit-namespace-error
branch
from
aa10ed9 to
80037f5
Compare
Resolves: flyteorg#5087 Signed-off-by: Chi-Sheng Liu <[email protected]>
Update unittests corresponding to the changes from SharedIndexInformer to Informer Resolves: flyteorg#5087 Signed-off-by: Chi-Sheng Liu <[email protected]>
MortalHappiness
force-pushed
the
bugfix/#5087-limit-namespace-error
branch
from
80037f5 to
70d0573
Compare
There was a problem hiding this comment.
@MortalHappiness thanks for the PR here! Great insight, but I'm actually wondering if we have gone too far here. The original code just retrieves the informer and uses the cache.sharedIndexInformer to periodically retrieve the list of k8s resources so that it can emit a metric. This obviously pulls from a cache.
Does it make more sense to just use the cache (ie. kubeClient.GetCache() ) within the ResourceLevelMonitor and periodically retrieve directly from there depending on object type? So this line could instead be:
rm := monitorIndex.GetOrCreateResourceLevelMonitor(ctx, metricsScope, kubeClient.GetCache(), entry.ResourceToWatch, gvk)
|
Superseded by #5238. |
Tracking issue
Resolves: #5087
Why are the changes needed?
See the issue for detailed information.
What changes were proposed in this pull request?
SharedIndexInformer(client-go) withInformer(controller-runtime)core.KubeClienttoResourceLevelMonitorobjectswithPodListHow was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link