clustermesh-apiserver: extract kvstore client initialization and heartbeat logic in separate cells #25554
Conversation
giorio94
added
area/clustermesh
|
/test (Jenkins tests did not run due to the outage) |
|
/test |
pkg/option/config.go
Outdated
| v := vp.GetStringSlice(k) | ||
| if len(v) > 0 { | ||
| entry.Infof(" --%s='%s'", k, strings.Join(v, ",")) | ||
| ss := vp.GetStringSlice(k) |
There was a problem hiding this comment.
Should we move this commit into a separate PR so that it can be backported to all branches?
There was a problem hiding this comment.
It actually seems that flags.StringToString should not be used (https://github.com/cilium/cilium/blob/main/contrib/scripts/check-viper.sh#L20-L23). Let me try to figure out how to make option.NewNamedMapOptions work for hive flags. In that way, this commit should no longer be necessary.
There was a problem hiding this comment.
I've opened #25643 which includes the logic to correctly parse StringMap flags through the hive machinery, and the commit to fix the logging of that type of flags. Backporting is not required, since StringMap flags are not currently used (an intermediate wrapper is used to support both json and KV encoding for legacy flags).
giorio94
force-pushed
the
mio/kvstore-cell
branch
from
992767c
to
c22945b
Compare
|
Force-pushed to rebase on top of #25643. Marking as blocked until that PR is merged. |
giorio94
force-pushed
the
mio/kvstore-cell
branch
from
c22945b
to
a14d45c
Compare
|
Rebased onto main to include the changes introduced in #25643 |
giorio94
removed
the
dont-merge/blocked
|
/test |
This commit implements a cell which handles the operations required to setup the kvstore client, based on the specified parameters (which are also propagated to the existing global variables to prevent changes for functions relying on those). In this first pass, the cell is expected to be used for self-contained components (like the clustermesh-apiserver), which don't involve complex dependencies during initialization. The kvstore client is returned through a promise, accounting for a possibly slow setup phase. The client is not explicitly closed when the cell is stopped, since that would attempt to revoke the lease, causing all entries associated with that lease to be deleted. This would not be the behavior expected by the consumers of this cell. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
This commit refactors the clustermesh-apiserver to use the newly introduced kvstore cell to initialize the etcd client. In order to reduce the amount of changes in this first phase, given the parallel refactoring already in progress, the backend is stored as a global variable and accessed by the different functions. The global variable will be removed in future commits, appropriately passing around the backend instance. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
This commit extracts the logic which periodically updates the heartbeat key in the kvstore in a separate cell, to reduce the coupling and allow it to be reused. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94
force-pushed
the
mio/kvstore-cell
branch
from
a14d45c
to
74e3659
Compare
|
Rebased onto main to fix a conflict. I've additionally dropped a leftover kvstore initialization, which caused the spurious creation of an additional etcd client: --- a/clustermesh-apiserver/main.go
+++ b/clustermesh-apiserver/main.go
@@ -576,21 +588,19 @@ func startServer(startCtx hive.HookContext, clientset k8sClient.Clientset, servi
}
var err error
- if err = kvstore.Setup(context.Background(), "etcd", option.Config.KVStoreOpt, nil); err != nil {
- log.WithError(err).Fatal("Unable to connect to etcd")
- }
config := cmtypes.CiliumClusterConfig{
ID: cfg.clusterID,
}
@@ -602,6 +612,7 @@ func startServer(startCtx hive.HookContext, clientset k8sClient.Clientset, servi
}
ciliumNodeStore, err = store.JoinSharedStore(store.Configuration{
+ Backend: backend,
Prefix: nodeStore.NodeStorePrefix,
KeyCreator: nodeStore.KeyCreator,
}) |
|
/test |
|
/test-1.24-5.4 |
maintainer-s-little-helper
bot
added
the
ready-to-merge
This PR extracts the kvstore client initialization and heartbeat logic from the clustermesh-apiserver in two separate cells, to better integrate in the hive paradigm and allow them to be reused by other components. In this phase, the clustermesh-apiserver still relies on a global variable for the kvstore client, which is planned to be removed with subsequent refactoring (in particular, it involves much less changes once #25049 is in).