Ignore detached nodes when doing validate cluster

[rajatjindal]

Ran into this issue while doing rolling-update on our cluster today. The rolling updated was cancelled (for unrelated reasons) before it could complete.

[k8s-ci-robot]

Hi @rajatjindal. 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.

[hakman]

/ok-to-test

[rifelpet]

I'm curious why a detached node should be ignored from validation? the node and underlying instance should still be healthy so why not consider it for validation?

[rajatjindal]

those nodes had no pods running on them and we ended up deleting them manually from AWS console.

i do not understand detached nodes logic 100%, may be @johngmyers can shed some light here.

i think from AWS and kops perspective those nodes still existed, but from k8s perspective the nodes were gone?

[johngmyers]

If a node is detached then it is on its way out of the cluster. A lack of being in the cluster is not a reason to hold up further disruption to the cluster.

/lgtm
Not approving due to employer proximity issues.

[rajatjindal]

/retest

[johngmyers]

To add more color, since detached nodes don't contribute to satisfying the IG target size check, the joined-to-cluster check is not not necessary to ensure the IG has enough capacity.

[johngmyers]

Thinking about this further, we'll probably want to also exempt detached nodes from the node readiness and system-node-critical unready pod checks, at least for purposes of rolling update.

[rajatjindal]

Thinking about this further, we'll probably want to also exempt detached nodes from the node readiness and system-node-critical unready pod checks, at least for purposes of rolling update.

can that be a separate PR or should I make that change as part of this PR?

[johngmyers]

It could be separate, but the intent here is "don't let extra sick nodes hold up a rolling update" and there is value in getting all of the "sick node" validation failures taken care of at once.

I have been thinking of a tighter integration between cluster validation and rolling update. There was previous work on ignoring "sick node" validation failures for nodes outside of the IG being updated. The next step would be to count sick nodes in the same IG against maxUnavailable, allowing maxUnavailable - sick drains to proceed in parallel.

[rajatjindal]

Hi @rifelpet do you think its reasonable to move ahead with the PR. if so, could you please help with the next steps?

thanks
Rajat Jindal

[rifelpet]

yes, I agree with the proposed logic now.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rifelpet]

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

[hakman]

/retest

[rajatjindal]

Thank you very much