Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Helm: add configuration options for kvstoremesh #26109

Merged
merged 3 commits into from Jun 16, 2023

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Jun 12, 2023

This PR is a followup of #26083, which extends the helm chart to allow configuring kvstoremesh.

Extend the Helm chart to allow configuring kvstoremesh.

@giorio94 giorio94 added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience dont-merge/blocked Another PR must be merged before this one. labels Jun 12, 2023
@giorio94 giorio94 requested review from a team as code owners June 12, 2023 09:49
@@ -2630,6 +2630,46 @@ clustermesh:
# cpu: 100m
# memory: 100Mi

kvstoremesh:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kvstoremesh configuration options are currently placed at this level for consistency with etcd, but I'm wondering whether they should be moved one level up. WDYT?

@giorio94 giorio94 added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Jun 12, 2023
Copy link
Contributor

@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just some minor questions/points.

Documentation/observability/metrics.rst Outdated Show resolved Hide resolved
# -- KVStoreMesh image.
image:
override: ~
repository: "quay.io/cilium/kvstoremesh-ci"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to be a CI image, is that intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've double-checked, and also the repositories for the other components point to CI images. It seems that they are sourced from here (and RELEASE is not set by default).

@giorio94
Copy link
Member Author

/test

@giorio94
Copy link
Member Author

giorio94 commented Jun 13, 2023

/ci-aks

Failed due to #26075

@giorio94
Copy link
Member Author

Force-pushed to explicitly set the etcd rate limit settings for kvstoremesh.

@giorio94
Copy link
Member Author

/test

@joestringer joestringer added the kind/enhancement This would improve or streamline existing functionality. label Jun 14, 2023
@giorio94
Copy link
Member Author

Rebased onto main now that #26083 has been merged

@giorio94 giorio94 removed the dont-merge/blocked Another PR must be merged before this one. label Jun 15, 2023
@giorio94
Copy link
Member Author

/test

Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giorio94 Good work. Approving to unblock with the understanding that changes are required prior to merge.

Documentation/observability/metrics.rst Outdated Show resolved Hide resolved
Documentation/observability/metrics.rst Outdated Show resolved Hide resolved
@maintainer-s-little-helper 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 Jun 16, 2023
@giorio94 giorio94 added dont-merge/bad-bot To prevent MLH from marking ready-to-merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jun 16, 2023
Add the `clustermesh-apiserver.{namespace}.svc` DNS name to the
clustermesh-apiserver etcd certificate, so that the secure
connection can be successfully established when connecting to the
local etcd instance through the service, rather than to a remote one
through the corresponding LB/NodePort service or an external DNS name.
This is required in particular in the kvstoremesh case.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
The clustermesh-apiserver runs in pod network, hence there's no
shortcoming in enabling those metrics by default.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
This commit extends the helm chart to allow configuring kvstoremesh. In
particular, the clustermesh-apiserver deployment is enriched with the
additional kvstoremesh sidecar container (when kvstoremesh is enabled),
appropriately mounting the secret containing the remote kvstore
configurations. Additionally, the configuration used by the agents is
modified to connect to the local kvstore instance (through the
corresponding service) instead of the remote ones.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
Copy link
Member Author

/test

@giorio94 giorio94 removed the dont-merge/bad-bot To prevent MLH from marking ready-to-merge. label Jun 16, 2023
@maintainer-s-little-helper 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 Jun 16, 2023
@borkmann borkmann merged commit bd53860 into cilium:main Jun 16, 2023
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/helm Impacts helm charts and user deployment experience kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants