[YUNIKORN-2625] Refactor Clients to avoid hard-code checks #844

arhtur007 · 2024-05-20T02:41:50Z

What is this PR for?

Use a for loop instead of listing all informer check manually.

What type of PR is it?

Todos

N/A

What is the Jira issue?

YUNIKORN-2625

How should this be tested?

make test

Screenshots (if appropriate)

Questions:

N/A

codecov · 2024-05-20T09:59:08Z

Codecov Report

Attention: Patch coverage is 82.60870% with 16 lines in your changes missing coverage. Please review.

Project coverage is 67.78%. Comparing base (8e680af) to head (3ad1a0f).
Report is 1 commits behind head on master.

Files	Patch %	Lines
pkg/client/clients.go	92.06%	4 Missing and 1 partial ⚠️
pkg/client/apifactory.go	0.00%	4 Missing ⚠️
pkg/client/kubeclient.go	0.00%	4 Missing ⚠️
pkg/client/apifactory_mock.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
+ Coverage   67.17%   67.78%   +0.60%     
==========================================
  Files          70       69       -1     
  Lines        7631     7629       -2     
==========================================
+ Hits         5126     5171      +45     
+ Misses       2287     2238      -49     
- Partials      218      220       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chia7712 · 2024-05-20T14:16:45Z

pkg/client/clients.go

 	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/volumebinding"

 	"github.com/apache/yunikorn-k8shim/pkg/conf"
 	"github.com/apache/yunikorn-k8shim/pkg/log"
 	"github.com/apache/yunikorn-scheduler-interface/lib/go/api"
 )

