Repository navigation
feat(arc): let the node recovery watcher delete a labelled node that stayed Unknown - #111
Conversation
…stayed Unknown An opt-in second pass deletes the Node object of a node whose Ready condition has been Unknown for github_runner_arc_node_recovery_prune_after_seconds and which carries the removable label. Empty leaves it off, and then the watcher's ClusterRole keeps get, list and patch only. The delete is a single API request with the node's uid and resourceVersion as preconditions, which kubectl delete cannot send, so a node that came back since it was listed is left alone. A control-plane or etcd member is refused even when labelled, as is the watcher's own node.
… test Phase 4 turns pruning on with the stopped worker carrying the removable label from its kubelet's registration, as a k3s agent does with --node-label. It checks the delete permission appears only then, that the Node is deleted no sooner than the prune threshold, that the other nodes remain, and that starting the worker registers a new Node with its label that reports Ready.
A rollout leaves the previous pod terminating beside the new one, and the deployment's log selector can pick the one that never saw the deletion.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsFinding details are still loading. Check the individual review comments. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The other node labels the fleet sets sit under github-runner.exadev.io, so the removable label belongs there too. The annotations keep the bare domain.
k3s removes a server's etcd member when that server's Node is deleted, and three voters are the fewest a cluster needs to tolerate losing one. The prune pass now counts the Ready nodes carrying the etcd role label, leaves a non-voting agent out of the count, and keeps the candidate with a logged reason and a failed pass when fewer than that would remain. The kind test labels three Ready nodes as etcd members to let the prune through, after checking that none and two keep it.
…est expects kind numbers workers in the order the config lists them, and the label patch belongs to the worker the test stops.
…efusal The refusal is logged only once the node is past the prune threshold, which phase 3 may not yet have reached when phase 4 turns pruning on.
A log request that names several pods stops at the first one it cannot read, which hid the new pod's output behind the terminating one.
|
@codex security review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 174eb1c020
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
With pruning on, the pod's ServiceAccount token can delete nodes, and the script's label, quorum and precondition checks are client-side. A token read from a lower-trust worker could be used against the API directly, so the pod now requires a control-plane node and tolerates its taint. With pruning off the pod stays unpinned and keeps its short eviction tolerations. The kind test drops its worker pin before phase 4 and checks the pod ends up on the control plane.
|
@codex security review |
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
|
@codex security review |
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
|
@codex security review |
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
|
🎉 This PR is included in version 2.4.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
1 participant
Fixes #110.
Adds an opt-in prune pass to the node recovery watcher. With
github_runner_arc_node_recovery_prune_after_secondsset (a whole number of seconds, strictly greater than the taint threshold, no default; empty means off) the watcher deletes the Node of a node whose Ready condition has been Unknown that long and which carriesgithub_runner_arc_node_recovery_prune_label(defaultgithub-runner.exadev.io/removable) with the valuetrue. Never deleted: an unlabelled node, the watcher's own node, a node with no Ready condition, and a node with anode-role.kubernetes.io/control-planeornode-role.kubernetes.io/etcdlabel even if it also has the removable one (the pass fails saying so, since deleting a k3s server's Node removes its etcd member). It also deletes nothing unless at least three Ready nodes carrying thenode-role.kubernetes.io/etcdlabel would remain (a non-voting agent and a member that is not Ready do not count; three because a cluster needs that many voters to tolerate losing one). Below that the node is kept, the pass fails and the log says why, so a cluster with no etcd members is never pruned. k3s removes a server's etcd member when its Node is deleted (pkg/etcd/member_controller.go,Removing etcd member from cluster due to node delete, only for a Node with the etcd role label), which is why the floor is there. It is a constant in the script, not a setting.RBAC. While pruning is on the watcher's ClusterRole gains
deleteonnodes, which is a cluster-wide delete on nodes: RBAC cannot select by label, so the script is what narrows it. While it is off the role is exactly what it was.Placement. The script's checks are client-side, so a ServiceAccount token that can delete nodes is a credential worth protecting: with pruning on the Deployment requires a control-plane node (node affinity on
node-role.kubernetes.io/control-planeand a toleration for its taint) instead of landing on any worker. With pruning off it stays unpinned as before. Found by the Codex security review.The delete.
kubectl deletecannot send preconditions, so the script sendsDELETE /api/v1/nodes/<name>with curl and the node'suidandresourceVersionasDeleteOptions.preconditions, using the pod's ServiceAccount token (passed to curl on stdin, not on the command line). A node that changed between the list and the delete gets a 409 and is left for the next pass, the same stance as the resourceVersion test in the taint patch. The watcher image gainscurl. A node past the prune threshold is deleted in place of being tainted, never both in one pass.Limit. Deleting the Node of a running k3s agent does not bring it back. On a throwaway k3s v1.34.1 cluster (server and one labelled agent in Docker) I deleted a running agent's Node: its kubelet logged
node "..." not foundfor the rest of the 3 minutes I watched, and the node-password Secret went with the Node.docker restartof that agent re-registered it about 8 seconds later with a new uid, the label applied again and Ready. I also cut the agent off from the network with its kubelet running, waited for Unknown, deleted the Node and reconnected: still absent after 4 minutes. So pruning is safe only for nodes that restart their agent when their own Node vanishes, or that reboot; that is the node's responsibility and the docs say so.Real-cluster runs. Three k3s v1.34.1 servers on embedded etcd plus two agents started with
--node-label github-runner.exadev.io/removable=true, in Docker, the watcher image built from this branch and the role's rendered manifests applied with pruning on (taint after 20s, prune after 60s, poll 5s) and no placement override. The cluster reports the role label the floor counts asnode-role.kubernetes.io/etcd=trueon all three servers (besidenode-role.kubernetes.io/control-plane=true); the agents carry neither. The ClusterRole came out asget, list, patch, deleteandcan-i delete nodesas the watcher's ServiceAccount gave yes; the pod was scheduled on a server by the role's affinity. With three Ready servers I stopped the first agent: the taint landed at 24s Unknown, the Node was deleted at 63s (Deleted ...: not reporting for 63s), all three servers stayed, and starting the agent again registered a new Node (new uid) that was Ready with the label, no step by hand. Then I stopped a server (leaving two Ready etcd members, confirmed by label selector) and the second agent: past the threshold the watcher refused on every pass (Not deleting ...: only 2 Ready etcd members would remain, fewer than the 3 a cluster needs to tolerate losing one), the agent's Node was still there after the threshold plus a further wait of many polls, and the stopped server was not pruned (it is unlabelled). Starting the server again made three voters, and the agent's Node was deleted at the next pass (Deleted ...: not reporting for 119s) with all three servers present. An earlier run on a single SQLite server and one agent (no etcd members, before the floor existed) showed what a running agent does when its Node goes: the limit above. The watcher in all of these ran as the role renders it (non-root, read-only root filesystem, dropped capabilities).Tests.
tests/unit/test_node_recovery.pyruns the script against a scripted kubectl and a curl stand-in that behaves like the API's preconditions (409 when uid or resourceVersion no longer match, 404 when gone): pruned when labelled and past the threshold; not when unlabelled, label nottrue, below the threshold, Ready, NotReady, its own node, or without a Ready condition; refused for a labelled control-plane or etcd node without stopping other nodes being pruned; left alone with a 409 when it recovered between list and delete; the token on stdin only; the label key configurable; disabled when the threshold is empty or unset; kept, with a failed pass and a logged reason, with fewer than three Ready etcd voters (none, two, a member that is not Ready, or agents standing in for voters) and pruned with three or more; refused at start-up when the threshold is not greater than the taint threshold or not a whole number.tests/unit/test_node_recovery_role.pyrenders the role's manifests with Ansible (ClusterRole withoutdeleteand no prune settings in the environment when off, with both when on) and evaluates the newvalidate.ymlchecks with Ansible itself. Each test was shown to fail when the code it covers is broken (label check, threshold comparison, preconditions, Unknown check, server refusal, empty-means-off, ordering validation, token on argv, own-node check, 409 handling; template verbs, template environment, validation comparison and label pattern).tests/node_recovery/run.sh(the kind job) gains a phase that turns pruning on with the stopped worker labelled by its kubelet, and checks the delete permission appears only then, the Node goes no sooner than the threshold, the other nodes stay, and the worker registers again labelled and Ready when started. It passes in CI on kind (Kubernetes 1.37): the Node was kept with no etcd voters and with two, went at the 60s threshold once a third voter existed, and the worker came back labelled and Ready. I did not run the kind job on my machine.