bhouse-nexthop opened a new pull request, #14611:
URL: https://github.com/apache/cloudstack/pull/14611
### 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.
- **Scale-down** gave up at once on a non-zero exit, counted an attempt
twice after an exception, left the node cordoned, and sent the cluster to
Alert. Nothing had been removed at that point.
- **Upgrade** retried with no wait and left the node cordoned on the last
failure. It now uncordons it, and also when it times out after the drain,
unless the node was cordoned before the upgrade.
- **Both** ran `kubectl drain` without `--timeout`, so a fixed 60s SSH
timeout cut it off.
There is now one helper,
`KubernetesClusterActionWorker.drainKubernetesClusterNode`:
- Each attempt counts once. A non-zero exit or an exception is retried after
a 30s wait, unless that would go past the operation's deadline.
- It runs `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.
- The timeout is cut down when less time is left before the operation's
deadline, so that pods aren't evicted by an attempt that can't be used. The SSH
wait is the drain timeout plus 60s.
- The error includes the end of kubectl's output, up to 2000 characters, as
a drain blocked by a PodDisruptionBudget logs every retry.
- The helper leaves the node as it is. The caller uncordons it, and only if
the caller cordoned it.
- Attempts: 3 on scale-down, as before;
`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.
- If any node can't be cordoned or drained, every cordoned node is
uncordoned. The cluster goes back to the state it was in, and the error names
the node. Nothing has been removed, so Alert would be wrong.
- After that, the nodes are deleted and their VMs destroyed as before.
`kubectl delete node` gets its own retries, with `--ignore-not-found` so that a
retry after a lost reply doesn't fail.
- If the scale times out before every node is drained, it is rolled back in
the same way, with nothing removed. Once the nodes are drained, the timeout no
longer stops the removal part way. Before, a timeout could leave some nodes
removed and the node count not updated.
- If deleting a node or destroying its VM fails, the cluster goes to Alert
as before. The drained nodes that are still in the cluster, and that were
cordoned by the scale-down, are uncordoned, so they aren't left unschedulable.
#### Upgrade
- **Ready check:** every node is now waited on to be Ready after it is
uncordoned, not only the first control node. Before, the next node could be
drained while the previous one was still coming back.
- **SSH address:** the script for node *i* was run on `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 lookup `KubernetesClusterRemoveWorker` already
uses. Shared networks, where nodes are reached on their own address, are
unchanged.
- **Error messages** name the Kubernetes node as well as the VM.
#### Retrying a failed upgrade
A failed upgrade left the cluster in Alert. `upgradeKubernetesCluster` only
accepted a Running cluster, so the upgrade could neither be retried nor
finished. No schema change is needed to fix this.
- **Recording the upgrade:** an upgrade now records the id of the version it
is upgrading to as a cluster detail, `upgradeKubernetesVersionId`, not for
display. That detail lives in the existing `kubernetes_cluster_details`
key/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.
- **Accepting the retry:** `upgradeKubernetesCluster` accepts 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 transition `Alert
-UpgradeRequested-> Upgrading`. Any other cluster in Alert is refused, with a
message saying why.
- **Upgrades that failed before this change:** these have no detail, so the
nodes' versions are used instead.
- The cluster's version only changes once every node has been upgraded.
- Each node's version
(`kubernetes_cluster_vm_map.kubernetes_node_version`) changes as soon as that
node has been upgraded, uncordoned and found Ready. It is already shown as
`kubernetesnodeversion` in the cluster's VM list, so progress can be followed
there.
- So a cluster still at the old version with a node at the target, not
counting manually upgraded nodes, had an upgrade to that version stop part way.
- **Skipping finished nodes:** a node is skipped when its recorded version
is the target, `kubectl get nodes` also 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.
- **Exact version match:** the existing check after each node is upgraded
now uses the same comparison. It is exact, so `1.28.1` no longer matches
`1.28.10`, and it works line by line, because the SSH output also holds stderr,
for example a warning from `sudo`. Semver build metadata (`+vendor.1`) is
ignored.
- **Order:** the first control node still comes first. If it was already
done, it is skipped, and `kubeadm upgrade apply` doesn't run again. The other
nodes only ever run `kubeadm upgrade node`.
- **UI:** the Upgrade action is now also shown for a cluster in Alert.
- **Racing the Alert state scanner:** the scanner tries to recover clusters
in Alert, and could otherwise overwrite the state of an upgrade that has just
started.
- The upgrade now stops if it can't move the cluster to Upgrading.
- `reconcileAlertCluster` no longer moves the cluster to Running unless it
managed to move it to Recovering first.
- A failed upgrade now detaches the binaries ISO before moving the cluster
to Alert. Otherwise a quick retry could have its ISO detached by the job that
failed.
- These guards narrow the race, but they don't close it, because cluster
state updates are not compare-and-set today
(`KubernetesClusterDaoImpl.updateState` writes the new state without checking
the old one). A separate PR, being prepared now, adds stuck-state recovery and
compare-and-set state updates, and closes this race.
#### 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 into `drainNodesToRemove` here, 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
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [x] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] Major
- [ ] Minor
- [ ] Trivial
### How Has This Been Tested?
Unit tests only. This has **not** been run against a live cluster or
hypervisor.
New tests:
- **`KubernetesClusterActionWorkerTest`:**
- the drain is retried after a non-zero exit and succeeds, with one wait
and no uncordon;
- an exception and then failures use every attempt and wait between them;
- kubectl's timeout is cut down to the time left before the deadline, no
attempt is made without time left, and there is no retry after an interrupted
wait;
- the exact `kubectl drain --timeout=300s` command and SSH wait are
checked;
- `getNodeSshAddress` uses the node's own port-forwarding rule on the
cluster's public IP, ignores a rule that is being revoked, falls back to
`sshPort + index` when there is none, and uses the private IP on a shared
network.
- **`KubernetesClusterScaleWorkerTest`:**
- every node is cordoned before any is drained;
- when the second node can't be drained, the first is uncordoned (the
second already was, by the helper), the cluster gets `OperationSucceeded` (back
to Running), not `OperationFailed`, and the error names the node and includes
kubectl's output;
- a cluster that started in Alert goes back to Alert;
- a scale that is past its deadline drains nothing, uncordons the nodes
and rolls back;
- a node that was cordoned before the scale-down, as reported even with a
line of stderr, is not uncordoned by it, and neither is a node whose state
can't be read.
- **`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:
- accepts the recorded upgrade version even when no node has reached it,
and another version with the same semantic version, and refuses a different
version;
- without that record, accepts a node at the target version, and refuses a
cluster with no node there or where only a manually upgraded node is there;
- refuses a target that is the cluster's current version, or another
version with the same semantic version.
- **`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 on `api` and `kubernetes-service`.
#### How did you try to break this feature and the system with this change?
- **A normal upgrade from Running:** every node is still at the old recorded
version, so the skip check makes no SSH call and nothing changes.
- **Shared networks** (SSH port 22): node addressing is unchanged.
- **Port-forwarding rules** that are being revoked are ignored when looking
up a node's SSH port.
- **An interrupted wait** between drain attempts stops the retries instead
of firing the rest with no wait.
- **A scale-down where every drain succeeds:** nodes are deleted and their
VMs destroyed in the same order as before. The difference is that all of them
are cordoned and drained first.
- **Not covered:** a scale-down that fails after some nodes have been
removed still goes to Alert, as before. The nodes that are still there are
uncordoned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