+type informer interface {


Maybe we just add a new method to Clients to return all informers. For example:

func (c *Clients) informers() []cache.SharedIndexInformer

The true impl is to collect informers from all resource informers. WDYT

Is it better to call Informer() cache.SharedIndexInformer every time we need it?

I agree with using slice instead of map since informers are set and remain unchanged.

Is it better to call Informer() cache.SharedIndexInformer every time we need it?

It seems both WaitForSync and Run are not hot methods, right? If so, creating the new collection should not a issue about performance regression I think.

I came across this comment in the source code:

// Always prefer using an informer factory to get a shared informer instead of getting an independent // one. This reduces memory footprint and number of connections to the server.

There doesn't appear to be any issue with it currently.
But I think we should avoid any potential hidden bugs in the codebase.

I think this is getting a little out of hand. This is initialization code to handle common Kubernetes objects. We don't want to expose slices, or other internal implementation details. This code is also not hot, as it gets called once during execution. We should favor simplicity here. I think this new implementation is overly complex.

I've updated the pr based on @chia7712's comment.

I looked at using something like slices for storing all the different informers. Due to the differences of the listers between the informers that becomes a slice of interface{} That then triggers type casting and all kinds of other handling and checks which is worse than having the separate variables and maintaining the two sets. Just storing the Informer() and not the Lister() for the informers does not make sense for me. The lister is normally accessed outside of startup the informer is never accessed as it just runs...

Any informer defined should be started and stopped. To not regress we could have a simple unit test that checks that all informers defined in the type are also returned in the getInformers() call. If someone adds a new informer we can then ensure that they are updated in both places

Any informer defined should be started and stopped. To not regress we could have a simple unit test that checks that all informers defined in the type are also returned in the getInformers() call. If someone adds a new informer we can then ensure that they are updated in both places

agree to that "started" and "stopped" are the most important here. It means Clients must have all "created" informers. Maybe we can add a small helper to collect all created informers into a slice, and then we pass the slice to Clients. For example:

type hasInformer interface { Informer() cache.SharedIndexInformer } func save[T hasInformer](n T, collector *[]cache.SharedIndexInformer) T { *collector = append(*collector, n.Informer()) return n }

We use a little generic here to avoid casting, and all created informers should be wrapped by save. For example:

var allInformers []cache.SharedIndexInformer nodeInformer := save(informerFactory.Core().V1().Nodes(), &allInformers) podInformer := save(informerFactory.Core().V1().Pods(), &allInformers)

Finally, allInformers have all created informers, and Clients will use it to start/stop all informers.

@wilfred-s WDYT?

chia7712 · 2024-05-20T18:46:57Z

pkg/client/clients.go

-			log.Log(log.ShimClient).Info("Waiting for informers to sync",
-				zap.Duration("timeElapsed", time.Since(syncStartTime).Round(time.Second)))
+
+LOOP:


All we want to do is to make sure all get synced, right? Hence, we can check informer one by one.

for _, informer := range c.getInformers() { for !informer.HasSynced() { time.Sleep(time.Second) counter++ if counter%10 == 0 { log.Log(log.ShimClient).Info("Waiting for informers to sync", zap.Duration("timeElapsed", time.Since(syncStartTime).Round(time.Second))) } } }

Also, that will create a slice only once.

wilfred-s · 2024-05-21T05:38:53Z

pkg/client/clients.go

@@ -67,38 +68,38 @@ func (c *Clients) GetConf() *conf.SchedulerConf {
 	return c.conf
 }

+func (c *Clients) getInformers() []cache.SharedIndexInformer {
+	return []cache.SharedIndexInformer{


This list should be in sync with the informerTypes or the other way around. That was the issue that triggered YUNIKORN-2621
Not sure how we do that without manual work. The informers side is simple, the listers are different for every type which makes it tricky.

This list should be in sync with the informerTypes or the other way around.
That was the issue that triggered YUNIKORN-2621
Not sure how we do that without manual work. The informers side is simple, the listers are different for every type which makes it tricky.

agree to that is tricky. Maybe we can have a inner interface HasInformer to offer informer, and then we cast it to true struct to create Clients. For example:

func (t Type) HasInformer(informerFactory informers.SharedInformerFactory) HasInformer { switch t { case PVInformerHandlers: return informerFactory.Core().V1().PersistentVolumes() case PodInformerHandlers: return informerFactory.Core().V1().Pods() } return nil } func NewAPIFactory(scheduler api.SchedulerAPI, informerFactory informers.SharedInformerFactory, configs *conf.SchedulerConf, testMode bool) *APIFactory { kubeClient := NewKubeClient(configs.KubeConfig) all := make(map[Type]HasInformer) for s := range informerTypes { all[Type(s)] = Type(s).HasInformer(informerFactory) } // init informers // volume informers are also used to get the Listers for the predicates nodeInformer := all[NodeInformerHandlers].(v1.NodeInformer) podInformer := all[PodInformerHandlers].(v1.PodInformer) }

With this change, we have to add new const Type when adding new informer in the future, as all structs are from Type(s).HasInformer. Also, the map all can pass to Clients to be used as "all informers". BTW, we can enable exhaustive of linters to avoid missing statements in switch.

In short, we can consider adding following changes.

enable exhaustive check - avoid missing switch statement

Type can return a new inner interface HasInformer - bind the informer to Type

cast HasInformer to true struct instead of accessing SharedInformerFactory - make sure all used informers come from Type

Manually initializing each informer is more intuitive. However, in order to ensure the list is synchronized, @chia7712 's method is better.

Another nit suggestion is that we should move all informer-related code to clients.go since all the informers are under the Client struct. Additionally, interfaces.go should be combined with kubeclient.go

arhtur007 · 2024-06-07T18:05:26Z

Hello @wilfred-s , @chia7712

I've updated the PR and add a test for checking informers.
pleas take a look at it when you're free, thank you.

pbacsko

I don't want to approve this just yet. To me, the more compact code results in a code which is more difficult to comprehend.

If others are OK with it I'm fine too, but I suggest dropping the save() + hasInformers combo.

pbacsko · 2024-06-11T19:31:02Z

pkg/client/clients.go

+	hasInformers := []hasInformer{}
+
+	podInformer := save(informerFactory.Core().V1().Pods(), &hasInformers)
+	nodeInformer := save(informerFactory.Core().V1().Nodes(), &hasInformers)
+	configMapInformer := save(informerFactory.Core().V1().ConfigMaps(), &hasInformers)
+	storageInformer := save(informerFactory.Storage().V1().StorageClasses(), &hasInformers)
+	pvInformer := save(informerFactory.Core().V1().PersistentVolumes(), &hasInformers)
+	pvcInformer := save(informerFactory.Core().V1().PersistentVolumeClaims(), &hasInformers)
+	priorityClassInformer := save(informerFactory.Scheduling().V1().PriorityClasses(), &hasInformers)
+	namespaceInformer := save(informerFactory.Core().V1().Namespaces(), &hasInformers)
+	csiNodeInformer := save(informerFactory.Storage().V1().CSINodes(), &hasInformers)
+	csiDriversInformer := save(informerFactory.Storage().V1().CSIDrivers(), &hasInformers)
+	csiStorageCapacityInformer := save(informerFactory.Storage().V1().CSIStorageCapacities(), &hasInformers)


Honestly, I don't know. We do save some lines by introducing this intermediate type, at the same, readability is not the best. In fact, I had to download and apply the patch locally to understand what's happening here.
I'm not the fan of this.

For example, even though this is more LOC, it's just much simpler:

var sharedInformers []cache.SharedIndexInformer podInformer := informerFactory.Core().V1().Pods() sharedInformers = append(sharedInformers, podInformer.Informer()) nodeInformer := informerFactory.Core().V1().Nodes() sharedInformers = append(sharedInformers, nodeInformer.Informer()) ...

Then sharedInformers can be passed as a slice to Client after it's done. This way, we also don't need to copy the slice every time we call getInformers(), another thing which I don't really like.

For example, even though this is more LOC, it's just much simpler:

yep, that is a good idea. However, the previous discussion (#844 (comment)) seems to want a style of avoid ignoring the SharedIndexInformer in creating XXXInformer . Otherwise, this simple way is good to me.

Let me summary the requests:

we must save the SharedIndexInformer from all used XXXInformer ([YUNIKORN-2625] Refactor Clients to avoid hard-code checks #844 (comment))

the informerTypes should be synced with all used XXXInformer ([YUNIKORN-2625] Refactor Clients to avoid hard-code checks #844 (comment))

I agree to @wilfred-s said "Not sure how we do that without manual work" - Maybe we should let this PR go and focus on adding missed CSINodeInformer and NamespaceInformer to informerTypes manually

pbacsko · 2024-06-11T19:31:37Z

pkg/client/apifactory.go

@@ -64,7 +44,7 @@ type APIProvider interface {
 // resource handlers defines add/update/delete operations in response to the corresponding resources updates.
 // The associated the type field points the handler functions to the correct receiver.
 type ResourceEventHandlers struct {
-	Type
+	InformerType


Was this changed to avoid confusion with type?

chia7712 self-requested a review May 20, 2024 06:06

chia7712 reviewed May 20, 2024

View reviewed changes

wilfred-s requested changes May 21, 2024

View reviewed changes

chenyulin0719 assigned arhtur007 May 23, 2024

[YUNIKORN-2625] Refactor Clients to avoid hard-code checks

eb4ae4e

arhtur007 force-pushed the YUNIKORN-2625 branch from f4248e0 to 5677544 Compare June 7, 2024 17:58

arhtur007 added 2 commits June 8, 2024 01:58

[YUNIKORN-2625] Add test

5677544

[YUNIKORN-2625] nit

3ad1a0f

pbacsko requested changes Jun 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YUNIKORN-2625] Refactor Clients to avoid hard-code checks #844

[YUNIKORN-2625] Refactor Clients to avoid hard-code checks #844

arhtur007 commented May 20, 2024

codecov bot commented May 20, 2024 •

edited

chia7712 May 20, 2024

arhtur007 May 20, 2024

arhtur007 May 20, 2024

chia7712 May 20, 2024

arhtur007 May 20, 2024

arhtur007 May 20, 2024

craigcondit May 20, 2024

arhtur007 May 20, 2024 •

edited

wilfred-s May 22, 2024 •

edited

chia7712 May 22, 2024

chia7712 May 20, 2024

wilfred-s May 21, 2024

chia7712 May 21, 2024

arhtur007 May 21, 2024 •

edited

arhtur007 commented Jun 7, 2024

pbacsko left a comment •

edited

pbacsko Jun 11, 2024

chia7712 Jun 11, 2024

pbacsko Jun 11, 2024

[YUNIKORN-2625] Refactor Clients to avoid hard-code checks #844

Are you sure you want to change the base?

[YUNIKORN-2625] Refactor Clients to avoid hard-code checks #844

Conversation

arhtur007 commented May 20, 2024

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

codecov bot commented May 20, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arhtur007 May 20, 2024 • edited

Choose a reason for hiding this comment

wilfred-s May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arhtur007 May 21, 2024 • edited

Choose a reason for hiding this comment

arhtur007 commented Jun 7, 2024

pbacsko left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 20, 2024 •

edited

arhtur007 May 20, 2024 •

edited

wilfred-s May 22, 2024 •

edited

arhtur007 May 21, 2024 •

edited

pbacsko left a comment •

edited