helm: kvstoremesh with external etcd by calcium6 · Pull Request #34174 · cilium/cilium

calcium6 · 2024-08-05T07:41:11Z

When running clustermesh-apiserver, there should be an option to enable and disable containers that are not needed.
For example the etcd and apiserver containers are not necessary when running kvstoremesh with an external kvstore.

This commit adds the options to disable these containers.

maintainer-s-little-helper · 2024-08-05T07:41:14Z

Commit 4081260 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

gandro · 2024-08-05T08:07:48Z

Thanks for the PR! I'll defer the initial review to the @cilium/sig-clustermesh team, since I lack the necessary domain knowledge to judge correctness.

Please also remove the merge commit from your branch, as we don't use merge commits in the Cilium project. Please use git rebase instead.

marseel · 2024-08-12T10:38:31Z

Thanks @calcium6 for PR I just have one question to make sure I understand it.

For example the etcd and apiserver containers are not necessary when running kvstoremesh with an external kvstore.

AFAIK kvstoremsh does not work with external kvstore right now / it's not officially supported.
Is the goal of this PR to introduce a new way of deploying it that allows for kvstoremesh container to propagate data across external kvstores? If so, I know that @hemanthmalla was also interested in that and we have some discussions around that.
We would probably need some documentation and testing on top of that as well.

oblazek · 2024-08-14T06:56:43Z

hey @marseel.. yes the ultimate goal is to propagate data across external kvstores ;) this is just one of the pieces, do you want us to send it as one PR? (with docs, tests etc..) :)

marseel · 2024-08-14T07:42:19Z

hey Marcel Zięba.. yes the ultimate goal is to propagate data across external kvstores ;) this is just one of the pieces, do you want us to send it as one PR? (with docs, tests etc..) :)

nah, it's fine to do it in smaller parts. I was just lacking context. cc @hemanthmalla I know that you were looking into this as well.

In this case, wouldn't it be better to have a single helm flag, something like kvstoremeshExternalKVStore instead of two separate flags? I don't think there is a case when we want to disable one of two of apiserver/Etcd.

oblazek · 2024-08-14T07:56:21Z

yeah, let's do that.. @calcium6 please let's fix this

hemanthmalla · 2024-08-14T13:53:05Z

@marseel Sorry, missed the first ping. Yes, we already run clustermesh-apiserver in this mode. But we don't use the upstream chart. IIRC, when I was testing this there are few validations we'll need to disable as well.

We brought this up in one of the community meetings as well, but we ended up deciding maybe we shouldn't document too many variations of clustermesh that might confuse users. But with enough users interested in this, maybe the timing is right :)

giorio94 · 2024-08-19T13:30:30Z

Generally speaking, it makes sense to me to have the possibility of using kvstoremesh in combination with an external kvstore. I wonder whether an equally valid use case could be kvstoremesh+sidecar etcd, when Cilium operates in kvstore mode (i.e., just omitting the apiserver container, which is not necessary in that case).

As mentioned by Marcel, the clustermesh conformance/upgrade tests would need to be extended to cover this configuration as well (that should be pretty trivial, as we are already testing cilium in kvstore mode in a few matrix entries there).

Additionally, there's at least one known issue due to the fact that kvstoremesh does not update the Cilium's heartbeat key in etcd, which is used by the status checks performed by Cilium agents. Although this is not a problem when reusing the same etcd cluster of Cilium (as the hearbeat is updated by the operator), it would not work if using a dedicated etcd cluster for kvstoremesh. The fix may be as trivial as registering the heartbeat cell in the kvstoremesh case as well, although guarded by a dedicated flag (to avoid having two components updating the heartbeat in parallel).

Moreover, please keep in mind that kvstoremesh does not currently perform an explicit reconciliation on restart, as it works under the assumption that the etcd instance is ephemeral and recreated from scratch every time. This means that, when using a persistent etcd cluster, possible cached entries disappearing while kvstoremesh is down will not be deleted from the cache until the corresponding lease expires (15 minutes by default, but configurable).

In this case, wouldn't it be better to have a single helm flag, something like kvstoremeshExternalKVStore instead of two separate flags?

I'm also personally in favor of an explicit flag, to prevent configuring unsupported/invalid combinations. If we decide that kvstoremesh+sidecar etcd is a valid use case, then we may have a mode flag that allows selecting among the three possible alternatives.

giorio94 · 2024-08-19T13:35:06Z

examples/kvstoremesh/kvstoremesh.yaml

