Convert the clustermesh subsystem into a hive.Cell #25561

giorio94 · 2023-05-19T16:57:01Z

This PR refactors the clustermesh subsystem into a cell, which is registered in the controlplane module. Please refer to the individual commit messages for more details about the preliminary steps.

Convert the clustermesh subsystem into a hive.Cell

YutaroHayakawa

Looks good to me assuming your last force push only changed the cmdref.

giorio94 · 2023-05-22T07:08:33Z

Looks good to me assuming your last force push only changed the cmdref.

Yes, the force push was only to fix the autogenerated cmdref: https://github.com/cilium/cilium/compare/e799226a916e3ca5767c0fc7f4c9696c7c38398d..8bad0ae5e9ad41b637472d375702e981b5147bfa

giorio94 · 2023-05-22T07:26:51Z

TravisCI failed with the same error reported in #25272. Rerunning

giorio94 · 2023-05-22T07:34:07Z

Last force-push only fixed an out-of-date comment.

joamaki · 2023-05-22T08:18:29Z

pkg/clustermesh/cell.go

+	cell.Provide(NewClusterMesh),
+
+	// Convert concrete objects into more restricted interfaces used by clustermesh.
+	cell.ProvidePrivate(func(sc *k8s.ServiceCache) ServiceMerger { return sc }),


