Fix: pool-coordinator #1126

LaurenceLiZhixin · 2023-01-06T09:43:57Z

fix some logical bugs and nil pointer.

Congrool · 2023-01-08T13:42:44Z

pkg/yurthub/certificate/hubself/cert_mgr.go

-// createInsecureRestClientConfig create insecure rest client config.
-func createInsecureRestClientConfig(remoteServer *url.URL) (*restclient.Config, error) {
+// CreateInsecureRestClientConfig create insecure rest client config.
+func CreateInsecureRestClientConfig(remoteServer *url.URL) (*restclient.Config, error) {


Why do we expose the function?
I don't find anywhere that uses it.

I'll fix it.

Congrool · 2023-01-08T13:45:47Z

pkg/yurthub/filter/approver_test.go

@@ -43,6 +43,7 @@ var supportedResourceAndVerbsForFilter = map[string]map[string]sets.String{
 	},
 	ServiceTopologyFilterName: {
 		"endpoints":      sets.NewString("list", "watch"),
+		"pods":           sets.NewString("list", "watch"),


Why to add this entry?

I'll fix it.

Congrool · 2023-01-08T14:03:59Z

pkg/yurthub/poolcoordinator/coordinator.go

@@ -203,10 +201,9 @@ func (coordinator *coordinator) Run() {
 				needUploadLocalCache = true
 				needCancelEtcdStorage = true
 				isPoolCacheSynced = false
-				etcdStorage = nil
-				poolCacheManager = nil
+				coordinator.poolCacheManager = nil


Do not update the coordinator field if we do not acquire lock, since these fields are used and exposed by IsReady and IsHealthy function, which may be called concurrently.

For example, we assume that coordinator is healthy when coordinator.electStatus != PendingHub, at which time the coordinator.poolCacheManager should also not be nil. Thus the condition coordinator.electStatus != PendingHub && coordinator.poolCacheManager != nil is a defined situation. And the caller of IsHealthy will always get non-nil poolCacheManager when it find the returned bool value of IsHealthy is true.

However, if we update coordinator.poolCacheManager here. The caller of IsHealthy may find that the poolcoordinator is healthy, but got a nil poolCacheManager, which situation is undefined.

I'll add lock here

Congrool · 2023-01-08T14:04:06Z

pkg/yurthub/poolcoordinator/coordinator.go

 			case LeaderHub:
-				poolCacheManager, etcdStorage, cancelEtcdStorage, err = coordinator.buildPoolCacheStore()
+				coordinator.poolCacheManager, coordinator.etcdStorage, cancelEtcdStorage, err = coordinator.buildPoolCacheStore()


Congrool · 2023-01-08T14:05:42Z

pkg/yurthub/poolcoordinator/coordinator.go

 						klog.Errorf("failed to upload local cache when yurthub becomes leader, %v", err)
 					} else {
 						needUploadLocalCache = false
 					}
 				}
 			case FollowerHub:
-				poolCacheManager, etcdStorage, cancelEtcdStorage, err = coordinator.buildPoolCacheStore()
+				coordinator.poolCacheManager, coordinator.etcdStorage, cancelEtcdStorage, err = coordinator.buildPoolCacheStore()


Congrool · 2023-01-08T14:11:56Z

pkg/yurthub/poolcoordinator/coordinator.go

@@ -316,7 +311,8 @@ func (coordinator *coordinator) IsReady() (cachemanager.CacheManager, bool) {
 	// If electStatus is not PendingHub, it means pool-coordinator is healthy.
 	coordinator.Lock()
 	defer coordinator.Unlock()
-	if coordinator.electStatus != PendingHub && coordinator.isPoolCacheSynced && !coordinator.needUploadLocalCache {
+	// fixme:  coordinator.isPoolCacheSynced now is not considered
+	if coordinator.electStatus != PendingHub && !coordinator.needUploadLocalCache {


Why to remove the isPoolCacheSynced?

Now isPoolCacheSynced is never set to 'true'.

Acctually we need isPoolCacheSynced. I'll submit a pr to fix it.

fixed by #1131

Congrool · 2023-01-08T14:40:01Z

pkg/yurthub/poolcoordinator/leader_election.go

@@ -55,7 +55,7 @@ func NewHubElector(
 		coordinatorClient:           coordinatorClient,
 		coordinatorHealthChecker:    coordinatorHealthChecker,
 		cloudAPIServerHealthChecker: cloudAPIServerHealthyChecker,
-		electorStatus:               make(chan int32),
+		electorStatus:               make(chan int32, 1),


Why to set the capability of electorStatus as 1?

The capability set to 0 would cause deadlock in line: https://github.com/openyurtio/openyurt/blob/pool-coordinator-dev/pkg/yurthub/poolcoordinator/leader_election.go#L91
@Congrool

How about moving he.electorStatus <- PendingHub to the begining of Run()?

Congrool · 2023-01-08T14:47:44Z

cmd/yurthub/app/start.go

 		if err != nil {
 			return fmt.Errorf("failed to wait for coordinator to run, %v", err)
 		}
 	}

+	// wait for async coordinator informer registry
+	time.Sleep(time.Second * 5)


It seems not a good way to use time.Sleep to sync.

You are right. I'll change it to notification.

Congrool · 2023-01-08T15:45:46Z

pkg/yurthub/storage/etcd/keycache.go

@@ -120,48 +120,49 @@ func (c *componentKeyCache) Recover() error {
 }

 func (c *componentKeyCache) getPoolScopedKeyset() (*keySet, error) {
-	client := c.getEtcdClient()


It seems the function is uninitialized. I'll fix it.

Will you fix it into my pr or create a new one ? @Congrool

I'll create a new pr to fix it.

fixed by #1137

Congrool · 2023-01-08T15:50:40Z

BTW, could you please split your PR into several commits each of which fixes one bug? It will be helpful for review, thanks!

LaurenceLiZhixin · 2023-01-09T01:49:32Z

BTW, could you please split your PR into several commits each of which fixes one bug? It will be helpful for review, thanks!

Sure, I'll do that later

… into pool-coordinator-fix

openyurt-bot · 2023-01-11T07:51:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LaurenceLiZhixin, rambohe-ch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rambohe-ch]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Fix: pool-coordinator

Fix: pool-coordinator

aa4964f

openyurt-bot added the size/L size/L: 100-499 label Jan 6, 2023

openyurt-bot requested review from charleszheng44 and Fei-Guo January 6, 2023 09:46

Fix: imports

74d0f69

Congrool reviewed Jan 8, 2023

View reviewed changes

LaurenceLiZhixin added 5 commits January 9, 2023 09:54

Fix

81413c6

Merge branch 'pool-coordinator-dev' of github.com:openyurtio/openyurt…

c942e0c

… into pool-coordinator-fix

Fix: gzip bug

144557a

Fix

02397dc

Fix

85a5e18

rambohe-ch approved these changes Jan 11, 2023

View reviewed changes

openyurt-bot added the approved approved label Jan 11, 2023

rambohe-ch merged commit 07fc159 into openyurtio:pool-coordinator-dev Jan 11, 2023

Congrool mentioned this pull request Jan 12, 2023

fix coordinator sync problem and update ready condition #1142

Merged

Congrool pushed a commit to Congrool/openyurt that referenced this pull request Jan 17, 2023

Fix: pool-coordinator (openyurtio#1126)

0a8659c

* Fix: pool-coordinator

Congrool mentioned this pull request Jan 17, 2023

cherry-pick yurthub code of pool-coordinator-dev #1156

Merged

LaurenceLiZhixin mentioned this pull request Jan 17, 2023

[REQUEST Membership]: New membership for <LaurenceLiZhixin> openyurtio/community#57

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: pool-coordinator #1126

Fix: pool-coordinator #1126

LaurenceLiZhixin commented Jan 6, 2023

Congrool Jan 8, 2023

LaurenceLiZhixin Jan 9, 2023

Congrool Jan 8, 2023

LaurenceLiZhixin Jan 9, 2023

Congrool Jan 8, 2023

LaurenceLiZhixin Jan 9, 2023

Congrool Jan 8, 2023

Congrool Jan 8, 2023

Congrool Jan 8, 2023

LaurenceLiZhixin Jan 9, 2023

Congrool Jan 9, 2023

Congrool Jan 12, 2023

Congrool Jan 8, 2023

LaurenceLiZhixin Jan 9, 2023

Congrool Jan 9, 2023

Congrool Jan 8, 2023

LaurenceLiZhixin Jan 9, 2023

Congrool Jan 8, 2023

LaurenceLiZhixin Jan 9, 2023

Congrool Jan 9, 2023

Congrool Jan 12, 2023

Congrool commented Jan 8, 2023 •

edited

Loading

LaurenceLiZhixin commented Jan 9, 2023

openyurt-bot commented Jan 11, 2023

Fix: pool-coordinator #1126

Fix: pool-coordinator #1126

Conversation

LaurenceLiZhixin commented Jan 6, 2023

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

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

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

Congrool commented Jan 8, 2023 • edited Loading

LaurenceLiZhixin commented Jan 9, 2023

openyurt-bot commented Jan 11, 2023

Congrool commented Jan 8, 2023 •

edited

Loading