Skip to content
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.

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

Self pod discovery does not respect pod terminating state #10681

Open
nefelim4ag opened this issue Nov 27, 2023 · 4 comments · May be fixed by #10686
Open

Self pod discovery does not respect pod terminating state #10681

nefelim4ag opened this issue Nov 27, 2023 · 4 comments · May be fixed by #10686
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@nefelim4ag
Copy link

nefelim4ag commented Nov 27, 2023

What happened:
With disabled publishService and used pod self-discovery of Node IP.
With more than one ingress-nginx pod in Deployment or DaemonSet.
While updating or if you manually delete the pod. While terminating pod will not be removed from ingress LoadBalancer status.

With current lifecycle hook /wait-shutdown controller will stop answer "ready" status and forwarded signal will stop Nginx.
So pod effectively will answer connection refused and NotReady.
If you try duct tape that behavior, with LifeCycle preStop hook like sleep 120, that will not work. Pod will be in Terminating & Ready state, so it still be in Ingress LoadBalancer status.

What you expected to happen:
Pod statuses are correctly handled.

Issue is here.
Controller only checks pod conditions, but Terminating state is special.
Correctly it must be handled like:

if v.GetDeletionTimestamp() == nil {
  // Pod not in terminating state
}

external-dns patch

How to reproduce this issue:
Deploy ingress-nginx helm:

helm repo add ingress-nginx https://kubernetes.github.io/ingress-nginx
helm template --create-namespace -n test ingress-nginx ingress-nginx/ingress-nginx \
  --set controller.publishService.enabled=false \
  --set controller.kind=Deployment \
  --set "controller.lifecycle.preStop.exec.command={sh,-c,sleep 120; kill 1}"

Remove any pod or trigger kubectl -n ingress-nginx rollout restart deployment ingress-nginx-controller

Example ingress:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: test
spec:
  ingressClassName: nginx
  rules:
    - host: example.com
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: ingress-nginx-controller-metrics
                port:
                  name: metrics
@nefelim4ag nefelim4ag added the kind/bug Categorizes issue or PR as related to a bug. label Nov 27, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 27, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@longwuyuan
Copy link
Contributor

could you write a step by step guide for someone to copy/paste on a minikube or kind cluster. You have left too much info out of the description so readers have to make assumptions and that may not be accurate as per your use case.

/remove-kind bug

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/bug Categorizes issue or PR as related to a bug. labels Nov 30, 2023
@longwuyuan
Copy link
Contributor

/triage needs-information

@k8s-ci-robot k8s-ci-robot added the triage/needs-information Indicates an issue needs more information in order to work on it. label Nov 30, 2023
@nefelim4ag
Copy link
Author

@longwuyuan updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
Development

Successfully merging a pull request may close this issue.

3 participants