I'd suggest to add a note to the documentation on how to deploy kvstoremesh in this context, rather than adding an example here. Examples tend to be easily missed and get stale over time, as well as require a-priory knowledge about the specific goal, as lacking the accompanying description.

maintainer-s-little-helper · 2024-08-29T14:33:40Z

Commit 9d20080 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2024-08-29T14:35:13Z

Commit 7d4d29e does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

have the option to use external etcd with kvstoremesh Signed-off-by: Jiri Vanura <[email protected]>

ghost · 2024-09-02T08:43:18Z

Documentation/network/clustermesh/clustermesh.rst

  cilium install --set cluster.name=$CLUSTER1 --set cluster.id=1 --context $CLUSTER1
  cilium install --set cluster.name=$CLUSTER2 --set cluster.id=2 --context $CLUSTER2

+.. note::


This note in "Specify the Cluster Name and ID" section seems out of place. How about moving it to "Enable Cluster Mesh" section?

I believe this comment has already been resolved by the original submitter:

https://github.com/cilium/cilium/pull/34174/files#diff-8c6c2ac85134f66ed70f9aca62524e18ac4b8003f7d41101521d023e491f3f82R199

giorio94

Thanks for the changes.

I've left a few additional comments inline. Overall, the main concern I have about the current proposal is that it would work in a very specific use-case only (and with specific settings, which are also not documented), while leading to a most likely broken configuration otherwise. I personally think that it should be generalized a bit so that it works out of the box in most common scenarios, as well as the validation should be extended to provide safe-guards and prevent users from enabling unsupported configurations. My comments mostly go in that direction. WDYT?

giorio94 · 2024-09-03T15:28:11Z

install/kubernetes/cilium/values.yaml.tmpl

+  # -- Use external KVStore for KVStoreMesh.
+  kvstoremeshExternalKVStore: false


The dedicated clustermesh.kvstoremesh section seems a better fit to me. Additionally, I'd clarify in the comment that the target etcd instance is the one configured via etcd.endpoints, as not obvious otherwise.

Do you have any opinions on:

If we decide that kvstoremesh+sidecar etcd is a valid use case, then we may have a mode flag that allows selecting among the three possible alternatives.