ProvidePrivate takes a variable number of constructors which might make this easier to read (matter of taste, I'm fine seeing either style used)

Thanks, I didn't notice that. Given that you don't have a specific preference, I'm going to keep it as it is now, because it seems slightly clearer to me.

giorio94 · 2023-05-22T09:01:19Z

ConformanceK8sKind failed but the logs don't mention any explicit failure. I'm going to rerun it and to see if the failure is consistent.

giorio94 · 2023-05-22T09:28:09Z

/test

giorio94 · 2023-05-22T12:54:03Z

/test-1.26-net-next

Hit known flake #25524

giorio94 · 2023-05-23T10:04:50Z

Force-pushed to rebase onto main and fix an import-related conflict.

giorio94 · 2023-05-23T10:04:56Z

/test

giorio94 · 2023-05-23T16:53:53Z

/test-1.26-net-next

giorio94 · 2023-05-23T16:55:51Z

Cilium Conformance E2E failed while provisioning the LVH VM: #25483. Manually restarting the affected matrix task

giorio94 · 2023-05-23T16:59:29Z

/test-runtime

Hit new flake: #25628

giorio94 · 2023-05-24T08:51:09Z

/test-runtime

Hit #25628 and #25637

squeed · 2023-05-24T12:07:26Z

pkg/clustermesh/clustermesh.go


 	// RemoteIdentityWatcher provides identities that have been allocated on a
 	// remote cluster.
 	RemoteIdentityWatcher RemoteIdentityWatcher

 	IPCache ipcache.IPCacher
+
+	// ClusterSizeDependantInterval allows to calculate intervals based on cluster size.
+	ClusterSizeDependantInterval kvstore.ClusterSizeDependantIntervalFunc `optional:"true"`


Nit: "Dependent"

I've used Dependant for consistency with the rest of the code base. Given that there are quite a lot of occurrences, I would prefer not to rename them all in this PR, as it would increase the number of modified files quite significantly.

heh, it's like HTTP referer all over again. Sure, I didn't realize it was so wide-spread.

squeed

A few minor nits, but looks good for my files

squeed · 2023-05-24T12:57:59Z

pkg/clustermesh/clustermesh.go

@@ -37,15 +38,12 @@ const (
 // Configuration is the configuration that must be provided to
 // NewClusterMesh()
 type Configuration struct {


Extremely minor nit: "Configuration" is a confusing name. It's really a set of dependencies, not a configuration.

But, like, it's also very much not a big deal.

That's right that it is not a pure configuration anymore. I'm not sure about a better name though, Dependencies also looks a bit weird when accessing for instance the cluster name as rc.mesh.deps.ClusterName (I personally feel that rc.mesh.conf.ClusterName is a bit more natural here).

Currently, the ServiceCache is created and stored as concrete object, rather than pointer type. Yet, this is problematic for the upcoming modification to provide it through the hive subsystem, because it contains a mutex and cannot be passed around. Hence, let's change it to be of pointer type and adapt the usages accordingly. We don't risk incurring in new nil pointer exceptions, because that struct already needed to be explicitly initialized as that it contains maps. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

Let's provide the ServiceCache instance through hive. This is one of the requirements to convert the clustermesh subsystem to a cell. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

The "PolicyTrifecta" is currently responsible to initialize the CachingIdentityAllocator, along others. Let's also return the RemoteIdentityWatcher sub-interface, which is one of the requirements to convert the clustermesh subsystem to a cell. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

giorio94 · 2023-05-25T08:11:41Z

Rebased onto main to fix a minor conflict

giorio94 · 2023-05-25T08:11:48Z

/test

daemon/cmd/daemon_main.go

lmb · 2023-05-25T16:15:00Z

pkg/clustermesh/clustermesh.go

+	return cm
+}
+
+func (cm *ClusterMesh) Start(hive.HookContext) error {


Ha, interesting! I kind of prefer the "cm implements a startable interface" rather than "cm appends hooks". I didn't realise how easy it is to convert from one to the other.

I'm not sure I fully got your comment. Are you suggesting to adopt a different approach?

pkg/clustermesh/clustermesh_test.go

lmb · 2023-05-25T16:19:22Z

pkg/clustermesh/clustermesh.go

@@ -247,12 +251,14 @@ func (cm *ClusterMesh) Close() {
 		cluster.onRemove()
 		delete(cm.clusters, name)
 	}
-	cm.controllers.RemoveAllAndWait()


This has probably moved somewhere, but I can't find where!

I have completely dropped this variable, since it was never actually used.

lmb · 2023-05-25T16:21:12Z

pkg/clustermesh/services_test.go

 	}, 10*time.Second), IsNil)
 }

 func (s *ClusterMeshServicesTestSuite) TearDownTest(c *C) {
-	if s.mesh != nil {
-		s.mesh.Close()
+	if s.hive != nil {


You might be able to do c.Cleanup in SetUpTest() now that #25484 is in.

I've dropped that cleanup part, since it is now handled by hivetest.Lifecycle.

pkg/clustermesh/types/types.go

lmb · 2023-05-25T16:22:56Z

pkg/kvstore/backend.go

@@ -23,14 +23,16 @@ type backendOption struct {

 type backendOptions map[string]*backendOption

+type ClusterSizeDependantIntervalFunc func(baseInterval time.Duration) time.Duration


Why did you turn this into a separate type?

No strong reason, it just seemed more idiomatic to me.

Introduce a new type that wraps the ClusterID and the ClusterName, so that is can be passed around through the hive machinery. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

This commit refactors the clustermesh subsystem into a cell, which is registered in the controlplane module. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

giorio94 · 2023-05-25T17:06:08Z

/test

thorn3r

LGTM 👍

giorio94 · 2023-05-26T07:30:49Z

Conformance E2E failed due to #25705. Manually rerunning.

giorio94 added area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/misc This PR makes changes that have no direct user impact. area/modularization labels May 19, 2023

giorio94 requested review from a team as code owners May 19, 2023 16:57

giorio94 requested review from thorn3r, squeed, aanm, christarazi and YutaroHayakawa May 19, 2023 16:57

giorio94 force-pushed the mio/clustermesh-cell branch from e799226 to 8bad0ae Compare May 22, 2023 06:53

YutaroHayakawa approved these changes May 22, 2023

View reviewed changes

giorio94 force-pushed the mio/clustermesh-cell branch from 8bad0ae to 3d5ecba Compare May 22, 2023 07:33

joamaki approved these changes May 22, 2023

View reviewed changes

giorio94 force-pushed the mio/clustermesh-cell branch from 3d5ecba to 97761cd Compare May 23, 2023 10:04

aanm approved these changes May 23, 2023

View reviewed changes

giorio94 mentioned this pull request May 23, 2023

CI: Test_PodCIDRAdvert (test-runtime) #25628

Closed

squeed reviewed May 24, 2023

View reviewed changes

squeed approved these changes May 24, 2023

View reviewed changes

squeed reviewed May 24, 2023

View reviewed changes

christarazi approved these changes May 24, 2023

View reviewed changes

giorio94 added 3 commits May 25, 2023 10:08

service-cache: initialize through the hive machinery

8e2c3a2

Let's provide the ServiceCache instance through hive. This is one of the requirements to convert the clustermesh subsystem to a cell. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

giorio94 force-pushed the mio/clustermesh-cell branch from 97761cd to 1c699c3 Compare May 25, 2023 08:11

giorio94 requested a review from a team as a code owner May 25, 2023 08:11

giorio94 requested a review from lmb May 25, 2023 08:11

lmb approved these changes May 25, 2023

View reviewed changes

giorio94 added 2 commits May 25, 2023 18:55

clustermesh: introduce the ClusterIDName type

80be6d8

Introduce a new type that wraps the ClusterID and the ClusterName, so that is can be passed around through the hive machinery. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

clustermesh: introduce hive.Cell

0d291a8

This commit refactors the clustermesh subsystem into a cell, which is registered in the controlplane module. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

giorio94 force-pushed the mio/clustermesh-cell branch from 1c699c3 to 0d291a8 Compare May 25, 2023 17:05

thorn3r approved these changes May 25, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 26, 2023

tklauser merged commit 69b5845 into cilium:main May 26, 2023
58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert the clustermesh subsystem into a hive.Cell #25561

Convert the clustermesh subsystem into a hive.Cell #25561

giorio94 commented May 19, 2023

YutaroHayakawa left a comment

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023

joamaki May 22, 2023

giorio94 May 22, 2023

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023 •

edited

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023 •

edited

giorio94 commented May 24, 2023 •

edited

squeed May 24, 2023

giorio94 May 24, 2023

squeed May 24, 2023

squeed left a comment

squeed May 24, 2023

giorio94 May 24, 2023

giorio94 commented May 25, 2023

giorio94 commented May 25, 2023

lmb May 25, 2023

giorio94 May 25, 2023

lmb May 25, 2023

giorio94 May 25, 2023

lmb May 25, 2023

giorio94 May 25, 2023

lmb May 25, 2023

giorio94 May 25, 2023

giorio94 commented May 25, 2023

thorn3r left a comment

giorio94 commented May 26, 2023

		@@ -23,14 +23,16 @@ type backendOption struct {

		type backendOptions map[string]*backendOption

		type ClusterSizeDependantIntervalFunc func(baseInterval time.Duration) time.Duration

Convert the clustermesh subsystem into a hive.Cell #25561

Convert the clustermesh subsystem into a hive.Cell #25561

Conversation

giorio94 commented May 19, 2023

YutaroHayakawa left a comment

Choose a reason for hiding this comment

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023

giorio94 commented May 22, 2023 • edited

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023

giorio94 commented May 23, 2023 • edited

giorio94 commented May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giorio94 commented May 25, 2023

giorio94 commented May 25, 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

giorio94 commented May 25, 2023

thorn3r left a comment

Choose a reason for hiding this comment

giorio94 commented May 26, 2023

giorio94 commented May 22, 2023 •

edited

giorio94 commented May 23, 2023 •

edited

giorio94 commented May 24, 2023 •

edited