Skip to content

Commit

Permalink
refactor: wait-shutdown preStop hook is not necessary
Browse files Browse the repository at this point in the history
/wait-shutdown preStop script's only job is to send SIGTERM to nginx-ingress-controller,
which is PID 1, so it's the same with or without in Kubernetes environments.

See #6287 for discussion.
  • Loading branch information
motoki317 committed Nov 28, 2024
1 parent 3fe18f1 commit ff45a2a
Show file tree
Hide file tree
Showing 7 changed files with 4 additions and 72 deletions.
10 changes: 0 additions & 10 deletions build/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,3 @@ ${GO_BUILD_CMD} \
-X ${PKG}/version.REPO=${REPO_INFO}" \
-buildvcs=false \
-o "${TARGETS_DIR}/dbg" "${PKG}/cmd/dbg"

echo "Building ${PKG}/cmd/waitshutdown"

${GO_BUILD_CMD} \
-trimpath -ldflags="-buildid= -w -s \
-X ${PKG}/version.RELEASE=${TAG} \
-X ${PKG}/version.COMMIT=${COMMIT_SHA} \
-X ${PKG}/version.REPO=${REPO_INFO}" \
-buildvcs=false \
-o "${TARGETS_DIR}/wait-shutdown" "${PKG}/cmd/waitshutdown"
2 changes: 1 addition & 1 deletion charts/ingress-nginx/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ metadata:
| controller.keda.triggers | list | `[]` | |
| controller.kind | string | `"Deployment"` | Use a `DaemonSet` or `Deployment` |
| controller.labels | object | `{}` | Labels to be added to the controller Deployment or DaemonSet and other resources that do not have option to specify labels # |
| controller.lifecycle | object | `{"preStop":{"exec":{"command":["/wait-shutdown"]}}}` | Improve connection draining when ingress controller pod is deleted using a lifecycle hook: With this new hook, we increased the default terminationGracePeriodSeconds from 30 seconds to 300, allowing the draining of connections up to five minutes. If the active connections end before that, the pod will terminate gracefully at that time. To effectively take advantage of this feature, the Configmap feature worker-shutdown-timeout new value is 240s instead of 10s. # |
| controller.lifecycle | object | `{}` | |
| controller.livenessProbe.failureThreshold | int | `5` | |
| controller.livenessProbe.httpGet.path | string | `"/healthz"` | |
| controller.livenessProbe.httpGet.port | int | `10254` | |
Expand Down
13 changes: 1 addition & 12 deletions charts/ingress-nginx/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -945,18 +945,7 @@ controller:
# annotations:
# description: Too many 4XXs
# summary: More than 5% of all requests returned 4XX, this requires your attention
# -- Improve connection draining when ingress controller pod is deleted using a lifecycle hook:
# With this new hook, we increased the default terminationGracePeriodSeconds from 30 seconds
# to 300, allowing the draining of connections up to five minutes.
# If the active connections end before that, the pod will terminate gracefully at that time.
# To effectively take advantage of this feature, the Configmap feature
# worker-shutdown-timeout new value is 240s instead of 10s.
##
lifecycle:
preStop:
exec:
command:
- /wait-shutdown
lifecycle: {}
priorityClassName: ""
# -- Rollback limit
##
Expand Down
43 changes: 0 additions & 43 deletions cmd/waitshutdown/main.go

This file was deleted.

1 change: 0 additions & 1 deletion rootfs/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ COPY --chown=www-data:www-data etc /etc

COPY --chown=www-data:www-data bin/${TARGETARCH}/dbg /
COPY --chown=www-data:www-data bin/${TARGETARCH}/nginx-ingress-controller /
COPY --chown=www-data:www-data bin/${TARGETARCH}/wait-shutdown /

# Fix permission during the build to avoid issues at runtime
# with volumes (custom templates)
Expand Down
1 change: 0 additions & 1 deletion rootfs/Dockerfile-chroot
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ COPY --chown=www-data:www-data etc /chroot/etc

COPY --chown=www-data:www-data bin/${TARGETARCH}/dbg /
COPY --chown=www-data:www-data bin/${TARGETARCH}/nginx-ingress-controller /
COPY --chown=www-data:www-data bin/${TARGETARCH}/wait-shutdown /
COPY --chown=www-data:www-data nginx-chroot-wrapper.sh /usr/bin/nginx

WORKDIR /chroot/etc/nginx
Expand Down
6 changes: 2 additions & 4 deletions test/e2e/gracefulshutdown/grace_period.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,14 @@ var _ = framework.IngressNginxDescribe("[Shutdown] Grace period shutdown", func(
if strings.Contains(v, "--shutdown-grace-period") {
continue
}

args = append(args, v)
}

args = append(args, "--shutdown-grace-period=90")
deployment.Spec.Template.Spec.Containers[0].Args = args
cmds := []string{"/wait-shutdown"}
deployment.Spec.Template.Spec.Containers[0].Lifecycle.PreStop.Exec.Command = cmds

grace := int64(3600)
deployment.Spec.Template.Spec.TerminationGracePeriodSeconds = &grace

_, err := f.KubeClientSet.AppsV1().Deployments(f.Namespace).Update(context.TODO(), deployment, metav1.UpdateOptions{})
return err
})
Expand Down

0 comments on commit ff45a2a

Please sign in to comment.