kubectl cordon causes downtime of ingress(nginx)

[ChikkannaSuhas]

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug

/kind feature

What happened:
In order to migrate to a new version of kubernetes on GKE. I cordoned(Kubectl cordon ) all nodes in the old node pool(1.8.9) in order to drain the node pool(pods on the node) with an intention to cause the pods to run on a new version of the node pool(1.10.2). As soon as I cordoned all the nodes in the old node pool, it caused the downtime of ingress(nginx). The nginx controller pods where running fine on the cordoned node pool. But the cordon caused the nodes to be removed from the target pool of ingress load balancer. Below are the documents that suggest the above same method that I followed as best practice for zero downtime upgrade.

1). https://cloud.google.com/kubernetes-engine/docs/tutorials/migrating-node-pool
2). https://cloudplatform.googleblog.com/2018/06/Kubernetes-best-practices-upgrading-your-clusters-with-zero-downtime.html

What you expected to happen:

I expected "kubectl cordon " not to have the node/s removed from the target pool of the loadbalancer because the nginx controller pods where running absolutely fine on these nodes.

How to reproduce it (as minimally and precisely as possible):
1).Cordon all the nodes in an old node pool that runs nginx controller.

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version): 1.10.2
• Cloud provider or hardware configuration: GKE
• OS (e.g. from /etc/os-release): Container OS
• Kernel (e.g. uname -a): 4.14.22+
• Install tools:
• Others: Cordon causes to remove the nodes from the target pool of the ingress load balancer. The health checks fail on the nodes of the target pool, which should not be the case. Because the nginx controller pods run fine on these nodes.

[jhorwit2]

Technically, this isn't a Kubernetes bug as this is working as intended; however, it does beg the question why we remove unschedulable nodes if they're marked as ready since we perform the following checks today to determine LB backend nodes.

• is it scheduable?
• is it not a master?
• is it not marked for exclusion?
• is it marked as ready?

If any of those are a no, then it'll be removed from the pool on the next sync.

Perhaps someone from @kubernetes/sig-network-bugs can shed some light.

/sig network

[jhorwit2]

cc @MrHohn

This problem extends beyond GCP/ingress controller and to anyone using pools to upgrade nodes. A typical workflow I see is you'd cordon a node pool so as you drain each node the pods don't go to another node in the pool that'll shortly be drained as well. The current logic does not support that and complicates cluster administration because cordoning does not respect PDB's.

[jhorwit2]

/remove-sig apps
/remove-sig gcp

(since the service controller falls under sig networking)

[k8s-ci-robot]

@jhorwit2: Those labels are not set on the issue: sig/

In response to this:

/remove-sig apps
/remove-sig gcp

(since the service controller falls under sig networking)

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.

[MrHohn]

Ref #44997.

I thought a general practice for draining the old pool is to move over one node at a time in a rolling fashion. Shouldn't there always be certain backend pods being able to receive & serve traffic?

cc @nicksardo @thockin

[jhorwit2]

@MrHohn WRT draining, it is recommended to do it one node at a time here; however, in real world scenarios doing it one node at a time can cause a lot of avoidable container churn if you don't cordon off the nodes you at least plan to drain so that pods aren't being scheduled on a node you'll be draining after the next one.

According to the comments in the service controller it seems the unschedulable part exists because it was how we differentiated masters in the past; however, we have labels for that now. We also have the service exclusion label (can't find when/if that'll be promoted though as i don't think there's a ticket on it).

[pkdetlefsen]

I'm having problems with this issue as well. The fact that nodes are removed from target pools when cordoned means that I sometimes get brief interruptions of the traffic to a service when I cordon a node.

I'm guessing the node isn't gracefully removed from the load balancer which results in some traffic being cut off trying to route through the recently cordoned node.

The same problem occasionally happens when deleting nodes / node pools or when the cluster autoscaler deletes a node.

We are running GKE ver. 1.10.2-gke.1. Our best defence against these issue have so far been to limit node changes as much as possible and not use any auto scaling.

[erkez]

I've posted a question regarding this issue in SO some weeks before it was created.

https://stackoverflow.com/questions/50488507/why-does-http-load-balancer-forwarding-rules-exclude-cordoned-gke-nodes/50515954

The suggested action was to use node taints (NoSchedule) instead of cordon, that way nodes will still be marked as Ready.

[ChikkannaSuhas]

@erkez Thank you for the link, it Certainly helped. Perhaps, if thats the case(as mentioned in the accepted answer of the above link), then to cordon all the nodes of old node pools should not be a recommended action, at least in the GKE upgrade docs.

[joekohlsdorf]

