Replace etcd init script and bump etcd version #29109

JamesLaverack · 2023-11-10T16:30:12Z

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

This is a follow-on PR from #26352, that rebases much of the work and changes the approach.

This change replaces the etcd init script that is normally executed in the etcd container when starting clustermesh-apiserver. Instead the etcd binary is bundled into the clustermesh-apiserver container image, and a new command is added in Go that will start the server and initalise the data directory.

This is intended as a direct replacement of the current etcd in clustermesh deployments.

Previously, we have been unable to update the etcd image as the script requires sh to be present, which isn't in newer etcd images that have moved to distroless.

Fixes: #23770

Replace etcd init script used for clustermesh with a Go equivalent.
Upgrade etcd to v3.5.10.

JamesLaverack · 2023-11-10T16:30:36Z

/ci-clustermesh

clustermesh-apiserver/etcdinit/root.go

images/clustermesh-apiserver/Dockerfile

pkg/etcd/init/init.go

install/kubernetes/cilium/values.yaml.tmpl

giorio94

Nice work @JamesLaverack!

The overall structure looks good to me. You can find a few comments and suggestions inline.

clustermesh-apiserver/etcdinit/root.go

pkg/etcd/init/init.go

maintainer-s-little-helper · 2023-11-14T19:28:15Z

Commits f9abac2, ecdf98c, 86e0deb do 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 · 2023-11-14T19:44:15Z

Commits f9abac2, ecdf98c, 86e0deb, 7f5476e do 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 · 2023-11-14T19:50:41Z

Commits f9abac2, ecdf98c, 86e0deb, 7f5476e, 7504106 do 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 · 2023-11-14T19:59:43Z

Commits f9abac2, ecdf98c, 86e0deb, 7f5476e, 7504106, 234086c do 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

Helm changes look good to me, thanks!

This change replaces the etcd init script that is normally executed in the etcd container when starting clustermesh-apiserver. Instead the etcd binary is bundled into the clustermesh-apiserver container image, and a new command is added in Go that will start the server and init the data directory. This is intended as a direct replacement of the current etcd in clustermesh deployments. Previously, we have been unable to update the etcd image as the script requires sh to be present, which isn't in newer etcd images that have moved to distroless. Signed-off-by: James Laverack <james@isovalent.com>

JamesLaverack · 2023-11-15T16:53:39Z

/test

JamesLaverack · 2023-11-16T03:11:29Z

Looks like a flake on one of the conformance tests?

/test

giorio94 · 2023-11-16T08:12:04Z

Looks like a flake on one of the conformance tests?

/test

FYI, you can restart a single test using the trigger phrase part of the workflow name (e.g., ci-clustermesh). Flakes should also be tracked linking them to an existing issue when present, or creating a new one.

giorio94

Looks great to me 🚀, thanks @JamesLaverack!

