Skip to content

WIP [OCPBUGS-82081] Add preStop lifecycle hook to DNS pods to reduce upgrade disruption #475

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.

Sign up for GitHub

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

Draft
davidesalerno wants to merge 1 commit into
base: master
Choose a base branch
from
+33 −0
Draft

WIP [OCPBUGS-82081] Add preStop lifecycle hook to DNS pods to reduce upgrade disruption #475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/manifests/assets/dns/daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ spec:
memory: 70Mi
securityContext:
readOnlyRootFilesystem: true
lifecycle:
preStop:
exec:
command: ["sleep", "5"]

@coderabbitai coderabbitai Bot May 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Does the OpenShift CoreDNS image used by cluster-dns-operator include a sleep executable in PATH for Kubernetes preStop exec hooks?

💡 Result:

No—the OpenShift CoreDNS image used by cluster-dns-operator does not include a general-purpose sleep executable in PATH for Kubernetes preStop exec hooks. Evidence: 1) The OpenShift-specific CoreDNS image Dockerfile only copies the CoreDNS binary into /usr/bin/ and does not add sleep (or any other shell tooling) into the image, so there is no basis to assume /bin/sleep or /usr/bin/sleep exists and is on PATH. The Dockerfile shows only a COPY of coredns to /usr/bin/ and an ENTRYPOINT of /usr/bin/coredns. [1] 2) The OpenShift DNS operator’s “graceful shutdown” behavior is implemented via the CoreDNS health plugin being configured to sleep during shutdown, not via a Pod lifecycle preStop exec that would require an in-image sleep binary. The operator commit explicitly says it configures “CoreDNS’s health plugin to sleep … when CoreDNS is shut down.” [2] Practical implication: - If you try to configure a Kubernetes preStop: exec: ['sleep', 'N'] for the OpenShift CoreDNS container, it is likely to fail because sleep is not present in the image by design. (This aligns with the general CoreDNS ecosystem issue where preStop can fail when the image lacks sleep.) [3] If you want, tell me your OpenShift version (e.g., 4.14/4.15/4.16/4.17) and I can point you to the exact image tag/digest that your cluster-dns-operator uses and whether it matches the Dockerfile behavior above.

Citations:

The sleep command is not available in the OpenShift CoreDNS image and will cause the preStop hook to fail.

The OpenShift CoreDNS container image is a minimal image containing only the CoreDNS binary, with no shell utilities like sleep. Using command: ["sleep", "5"] in the preStop.exec hook will fail at runtime, and the intended drain delay will not occur. If graceful shutdown is required, use the CoreDNS health plugin configuration (which is the operator's standard approach) rather than a Pod lifecycle hook that depends on missing binaries.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/manifests/assets/dns/daemonset.yaml` at line 62, The preStop lifecycle
hook currently uses command ["sleep","5"] (preStop.exec) which is not present in
the minimal CoreDNS image and will always fail; remove or disable the
preStop.exec block that calls sleep in the DaemonSet and instead implement
graceful shutdown via the CoreDNS health plugin/operator configuration (i.e.,
update the Corefile/operator settings rather than relying on a shell utility in
the container).

- name: kube-rbac-proxy
# image and args are set at runtime by the operator based on the
# centralized TLS security profile from apiservers.config.openshift.io/cluster
Expand All @@ -75,6 +79,7 @@ spec:
name: tmp-dir
securityContext:
readOnlyRootFilesystem: true
terminationGracePeriodSeconds: 40
dnsPolicy: Default
# nodeSelector is set at runtime.
volumes:
Expand Down
5 changes: 5 additions & 0 deletions pkg/operator/controller/controller_dns_daemonset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,11 @@ func daemonsetConfigChanged(current, expected *appsv1.DaemonSet) (bool, *appsv1.
changed = true
break
}
if !cmp.Equal(a.Lifecycle, b.Lifecycle, cmpopts.EquateEmpty()) {
updated.Spec.Template.Spec.Containers = expected.Spec.Template.Spec.Containers
changed = true
break
}
}
}

