Remove etcd.managed Helm setting #32921
Conversation
|
/test |
There was a problem hiding this comment.
Thanks a lot for this cleanup! 🧹
The following helm values entries could now be deleted as well, as no longer used:
- https://github.com/cilium/cilium/blob/pr%2Fjoe%2Fdrop-managed-etcd/install/kubernetes/cilium/values.yaml.tmpl#L2406-L2492
- https://github.com/cilium/cilium/blob/pr%2Fjoe%2Fdrop-managed-etcd/install/kubernetes/cilium/values.yaml.tmpl#L2497-L2498
- https://github.com/cilium/cilium/blob/pr%2Fjoe%2Fdrop-managed-etcd/install/kubernetes/cilium/values.yaml.tmpl#L97-L101
And these comments should be updated not to mention managed anymore:
d9bed3b to
7275280
Compare
7275280 to
a605fda
Compare
|
Good pointers @giorio94 . This time I did a deeper clean by scanning through the output of |
a605fda to
5a96978
Compare
|
/test |
I've addressed your feedback, please take another look.
|
ci-ingress hit known CI issue #31857 |
|
/ci-ingress |
There was a problem hiding this comment.
The first commit looks good to me, thanks!
I'm not sure we can merge the second one straight away without going through a proper deprecation process though, as I'm pretty sure that there are users who are still running etcd in pod network leveraging other operators (we never officially deprecated that support AFAIK), but relying upon the same reserved identities. Similarly, there are quite a few other pieces of logic spread across the code-base to enable such setup (e.g., the CRD to kvstore watch handover), which would also be need to be considered if we were to completely remove that support.
I'd also personally preserve the logic to automatically translate DNS names matching k8s services to the corresponding ClusterIP, as potentially useful in certain circumstances (and harmless in the others). Related to this, I've recently opened a PR to generalize and decouple that logic from the etcd.managed setting, which should already largely simplify that part.
|
Probably it makes sense for me to break this down and drop the second commit for now, stick to just the Helm changes then we can revisit the other changes down the line. Thanks for the feedback @giorio94 . |
Yeah, I totally agree with removing it. I'd just not break the support for etcd running in pod network yet, as slightly different, and not formally deprecated AFAIK.
SGTM, thanks! You may want to include the |
5a96978 to
99f4a7f
Compare
|
/test |
The etcd-operator Helm templates rely on a piece of software which is no longer maintained upstream, and it relies on outdated CRDs which are no longer supported since Kubernetes 1.22. The setting has been hidden and not documented for several releases, we can remove it now. Signed-off-by: Joe Stringer <joe@cilium.io>
99f4a7f to
1f4ae6a
Compare
|
/test |
The etcd-operator Helm templates rely on a piece of software which is
no longer maintained upstream, and it relies on outdated CRDs which are
no longer supported since Kubernetes 1.22. The setting has been hidden
and not documented for several releases, we can remove it now.