kubectl help cordon only says: Mark node as unschedulable.
And I can't find what more it does in the documentation. So I would expect that it has exactly the same behaviour as a NoSchedule taint.

Why is removing running pods from the balancer working as intended?
If it isn't a bug then the documentation is certainly unclear in this regard.

[dwradcliffe]

This just caused a major outage for us too.

I agree with the previous posts, while this may be working as the code intended, it does not follow the patterns and documentation k8s provides. When I cordon a node it should not impact the traffic going to the node or any pods on it. I should be able to gradually remove traffic by draining nodes/pods and respecting PDBs.

[jhorwit2]

@thockin @smarterclayton this issue touches on the fact we are removing masters as well and inadvertently removing cordoned nodes. The logic in the service controller mentions we only filter out unschedulable nodes since that's how masters were setup.

[Chili-Man]

We wanted to replace all of the kubernetes cluster nodes to different node instance types, so we started off by cordoning all of the nodes, but it surprisingly caused an outage for us.

As @joekohlsdorf pointed out, the only documentation around cordoning is Mark node as unschedulable, which I took to mean that new pods can not be scheduled on those nodes. It was surprising behavior to find out that, not only does it not allow new pods to be scheduled on those nodes, but that it also causes the service controller to remove unschedulable nodes from load balancing pools. I understand that it's working as intended, but if that was also documented as part of the cordon operation, I would have been able to avoid an outage.

[thockin]

I finally caught up to this bug. Fun one.

I agree that this is working-as-intended and that it stinks. We can do better.

As a short-term workaround, you can taint the nodes before cordoning one-by-one. That will stop new work from arriving, but will leave old work there. If we agree on an appropriately named taint, we can have nginx tolerate it by default. Am I missing any reason why this would not work? @bsalamat @mml

If that flies, we should fix docs (@chenopis those are GCP docs). I am happy to help with that.

Longer term maybe we can do something smarter. I am not sure what that would be, just now. Ideas?

[dwradcliffe]

Is there any reason we can't just leave the nodes in the load balancer pool when the node is cordoned? And when the pods are terminated the nodes can be removed?

I may be missing some context here?

[thockin]

The presumption is that a node that is unschedulable is going away, or needs some other repair. Using it as a target for load-balancers means there could be sessions routed THROUGH that node. Even as the pods are drained. Right up until the node suddenly dies, causing an outage of a different flavor. What should really happen is that a node which becomes unschedulable starts draining connections. Once all the pods are moved off, the node can be removed from the pool entirely. I don't think any cloud providers, even GCP, handle this just right today.

…

On Mon, Sep 17, 2018 at 5:20 PM David Radcliffe ***@***.***> wrote: Is there any reason we can't just leave the nodes in the load balancer pool when the node is cordoned? And when the pods are terminated the nodes can be removed? I may be missing some context here? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#65013 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVHz-p_M7-hezukBrJDyJ8Ynu8XbBks5ucDxAgaJpZM4UkM4m> .

[thockin]

Wait, if you brought up a new nodepool, all those new nodes should be in the TargetPool. Cordoning the old nodepool should only terminate the existing connections, new connections should be fine. Something doesn't add up...

[thockin]

Was this using ExternalTrafficPolicy: Local

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[thockin]

/lifecycle frozen
/remove-lifecycle stale
/remove-lifecycle rotten

[olvesh]

@daenney - As I mentioned in #65013 (comment) I set up a script that first taints all nodes in the old node pool and then cordon+taint one by one node. If your ingress services are redundant this should work well.

[blakebarnett]

Is there a plan for cordon behavior to improve on GCP? We have relied on this behavior for years on our AWS clusters and it was very surprising to find out.

[smarterclayton]

This is a bug with service load balancers - not something we should document. We defined a label in https://github.com/kubernetes/enhancements/blob/master/keps/sig-architecture/2019-07-16-node-role-label-use.md#service-load-balancer that cluster admins can use - Unschedulable has nothing to do with SLB inclusion.

node.kubernetes.io/exclude-from-external-load-balancers which is beta and on by default in 1.19.

[smarterclayton]

The presumption is that a node that is unschedulable is going away, or needs some other repair.

Disagree. Unschedulable means "no new workloads". It has nothing to do with existing workloads, that is why we have drain (but we should have something in kube that communicates nodes are draining).

I have opened #90823 to remove this check - we have an alternate approach now and the old logic is wrong (the comment in the code even says "this is for masters!" and that's wrong now too as part of the kep).

[danehans]

It has nothing to do with existing workloads, that is why we have drain (but we should have something in kube that communicates nodes are draining).

A node status condition?

[smarterclayton]

Possibly, or a taint, or a label. Something concrete.

[droslean]

/milestone v1.19

[AlphaWong]

@dictcp