Just a few final nits inline (feel free to ignore them if you don't want to go through a new round of CI testing though).

clustermesh-apiserver/etcdinit/root.go

install/kubernetes/cilium/values.yaml.tmpl

pkg/kvstore/etcdinit/init.go

clustermesh-apiserver/etcdinit/root.go

giorio94 · 2023-11-16T08:26:22Z

Removing the ready to merge label, to wait for Tom's review.

tommyp1ckles · 2023-11-21T04:14:48Z

@JamesLaverack @giorio94 sorry for the late review, looks good

This is a follow on from the review of cilium#29109, and is a collection of minor changes: - Remove unused variables in install/kubernetes Makefile values file - Remove etcd image from check-docker-images.sh script - Remove now-unused external dependencies block from check-docker-images.sh script - Clarify doc comment for ClusterMeshEtcdInit function, to correctly state the purpose of the hasConfig key - defer etcdClient.Close() after creating etcdClient - Use errors.Join to handle errors in defered cleanup code, where the main function may have also returned an error - Correct typo in comment: "it's" to "its" Signed-off-by: James Laverack <james@isovalent.com>

This is a follow on from the review of #29109, and is a collection of minor changes: - Remove unused variables in install/kubernetes Makefile values file - Remove etcd image from check-docker-images.sh script - Remove now-unused external dependencies block from check-docker-images.sh script - Clarify doc comment for ClusterMeshEtcdInit function, to correctly state the purpose of the hasConfig key - defer etcdClient.Close() after creating etcdClient - Use errors.Join to handle errors in defered cleanup code, where the main function may have also returned an error - Correct typo in comment: "it's" to "its" Signed-off-by: James Laverack <james@isovalent.com>

This is a follow on from the review of cilium#29109, and is a collection of minor changes: - Remove unused variables in install/kubernetes Makefile values file - Remove etcd image from check-docker-images.sh script - Remove now-unused external dependencies block from check-docker-images.sh script - Clarify doc comment for ClusterMeshEtcdInit function, to correctly state the purpose of the hasConfig key - defer etcdClient.Close() after creating etcdClient - Use errors.Join to handle errors in defered cleanup code, where the main function may have also returned an error - Correct typo in comment: "it's" to "its" Signed-off-by: James Laverack <james@isovalent.com>

JamesLaverack requested review from a team as code owners November 10, 2023 16:30

JamesLaverack requested review from tommyp1ckles, gandro and thorn3r November 10, 2023 16:30

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 10, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 10, 2023

giorio94 self-requested a review November 10, 2023 16:43

JamesLaverack force-pushed the etcd-init-clustermesh-single-image branch 3 times, most recently from c65e8e1 to 37f9e79 Compare November 10, 2023 17:43

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. labels Nov 10, 2023

maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 10, 2023

tommyp1ckles reviewed Nov 11, 2023

View reviewed changes

tommyp1ckles removed the kind/community-contribution This was a contribution made by a community member. label Nov 11, 2023

giorio94 requested changes Nov 13, 2023

View reviewed changes

thorn3r removed their request for review November 13, 2023 14:06

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 14, 2023

JamesLaverack requested a review from a team as a code owner November 14, 2023 19:44

JamesLaverack force-pushed the etcd-init-clustermesh-single-image branch from 234086c to 59bd78e Compare November 14, 2023 20:02

gandro approved these changes Nov 15, 2023

View reviewed changes

JamesLaverack force-pushed the etcd-init-clustermesh-single-image branch from cf6cb44 to 94ee894 Compare November 15, 2023 14:58

gandro approved these changes Nov 15, 2023

View reviewed changes

maintainer-s-little-helper bot requested a review from giorio94 November 16, 2023 07:56

giorio94 approved these changes Nov 16, 2023

View reviewed changes

maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Nov 16, 2023

giorio94 removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 16, 2023

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 Nov 16, 2023

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 Nov 16, 2023

giorio94 requested a review from tommyp1ckles November 20, 2023 08:11

giorio94 mentioned this pull request Nov 20, 2023

ClusterMesh uses an etcd image that's not multi-arch #27899

Closed

2 tasks

tommyp1ckles approved these changes Nov 21, 2023

View reviewed changes

giorio94 removed the dont-merge/bad-bot To prevent MLH from marking ready-to-merge. label Nov 21, 2023

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 Nov 21, 2023

julianwiedmann merged commit 4bfc508 into cilium:main Nov 21, 2023
61 checks passed

JamesLaverack deleted the etcd-init-clustermesh-single-image branch November 21, 2023 09:09

JamesLaverack mentioned this pull request Nov 29, 2023

Follow-up nits from etcd init script pull request #29489

Merged

8 tasks

JamesLaverack mentioned this pull request Apr 17, 2024

Add jameslaverack as member cilium/community#103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace etcd init script and bump etcd version #29109

Replace etcd init script and bump etcd version #29109

JamesLaverack commented Nov 10, 2023 •

edited

Loading

JamesLaverack commented Nov 10, 2023

giorio94 left a comment

maintainer-s-little-helper bot commented Nov 14, 2023

maintainer-s-little-helper bot commented Nov 14, 2023

maintainer-s-little-helper bot commented Nov 14, 2023

maintainer-s-little-helper bot commented Nov 14, 2023

gandro left a comment

JamesLaverack commented Nov 15, 2023

JamesLaverack commented Nov 16, 2023

giorio94 commented Nov 16, 2023

giorio94 left a comment

giorio94 commented Nov 16, 2023

tommyp1ckles commented Nov 21, 2023

Replace etcd init script and bump etcd version #29109

Replace etcd init script and bump etcd version #29109

Conversation

JamesLaverack commented Nov 10, 2023 • edited Loading

JamesLaverack commented Nov 10, 2023

giorio94 left a comment

Choose a reason for hiding this comment

maintainer-s-little-helper bot commented Nov 14, 2023

maintainer-s-little-helper bot commented Nov 14, 2023

maintainer-s-little-helper bot commented Nov 14, 2023

maintainer-s-little-helper bot commented Nov 14, 2023

gandro left a comment

Choose a reason for hiding this comment

JamesLaverack commented Nov 15, 2023

JamesLaverack commented Nov 16, 2023

giorio94 commented Nov 16, 2023

giorio94 left a comment

Choose a reason for hiding this comment

giorio94 commented Nov 16, 2023

tommyp1ckles commented Nov 21, 2023

JamesLaverack commented Nov 10, 2023 •

edited

Loading