Expand Down
23 changes: 23 additions & 0 deletions pkg/operator/controller/controller_dns_daemonset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ func TestDesiredDNSDaemonset(t *testing.T) {
if e, a := coreDNSImage, c.Image; e != a {
t.Errorf("expected daemonset dns image %q, got %q", e, a)
}
if c.Lifecycle == nil || c.Lifecycle.PreStop == nil {
t.Error("expected dns container to have a preStop lifecycle hook")
} else if !reflect.DeepEqual(c.Lifecycle.PreStop.Exec.Command, []string{"sleep", "5"}) {
t.Errorf("unexpected preStop command: %v", c.Lifecycle.PreStop.Exec.Command)
}
Comment on lines +81 to +85

@coderabbitai coderabbitai Bot May 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard against nil PreStop.Exec before reading Command.

Current assertion can panic if PreStop exists but Exec is nil; make it a clean test failure instead.

Suggested fix 
-				if c.Lifecycle == nil || c.Lifecycle.PreStop == nil {
+				if c.Lifecycle == nil || c.Lifecycle.PreStop == nil || c.Lifecycle.PreStop.Exec == nil {
 					t.Error("expected dns container to have a preStop lifecycle hook")
 				} else if !reflect.DeepEqual(c.Lifecycle.PreStop.Exec.Command, []string{"sleep", "5"}) {
 					t.Errorf("unexpected preStop command: %v", c.Lifecycle.PreStop.Exec.Command)
 				}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if c.Lifecycle == nil || c.Lifecycle.PreStop == nil {
t.Error("expected dns container to have a preStop lifecycle hook")
} else if !reflect.DeepEqual(c.Lifecycle.PreStop.Exec.Command, []string{"sleep", "5"}) {
t.Errorf("unexpected preStop command: %v", c.Lifecycle.PreStop.Exec.Command)
}
if c.Lifecycle == nil || c.Lifecycle.PreStop == nil || c.Lifecycle.PreStop.Exec == nil {
t.Error("expected dns container to have a preStop lifecycle hook")
} else if !reflect.DeepEqual(c.Lifecycle.PreStop.Exec.Command, []string{"sleep", "5"}) {
t.Errorf("unexpected preStop command: %v", c.Lifecycle.PreStop.Exec.Command)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/operator/controller/controller_dns_daemonset_test.go` around lines 81 -
85, The test currently reads c.Lifecycle.PreStop.Exec.Command without guarding
Exec and can panic; update the assertions to explicitly check that c.Lifecycle
and c.Lifecycle.PreStop are non-nil and then that c.Lifecycle.PreStop.Exec is
non-nil before accessing Command (e.g., replace the else-if using
reflect.DeepEqual with a nested check that calls t.Error/t.Errorf if Exec is
nil, and only then compares Exec.Command to []string{"sleep","5"}), referencing
the test's container variable c and its Lifecycle.PreStop.Exec.Command for
locating the change.

case "kube-rbac-proxy":
if e, a := kubeRBACProxyImage, c.Image; e != a {
t.Errorf("expected daemonset kube rbac proxy image %q, got %q", e, a)
Expand All @@ -86,6 +91,11 @@ func TestDesiredDNSDaemonset(t *testing.T) {
t.Errorf("unexpected daemonset container %q", c.Name)
}
}
if ds.Spec.Template.Spec.TerminationGracePeriodSeconds == nil {
t.Error("expected terminationGracePeriodSeconds to be set")
} else if *ds.Spec.Template.Spec.TerminationGracePeriodSeconds != 40 {
t.Errorf("expected terminationGracePeriodSeconds=40, got %d", *ds.Spec.Template.Spec.TerminationGracePeriodSeconds)
}
}
}

Expand Down Expand Up @@ -426,6 +436,19 @@ func TestDaemonsetConfigChanged(t *testing.T) {
},
expect: true,
},
{
description: "if a container lifecycle is added",
mutate: func(daemonset *appsv1.DaemonSet) {
daemonset.Spec.Template.Spec.Containers[0].Lifecycle = &corev1.Lifecycle{
PreStop: &corev1.LifecycleHandler{
Exec: &corev1.ExecAction{
Command: []string{"sleep", "5"},
},
},
}
},
expect: true,
},
{
description: "if an unexpected additional container is added",
mutate: func(daemonset *appsv1.DaemonSet) {
Expand Down