Update etcd-manager to v3.0.20250629 #17452
Conversation
|
Skipping CI for Draft Pull Request. |
|
/test pull-kops-e2e-k8s-aws-calico |
|
pull-kops-e2e-k8s-aws-calico succeeded using the new |
| - hostPath: | ||
| path: /etc/ssl | ||
| type: Directory | ||
| name: ca-certificates |
There was a problem hiding this comment.
I'm a little torn about this. On the one hand, it's great that we pick up system ca-certificates so if they update we don't have to do anything (although in practice these don't change fast, but I guess if someone wanted to install their own in-house ca-certificate this now work). On the other hand, using hostPath to pick up ca-certificates isn't the general pattern for other pods, and I don't know if we're going to have new problems.
Given it is etcd-manager, which is really a system component and in another universe could run as a systemd service, I say we run with it!
There was a problem hiding this comment.
Would be interesting if we could use https://github.com/kubernetes/kubernetes/tree/master/build/build-image.
| @@ -558,7 +564,7 @@ func (b *EtcdManagerBuilder) buildPod(etcdCluster kops.EtcdClusterSpec, instance | |||
| } | |||
|
|
|||
| { | |||
| container.Command = exec.WithTee("/etcd-manager", args, "/var/log/etcd.log") | |||
| container.Command = exec.WithTee("/ko-app/etcd-manager", args, "/var/log/etcd.log") | |||
There was a problem hiding this comment.
I wonder if we should use alsologtostderr and logfile instead of the tee "hack"
There was a problem hiding this comment.
The goal would be to just override args, and rely on Entrypoint being set correctly
There was a problem hiding this comment.
That would be nice, if possible.
|
/test pull-kops-e2e-k8s-aws-calico |
|
/test pull-kops-e2e-k8s-aws-calico |
|
/test pull-kops-e2e-k8s-aws-amazonvpc |
|
I'm surprised with the bazel-less builds there is a significant increase in image size. We also lose some reproducibility based on some timestamps changing: |
Bazel builds were based on |
|
@rifelpet Any idea about the failure in
|
|
Looks like RHEL/AL2023 use different paths for certs: // Possible directories with certificate files; all will be read.
var certDirectories = []string{
"/etc/ssl/certs", // SLES10/SLES11, https://golang.org/issue/12139
"/etc/pki/tls/certs", // Fedora/RHEL
} |
|
/test pull-kops-e2e-k8s-aws-amazonvpc |
One thing we could do is use bind mounting to "fix" this, making sure that the path is always in either a standard place we define, or alternatively bind mounting to e.g. /etc/kubernetes/etcd-manager/ca-certs/ so that we can do per-pod configuration. That way we could have a static manifest, which I personally think is easier to follow. It's overkill for this use case, because etcd-manager comes in so early though, so I think we probably should not do this in this PR (it's also a separable idea, so I think we should separate it into another PR if we do choose to pursue it). Do you think this PR is ready to merge? It is looking good... |
|
@justinsb I think for sure it can be improved, but we should merge it and get some testgrid feedback. |
|
Thanks @hakman /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @rifelpet @justinsb