Ingress Status: Improve checking for rollover or shutdown.

[nefelim4ag]

What this PR does / why we need it:

Filter out pods in terminating state from pod discovery. Reuse common labels filter for pod discovery between ReplicaSets.

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)

Which issue/s this PR fixes

fixes #10680 - incorrect handling of pod rollout without publishService fixes #10681 - missing handling of pods in terminating state

How Has This Been Tested?

Build image, replace current 1.9.4 image with patched. Make kubectl -n ingress-nginx rollout restart deployment ingress-nginx-controller for RS handling tests. Patch LifeCycle.preStop to:

lifecycle:
  preStop:
    exec:
      command:
      - sh
      - -ec
      - sleep 180; kill 1

And monitoring ingress status for correct changes:

• Pods from new ReplicaSets are shown in Ingress Status
• Pods in terminating state are missing from Ingress Status

Checklist:

• [x] I've read the CONTRIBUTION guide
• [x] I have added unit and/or e2e tests to cover my changes.
• [x] All new and existing tests passed.

Current status_test.go are a little fragile and I currently struggle with just adding pods with new statuses and adding tests. But I'm working on it, any help will be appreciated.

Thanks!

[netlify[bot]]

You don't have to do anything. This feature should be added to simplewall. So that it automatically blocks the program by file name, and not by path + name. Or automatically blocked the path + file name if the file name is in the list of blocked ones. Now there is no such possibility. Although some programs constantly change the path to the file by updating. \version_number\application_name. Or even worse \path\version_number.exe At least in 1 case it should be possible to block them. Case 2 is generally difficult, it is necessary to block all applications that are along the path... simplewall does not give such an opportunity for some reason.

ah ok i understand now. Unfortunately, such a feature is not implemented into SimpleWall. Thanks for the explanation.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nefelim4ag Once this PR has been reviewed and has the lgtm label, please assign puerco for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

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.

[k8s-ci-robot]

Welcome @nefelim4ag!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @nefelim4ag. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[Gacko]

Looks great! Unfortunately I'm not eligible for approving controller PRs, yet. But if you really wanna go the extra mile, you could have a look at this issue and check if your PR can (at least partly) improve what's not working in there. I know, your change is focus on not having a published service, but maybe you can at least have a look. :)

[nefelim4ag]

@Gacko You are right, this is the same problem here.

But I'm not sure they are directly related to each other because helm upgrade path and drop of LB status must be handled by isRunningMultiplePods(). I need to spend some more time on get LB working and reproducing that issue with enabled debug.

UPD: It is similar, someone adds helm version labels to the pod:

app.kubernetes.io/version: 1.8.4
helm.sh/chart: ingress-nginx-4.7.3

They are creating problems. As quick fix, it is possible to make them "common" and add them to the filter.

But right solution will be to make some more magic & rewrite that discovery logic completely. Maybe always use publishService, or follow by ownerReferences in pod data and use selector from deployment/daemonset. I think that selector usage will not break anything to users, at least behavior will look similar.

Anyway, only maintainers have the right to choose a solution, and this is out of the scope of this PR (IMHO). I can only append rules to the current filter.

[Gacko]

/assign

[nefelim4ag]

Added tests, and now helm labels will be ignored, which can work as duct tape for:

• https://github.com/kubernetes/ingress-nginx/issues/10475

I can squash/rebase on request, np.

[Gacko]

/retitle Ingress Status: Improve checking for rollover or shutdown.

[nefelim4ag]

Hello, how can I assist somehow with this PR?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nefelim4ag Once this PR has been reviewed and has the lgtm label, please ask for approval from gacko. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[longwuyuan]

• Is it feasible to do real tests on minikube and/or kind clusters to first show the problem and then to deploy controller with changed code to show the solution (as well as lack of impact on any other part of the controller from the change)

• Squash commits

[nefelim4ag]

@longwuyuan I did not quite understand how I could provide that to someone (testing cluster), I used the patched version this whole time because it fixed my issues on my dev/prod clusters. But my words are worth nothing, there are unit tests for that.

I described the problems (we talk about the usage of ingress nginx without LB, where this code applies):

• #10475. Helm upgrade is 1.8.4 -> 1.9.1, or 1.9.1 -> 1.10.1 is a simple reproduction to check how the controller will not detect new nodes with new labels and not set them to ingress status.
• Missing check for pod 'terminating' status.

There are images: https://gallery.ecr.aws/xflow-ltd/ingress-nginx, so 1.9.4-p2, 1.10.1-p1 Just in case someone needed them.

---

Yes, squash sounds good after that amount of time.