Repository navigation
CKS: fix node drain on scale-down and upgrade, make a failed upgrade resumable - #14611
Open
bhouse-nexthop wants to merge 2 commits into
Open
bhouse-nexthop wants to merge 2 commits into
bhouse-nexthop wants to merge 2 commits into
Conversation
…resumable Scale-down and upgrade both drain a node before touching it, each with its own copy of the drain loop, and neither works well: - Scale-down gives up at once when kubectl drain exits non-zero (a PodDisruptionBudget that does not allow an eviction yet, say), and when it does retry after an exception it counts the attempt twice. The node is left cordoned, and the cluster goes to Alert although nothing has been removed and every node still works. - Upgrade retries straight away, with no wait for the eviction to become possible, and on the last failure leaves the node cordoned. - In both, kubectl drain has no --timeout, so it is cut off by a fixed 60s SSH timeout rather than a deadline of its own. Both workers now use one drain helper in KubernetesClusterActionWorker: one count per attempt, a retry (after a wait) on a non-zero exit or an exception, kubectl drain --timeout from the new setting cloud.kubernetes.cluster.node.drain.timeout (300s), cut down so that no attempt runs past the operation's deadline, with the SSH wait derived from it, and the end of the drain output in the error. The caller uncordons the node, and only if it cordoned it. Scale-down cordons every node it is going to remove (leaving alone the ones that were cordoned already), drains them, and if one cannot be drained uncordons them all and puts the cluster back in the state it was in, with an error that names the node, instead of Alert; the same happens if the scale times out before every node is drained. Once they are drained, the removal is not stopped part way by the timeout any more. If removing a node fails, the nodes still in the cluster are uncordoned. The upgrade had two more problems: - It only waited for the first control node to be Ready, so the next node could be drained while the previous one was still coming back. It now waits for every node, after it has been uncordoned. - It reached node i on sshPort + i. The port-forwarding rules are not created in the order of the upgrade's VM list (manually upgraded nodes are left out of that list, and nodes added later get the next free port), so the script could run on the wrong node. It now looks up the node's own SSH port-forwarding rule, as the remove worker already does. A failed upgrade left the cluster in Alert with no way to finish it, as upgradeKubernetesCluster only accepts a Running cluster. An upgrade now records the version it is upgrading to as a cluster detail (an existing key/value table) and removes it once the cluster is Running or Stopped again. upgradeKubernetesCluster accepts a cluster in Alert (Alert -> Upgrading) when that detail is the requested version (or another one with the same semantic version), which is a real upgrade of the cluster's own version, and all of the cluster's VMs are running, and refuses any other Alert cluster with a clear message. For an upgrade that failed before this change, a cluster still at the old version with nodes recorded at the target version (kubernetesnodeversion, which is set as each node is done) is accepted too. Nodes that are already at the target version, both as recorded and as reported by kubectl get node, and that are Ready, are skipped. The first control node is still upgraded first; if it is already done, kubeadm upgrade apply is not run again. The node version check now compares versions exactly, so 1.28.1 no longer matches 1.28.10, line by line, as the SSH output can hold stderr too, and ignoring semver build metadata. Failure messages name the node the upgrade stopped on. The upgrade stops if it can't move the cluster to Upgrading, and the Alert state scanner no longer marks a cluster Running when it couldn't move it to Recovering, and a failed upgrade detaches its ISO before going to Alert, so a retry and the scanner or the failing job don't undo each other's work. No schema change. Signed-off-by: Brad House <bhouse@nexthop.ai>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14611 +/- ##
============================================
+ Coverage 18.32% 18.36% +0.04%
- Complexity 16644 16716 +72
============================================
Files 5939 5939
Lines 537768 538056 +288
Branches 65929 65979 +50
============================================
+ Hits 98551 98831 +280
+ Misses 428008 427985 -23
- Partials 11209 11240 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Draft
2 of 9 tasks
Collaborator
Author
Test report: CKS node drain on scale-down and upgrade, and resuming a failed upgradeEnvironment
Results
S1: drain blocked by a PodDisruptionBudget
S2: the same scale-down, with the PDB relaxed
S3: per-node SSH address during the upgrade
S4: a failed upgrade, then a retry from Alert (cluster C)
Result: pass.
Not covered
|
This was referenced Nov 9, 2026
When a scale-down removes a worker, it deletes the node from Kubernetes, destroys and expunges its VM, and only then deletes the VM's entry in kubernetes_cluster_vm_map. If the destroy or the expunge failed, the exception skipped that last step and the entry stayed, for a VM that was Destroyed, or still running but no longer a Kubernetes node. Nothing recovered from that: the state scanner moves an Alert cluster back to Running only when every mapped VM is Running and Kubernetes reports as many ready nodes as the cluster has, which a VM whose node is gone never satisfies, and a cluster in Alert can't be scaled. The node count and the next scale-down's choice of nodes were worked out from the stale entry too. The node leaving Kubernetes is now the point at which its VM leaves the cluster: - Once the node has been deleted from Kubernetes, the map entry is deleted after the attempt to destroy and expunge the VM, whatever its outcome. (Not before it: the VM's expunge looks the entry up, to leave alone the port-forwarding rules of a CKS node in an NSX zone.) - If the VM can't be expunged, the scale-down goes on, with a warning in the log and a WARN event that names the VM and the state it was left in. The expunge task retries it (it picks up Destroyed, Expunging and Error VMs), or an administrator can. It no longer holds the cluster in Alert. Until it is expunged it keeps the cluster's network in use, so deleting the cluster with its network is refused, with an error that names the VM, as for any VM on the network that is not in the cluster. Nothing is deleted in that case, and the delete can be retried once the VM is gone. - If the VM can't be destroyed, the scale-down fails, as it did, with an error that names the VM and says it is no longer part of the cluster and has to be destroyed by hand. (If the VM turns out to be Destroyed or Expunging anyway, it is expunged as usual and the scale-down goes on. A VM left in Error is not taken as destroyed, as it may still be running on its host.) Its entry is still dropped: keeping it would leave the cluster stuck in Alert as above, and a retry would try to cordon a node that Kubernetes no longer has. Without the entry the scanner can bring the cluster back to Running and the scale-down can be retried; the VM left behind is an ordinary VM of the account. - A VM left behind either way has the cluster's SSH port-forwarding rule on it revoked, as expunging it would have done. The cluster's rule updates may not: none runs if the scale-down stops, and a VPC tier's only clears the remaining nodes' ports, while the rule, on a port beyond theirs, would conflict with the next node added. (A rule provided by NSX is left alone, as expunge leaves a CKS node's rules alone in an NSX zone, and so is the rule the scale-down reaches the cluster through, which the remaining nodes still need.) - If the scale-down stops part way, for any reason, including a failure to update the network rules once every node is gone, the cluster's node count, cores and memory are reduced by the VMs whose entries were deleted, so that they match the map again (the scanner and the next scale compare the two). As in a full scale-down, which still sets them once at the end, every VM removed comes off the node count, including a control node removed by ID. Signed-off-by: Brad House <bhouse@nexthop.ai>
Collaborator
Author
|
Added a commit, 1370152 ("CKS: drop a removed node's VM map entry even if its VM can't be expunged"), for a related scale-down bug in the node-removal step this PR already touches. When a node had been deleted from Kubernetes but its VM then failed to destroy or expunge, the exception skipped the deletion of the VM's Now the entry is deleted once the node is out of Kubernetes, whatever happens to the VM:
The commit message has the details, and there are unit tests for the happy path, expunge failure, destroy failure and partial failure. As before, I have only run unit tests, not tested against a live cluster. |
Collaborator
Author
Test report: scale-down when the removed node's VM can't be expunged or destroyed (commit 1370152)Summary: a normal scale-down still drains, removes and expunges the node. When the removed node's VM can't be expunged, the scale-down now completes: it records a WARN event naming the VM, drops the VM's map entry, and fixes the node count, cores and memory. The cluster goes back to Running, not Alert. Once the blocker is gone, the expunge task removes the VM. When the VM can't even be destroyed, the scale-down fails with an error that names the VM. Its map entry is still dropped, so the cluster stays Running and a retry works. Environment
Results
How the failures were caused
R: normal scale-down
E: expunge fails
E2: blocker removed
D: destroy fails, then a retry
Result: pass. A removed node's VM that can't be expunged no longer leaves a stale map entry or holds the cluster in Alert. The scale-down completes with a WARN event, and the expunge task cleans the VM up once it can. A VM that can't be destroyed fails the scale with a clear error, and the cluster can still be scaled. Not covered
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes how a CKS cluster drains a node on scale-down and upgrade, and lets a failed upgrade be retried and pick up where it stopped.
Fixes #14610
The drain fixes and the resumable upgrade are in one PR because they change the same upgrade loop in
KubernetesClusterUpgradeWorker: the drain, the Ready wait, the per-node SSH address and the skip of finished nodes are all steps of that loop, and splitting them would mean two PRs conflicting on every line of it.Drain (scale-down and upgrade)
Both workers had their own drain loop, and neither handled a drain that fails or takes a while. The most common cause is a PodDisruptionBudget that doesn't allow an eviction yet.
kubectl drainwithout--timeout, so a fixed 60s SSH timeout cut it off.There is now one helper,
KubernetesClusterActionWorker.drainKubernetesClusterNode:kubectl drain --timeout=<cloud.kubernetes.cluster.node.drain.timeout>s. The new setting defaults to 300 and is dynamic; values below 1 use the default.cloud.kubernetes.cluster.upgrade.retries+ 1 on upgrade, as before.Scale-down now cordons every node it is going to remove, then drains them one by one. A node that was cordoned already, for example by an operator, is left cordoned on a rollback. So is a node whose state can't be read. Cordoning them all first stops pods evicted from one being rescheduled onto another that is also going away.
kubectl delete nodegets its own retries, with--ignore-not-foundso that a retry after a lost reply doesn't fail.A removed node's VM that can't be destroyed or expunged
kubernetes_cluster_vm_mapentry, which then held the cluster in Alert for good (and Alert blocks scaling). The entry is now deleted whatever happens to the VM. An expunge failure no longer fails the scale-down; it is a warning event, and the expunge task retries the VM. A destroy failure still fails it, naming the VM to destroy by hand. If the scale-down stops part way, the cluster's node count, cores and memory are brought back in line with the map, and a VM left behind has the cluster's SSH port-forwarding rule revoked.Upgrade
sshPort + i. That only works when the upgrade's VM list and the port-forwarding rules are in the same order. They aren't when there are manually upgraded nodes, which are left out of the list, or nodes added later, which get the next free port. The script could then run on the wrong node. Each node is now reached through its own SSH port-forwarding rule, the same lookupKubernetesClusterRemoveWorkeralready uses. Shared networks, where nodes are reached on their own address, are unchanged.Retrying a failed upgrade
A failed upgrade left the cluster in Alert.
upgradeKubernetesClusteronly accepted a Running cluster, so the upgrade could neither be retried nor finished. No schema change is needed to fix this.upgradeKubernetesVersionId, not for display. That detail lives in the existingkubernetes_cluster_detailskey/value table. It is removed once the cluster is Running or Stopped again, for example because the upgrade succeeded or the Alert state scanner recovered the cluster, so it can't make a later, unrelated Alert look like a failed upgrade.upgradeKubernetesClusteraccepts a cluster in Alert when that detail is the requested version, or another version with the same semantic version (for example one registered with a fixed ISO), and all of the cluster's VMs are running. A version with the same semantic version as the cluster's own is never taken for a retry. There is a new transitionAlert -UpgradeRequested-> Upgrading. Any other cluster in Alert is refused, with a message saying why.kubernetes_cluster_vm_map.kubernetes_node_version) changes as soon as that node has been upgraded, uncordoned and found Ready. It is already shown askubernetesnodeversionin the cluster's VM list, so progress can be followed there.kubectl get nodesalso reports the target version for it, and it is Ready. The recorded value is never trusted on its own, and a node that went NotReady is upgraded again. A node that is Ready but was cordoned after its upgrade is skipped, and stays cordoned.1.28.1no longer matches1.28.10, and it works line by line, because the SSH output also holds stderr, for example a warning fromsudo. Semver build metadata (+vendor.1) is ignored.kubeadm upgrade applydoesn't run again. The other nodes only ever runkubeadm upgrade node.reconcileAlertClusterno longer moves the cluster to Running unless it managed to move it to Recovering first.KubernetesClusterDaoImpl.updateStatewrites the new state without checking the old one). Stop concurrent CKS operations clobbering each other, recover stuck states #14609 adds compare-and-set state updates and stuck-state recovery, and closes this race. The two PRs are independent; whichever merges second needs a small rebase.Overlap with #14604
#14604 ("Allow scaling a Kubernetes cluster that is in Alert") also changes
KubernetesClusterScaleWorker.removeKubernetesClusterNode, to skip the drain for a VM that is down. Whichever of the two is merged second will need a small rebase. The skip moves intodrainNodesToRemovehere, so a down VM is neither cordoned nor drained.The two don't depend on each other. When this PR rolls back a scale whose drain failed, a cluster that started in Alert goes back to Alert, not Running, so it stays correct once #14604 allows scaling from Alert. On 4.22 alone that branch is not reached.
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
Unit tests only. This has not been run against a live cluster or hypervisor.
New tests:
KubernetesClusterActionWorkerTest:kubectl drain --timeout=300scommand and SSH wait are checked;getNodeSshAddressuses the node's own port-forwarding rule on the cluster's public IP, ignores a rule that is being revoked, falls back tosshPort + indexwhen there is none, and uses the private IP on a shared network.KubernetesClusterScaleWorkerTest:OperationSucceeded(back to Running), notOperationFailed, and the error names the node and includes kubectl's output;KubernetesClusterScaleWorkerTest, node removal: the happy path destroys, expunges and then drops each map entry; an expunge failure (false or an exception) drops the entry, records a warning and goes on; a destroy failure drops the entry, fails the scale, uncordons the nodes still in the cluster and takes the removed VMs off the node count, cores and memory; a destroy error for a VM that turns out destroyed or expunged goes on; a failed Kubernetes node delete keeps the entry; a failure after every node is removed still reconciles the counts; only the cluster's SSH rule on the removed VM is revoked.KubernetesClusterUpgradeWorkerTest: a node is skipped only when both its recorded and its reported version are the target and it is Ready, also when it is cordoned. It is not skipped when only the recorded one is, when the node is NotReady, when the node can't be reached, when the recorded version is older, or when the versions only share a prefix.KubernetesClusterManagerImplTest: the Alert retry check:KubernetesClusterUtilTest: the version match ignores a trailing newline, a line of stderr and build metadata, and is otherwise exact.mvn -pl plugins/integrations/kubernetes-service -am test -Dtest='KubernetesCluster*Test'passes: 84 tests across 7 classes. Checkstyle passes onapiandkubernetes-service.How did you try to break this feature and the system with this change?