fix: change etcd to use /livez and /readyz #7381
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,10 +67,10 @@ func installKarmadaEtcd(client clientset.Interface, name, namespace string, cfg | |
| } | ||
|
|
||
| etcdStatefulSetBytes, err := util.ParseTemplate(KarmadaEtcdStatefulSet, struct { | ||
| KarmadaInstanceName, StatefulSetName, Namespace, Image string | ||
| ImagePullPolicy, EtcdClientService, CertsSecretName string | ||
| InitialCluster, EtcdDataVolumeName, EtcdCipherSuites string | ||
| Replicas, EtcdListenClientPort, EtcdListenPeerPort, EtcdMetricsPort int32 | ||
| }{ | ||
|
Comment on lines
69
to
74
There was a problem hiding this comment. The anonymous struct used for template parsing is becoming quite large (13 fields). While the repository style guide suggests limiting function parameters to 5, the same principle of readability and maintainability applies to structs. Consider refactoring this into a named struct. Note that per repository rules, the struct name should be plural if it logically represents a collection of multiple items (e.g., WorkloadAffinityGroups). References
Member
There was a problem hiding this comment. It's okay if this PR isn't handled. |
||
| KarmadaInstanceName: name, | ||
| StatefulSetName: util.KarmadaEtcdName(name), | ||
|
|
@@ -85,6 +85,7 @@ func installKarmadaEtcd(client clientset.Interface, name, namespace string, cfg | |
| Replicas: *cfg.Replicas, | ||
| EtcdListenClientPort: constants.EtcdListenClientPort, | ||
| EtcdListenPeerPort: constants.EtcdListenPeerPort, | ||
| EtcdMetricsPort: constants.EtcdMetricsPort, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("error when parsing Etcd statefuelset template: %w", err) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,7 @@ spec: | |
| - --name=$(KARMADA_ETCD_NAME) | ||
| - --listen-client-urls=https://0.0.0.0:{{ .EtcdListenClientPort }} | ||
| - --listen-peer-urls=http://0.0.0.0:{{ .EtcdListenPeerPort }} | ||
| - --listen-metrics-urls=http://0.0.0.0:{{ .EtcdMetricsPort }} | ||
| - --advertise-client-urls=https://{{ .EtcdClientService }}.{{ .Namespace }}.svc.cluster.local:{{ .EtcdListenClientPort }} | ||
|
vgt-rangehrn marked this conversation as resolved.
Comment on lines
53
to
56
There was a problem hiding this comment. Binding etcd’s metrics/health listener to 0.0.0.0 over plain HTTP makes /metrics, /livez, and /readyz reachable from any pod that can reach the pod IP, which is a security posture change compared to the previous mTLS-only client port checks. If this endpoint only needs to be consumed by kubelet, consider mitigating exposure (e.g., a NetworkPolicy to restrict access in the namespace, or alternative probe mechanisms that don’t require an unauthenticated HTTP listener).
Author
There was a problem hiding this comment. I think this shouldn't be an issue, but I'm open to other opinions. |
||
| - --initial-cluster={{ .InitialCluster }} | ||
| - --initial-cluster-state=new | ||
|
|
@@ -70,23 +71,35 @@ spec: | |
| apiVersion: v1 | ||
| fieldPath: metadata.name | ||
| livenessProbe: | ||
| httpGet: | ||
| path: /livez | ||
| port: {{ .EtcdMetricsPort }} | ||
| scheme: HTTP | ||
|
vgt-rangehrn marked this conversation as resolved.
|
||
| initialDelaySeconds: 60 | ||
| timeoutSeconds: 5 | ||
| periodSeconds: 10 | ||
| successThreshold: 1 | ||
| failureThreshold: 3 | ||
|
Comment on lines
73
to
82
There was a problem hiding this comment. This changes the etcd liveness probe from a very long initial delay/period (600s/60s) to a much more aggressive cadence (initialDelaySeconds=15, failure after ~30s). That can cause restart loops on slower starts (e.g., disk recovery or large data dirs). Consider keeping the previous timing values, or adding a startupProbe to cover slow initialization while keeping liveness strict for steady state.
Author
There was a problem hiding this comment. In my testing, etcd took something like 12s to be ready. I increased the initialDelay to 60s, so the pod would only be restarted if it wasn't ready after 90s. I think that should be fine, but I'm open to input. |
||
| readinessProbe: | ||
| httpGet: | ||
| path: /readyz | ||
| port: {{ .EtcdMetricsPort }} | ||
| scheme: HTTP | ||
| initialDelaySeconds: 10 | ||
| timeoutSeconds: 5 | ||
| periodSeconds: 5 | ||
| successThreshold: 1 | ||
| failureThreshold: 30 | ||
| ports: | ||
| - containerPort: {{ .EtcdListenClientPort }} | ||
| name: client | ||
| protocol: TCP | ||
| - containerPort: {{ .EtcdListenPeerPort }} | ||
| name: server | ||
| protocol: TCP | ||
| - containerPort: {{ .EtcdMetricsPort }} | ||
| name: metrics | ||
| protocol: TCP | ||
| volumeMounts: | ||
| - mountPath: /var/lib/etcd | ||
| name: {{ .EtcdDataVolumeName }} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I observed that the value of initialDelaySeconds has been adjusted significantly. What is the reason for this adjustment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested locally, everything is working fine, not sure why we set it to 600s before.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with the configuration from here and increased the initialDelaySeconds based on the copilot comment (and my own testing). On the other hand, there would probably be no harm in leaving it at 600.