Mostly to decide whether to generalize the setting to potentially enable this combination in the future (it doesn't need to be as part of this PR, considering that it requires a bit of extra work). Still, I'd rather prefer to pick a future-proof setting from the beginning if possible, to avoid having to deal with backward-compatible changes.

Yes, I also think it would be better to add a mode flag to select from the three options, I'll do that.

I believe this comment has already been resolved by the original submitter:

https://github.com/cilium/cilium/pull/34174/files#diff-0ea42ad21164b19bec1732225e254d3096d1e4040481c00053669287d81015feR3055

giorio94 · 2024-09-03T15:29:38Z

install/kubernetes/cilium/templates/validate.yaml

 {{/* validate clustermesh-apiserver */}}
 {{- if .Values.clustermesh.useAPIServer }}
-  {{- if and (ne .Values.identityAllocationMode "crd") (ne .Values.identityAllocationMode "doublewrite-readkvstore") (ne .Values.identityAllocationMode "doublewrite-readcrd") }}
+  {{- if and (ne .Values.identityAllocationMode "crd") (ne .Values.identityAllocationMode "doublewrite-readkvstore") (ne .Values.identityAllocationMode "doublewrite-readcrd") (not .Values.clustermesh.kvstoremeshExternalKVStore) }}


I'd suggest adding a similar validation to ensure that the external kvstore mode can only be enabled if etcd.enabled is set to true, and etcd.endpoints is not empty, to avoid surprises (given the current implementation). Similarly, I'd suggest validating that kvstoremesh.enabled=true when useAPIServer=true and kvstoremeshExternalKVStore=true, otherwise the resulting deployment is invalid. Finally, it should be forbidden to enable externalWorkloads.enabled=true if kvstoremeshExternalKVStore=true.

I believe this comment has already been resolved: https://github.com/cilium/cilium/pull/34174/files#diff-63d6e1fdb929a5058c43790c57f422863b269e45c2be10eb6564ca51fe1a0f4eR171

Documentation/network/clustermesh/clustermesh.rst

giorio94 · 2024-09-03T15:44:40Z

install/kubernetes/cilium/templates/clustermesh-apiserver/deployment.yaml

The creation of the clustermesh-apiserver specific ClusterRole and ClusterRoleBinding should be disabled when running only KVStoreMesh, as not required. Similarly, the creation of the clustermesh-apiserver Service should be disabled, and the clustermesh-apiserver-metrics Service/ServiceMonitor should be adapted to disable all but KVStoreMesh-related entries. Finally, the creation of the clustermesh-remote-users ConfigMap and of the different TLS-related secrets should be most likely disabled as well.

I believe this coment has already been resolved by the original submiter.

giorio94 · 2024-09-03T15:51:11Z

install/kubernetes/cilium/templates/clustermesh-apiserver/deployment.yaml

        {{- end }}
+      {{- end }}
      containers:
+      {{- if not .Values.clustermesh.kvstoremeshExternalKVStore}}


There are a bunch of volumes configured for this deployment that should be skipped when running in KVStoreMesh-only mode, as not mounted in that case.

I believe this comment has already beed resolved:

https://github.com/cilium/cilium/pull/34174/files#diff-c85fb06a8dab414080c30e0cf3361b9baade1b357dd2d90e1a1797be2dbdcf31R358

https://github.com/cilium/cilium/pull/34174/files#diff-c85fb06a8dab414080c30e0cf3361b9baade1b357dd2d90e1a1797be2dbdcf31R336

giorio94 · 2024-09-03T16:08:03Z

install/kubernetes/cilium/templates/clustermesh-config/clustermesh-secret.yaml

 data:
  {{- $kvstoremesh := and .Values.clustermesh.useAPIServer .Values.clustermesh.apiserver.kvstoremesh.enabled }}
-  {{- $override := ternary (printf "https://clustermesh-apiserver.%s.svc:2379" .Release.Namespace) "" $kvstoremesh }}
+  {{- $override := ternary (join "\n- " $.Values.etcd.endpoints) (ternary (printf "https://clustermesh-apiserver.%s.svc:2379" .Release.Namespace) "" $kvstoremesh) .Values.clustermesh.kvstoremeshExternalKVStore }}


When external KVStore mode is enabled, the Cilium agents appear to default using the etcd endpoints configured via .Values.etcd.endpoints. However, this change does not seem to account for the handling of the TLS certificates, accounting for .Values.etcd.ssl. Similarly, neither the endpoints nor the certificates seem to be automatically configured in the kvstoremesh container, which would not work out of the box.

giorio94 · 2024-09-03T16:11:08Z

install/kubernetes/cilium/templates/validate.yaml

+  {{- if and (ne .Values.identityAllocationMode "crd") (ne .Values.identityAllocationMode "doublewrite-readkvstore") (ne .Values.identityAllocationMode "doublewrite-readcrd") (not .Values.clustermesh.kvstoremeshExternalKVStore) }}
    {{ fail (printf "The clustermesh-apiserver cannot be enabled in combination with .Values.identityAllocationMode=%s. To establish a Cluster Mesh, directly configure the parameters to access the remote kvstore through .Values.clustermesh.config" .Values.identityAllocationMode ) }}
  {{- end }}
  {{- if .Values.disableEndpointCRD }}


The disableEndpointCRD validation can be skipped if kvstoremeshExternalKVStore=true.

I believe this comment has already been resolved: https://github.com/cilium/cilium/pull/34174/files#diff-63d6e1fdb929a5058c43790c57f422863b269e45c2be10eb6564ca51fe1a0f4eR155

giorio94

(Requesting changes as per above comments)

changed the flag name clustermesh.kvstoremeshExternalKVStore to clustermesh.apiserver.kvstoremesh.useExternalKVStore Signed-off-by: Jiri Vanura <[email protected]>

changed the flag clustermesh.apiserver.kvstoremesh.useExternalKVStore to clustermesh.apiserver.kvstoremesh.kvstoreMode Signed-off-by: Jiri Vanura <[email protected]>

github-actions · 2024-10-27T02:03:01Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

github-actions · 2024-11-11T01:59:15Z

This pull request has not seen any activity since it was marked stale.
Closing.

balous · 2024-11-13T15:18:07Z

I'd like to revive this PR. I checked all the PR comments and think that most of them were addressed by the original submitter. The only exception I see is the TLS certificate comment, I am going to address it myself. Should I post a new PR with my new commit(s)?

giorio94 · 2024-11-13T16:24:20Z

Should I post a new PR with my new commit(s)?

Hey @balous, yes, I guess that's the easiest way given that this one got closed due to inactivity.
Please, link the new one here as well.

balous · 2024-11-27T19:35:33Z

Should I post a new PR with my new commit(s)?

Hey @balous, yes, I guess that's the easiest way given that this one got closed due to inactivity. Please, link the new one here as well.

Here it is: #36216

I believe all comments from this PR were addressed (especially the TLS config one) additional fix in metrics-service.yaml was made.

calcium6 requested review from a team as code owners August 5, 2024 07:41

calcium6 requested review from joamaki and jrajahalme August 5, 2024 07:41

calcium6 requested a review from a user August 5, 2024 07:41

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 5, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 5, 2024

calcium6 force-pushed the kvstoremesh-deploy branch from 4081260 to e1b150b Compare August 5, 2024 08:25

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 5, 2024

gandro 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/helm Impacts helm charts and user deployment experience labels Aug 5, 2024

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 5, 2024

aanm requested a review from giorio94 August 9, 2024 13:33

giorio94 reviewed Aug 19, 2024

View reviewed changes

calcium6 marked this pull request as draft August 29, 2024 08:49

calcium6 closed this Aug 29, 2024

calcium6 force-pushed the kvstoremesh-deploy branch from e1b150b to 21700f1 Compare August 29, 2024 13:04

calcium6 reopened this Aug 29, 2024

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Aug 29, 2024

calcium6 changed the title ~~helm: be able to run only Kvstoremesh container in clustermesh-apiserver~~ helm: external etcd with kvstoremesh Aug 29, 2024

calcium6 force-pushed the kvstoremesh-deploy branch from 9d20080 to 7d4d29e Compare August 29, 2024 14:35

calcium6 changed the title ~~helm: external etcd with kvstoremesh~~ helm: kvstoremesh with external etcd Aug 29, 2024

calcium6 force-pushed the kvstoremesh-deploy branch from 7d4d29e to 1f86f38 Compare August 29, 2024 14:38

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 29, 2024

helm: kvstoremesh with external etcd

a975482

have the option to use external etcd with kvstoremesh Signed-off-by: Jiri Vanura <[email protected]>

calcium6 force-pushed the kvstoremesh-deploy branch from 1f86f38 to a975482 Compare August 29, 2024 14:40

calcium6 marked this pull request as ready for review August 29, 2024 14:51

ghost reviewed Sep 2, 2024

View reviewed changes

giorio94 self-requested a review September 3, 2024 14:14

giorio94 reviewed Sep 3, 2024

View reviewed changes

giorio94 requested changes Sep 3, 2024

View reviewed changes

calcium6 marked this pull request as draft September 9, 2024 06:16

helm: validation

6c796c7

changed the flag name clustermesh.kvstoremeshExternalKVStore to clustermesh.apiserver.kvstoremesh.useExternalKVStore Signed-off-by: Jiri Vanura <[email protected]>

calcium6 force-pushed the kvstoremesh-deploy branch from 3430b21 to 6c796c7 Compare September 12, 2024 11:06

helm: kvstore modes for kvstoremesh

97bd2e4

changed the flag clustermesh.apiserver.kvstoremesh.useExternalKVStore to clustermesh.apiserver.kvstoremesh.kvstoreMode Signed-off-by: Jiri Vanura <[email protected]>

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 27, 2024

github-actions bot closed this Nov 11, 2024

balous mentioned this pull request Nov 27, 2024

helm: kvstoremesh with external etcd #36216

Merged

balous mentioned this pull request May 6, 2025

Kvstoremesh heartbeat with external etcd #39356

Closed

3 tasks

giorio94 mentioned this pull request May 19, 2025

helm: possibility to use kvstoremesh in kvstore identity allocation mode #39523

Merged

6 tasks

		# -- Use external KVStore for KVStoreMesh.
		kvstoremeshExternalKVStore: false

Conversation

calcium6 commented Aug 5, 2024

Uh oh!

maintainer-s-little-helper bot commented Aug 5, 2024

Uh oh!

gandro commented Aug 5, 2024

Uh oh!

marseel commented Aug 12, 2024

Uh oh!

oblazek commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marseel commented Aug 14, 2024

Uh oh!

oblazek commented Aug 14, 2024

Uh oh!

hemanthmalla commented Aug 14, 2024

Uh oh!

giorio94 commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maintainer-s-little-helper bot commented Aug 29, 2024

Uh oh!

maintainer-s-little-helper bot commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giorio94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giorio94 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 27, 2024

Uh oh!

github-actions bot commented Nov 11, 2024

Uh oh!

balous commented Nov 13, 2024

Uh oh!

giorio94 commented Nov 13, 2024

Uh oh!

balous commented Nov 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

oblazek commented Aug 14, 2024 •

edited

Loading