From 989a36346de3f9cc65486051e4b2fc176e6aa6b4 Mon Sep 17 00:00:00 2001 From: Joseph Mearman Date: Tue, 3 Nov 2026 21:23:50 +0000 Subject: [PATCH 1/2] feat(arc): let node-recovery read the regrade Lease and take the regrade level label A Role in the Lease's namespace allows get on the one Lease that serialises voter swaps and regrading, and nothing else there. The watcher is told the Lease, the annotation that lists a swap's nodes and the level label through its environment; the level label follows the cluster role's setting. --- roles/github_runner_arc/defaults/main.yml | 2 + .../github_runner_arc/meta/argument_specs.yml | 3 ++ .../templates/node-recovery.yaml.j2 | 39 ++++++++++++++++++- roles/github_runner_arc/vars/main.yml | 5 +++ tests/unit/test_node_recovery_role.py | 33 ++++++++++++++++ 5 files changed, 81 insertions(+), 1 deletion(-) diff --git a/roles/github_runner_arc/defaults/main.yml b/roles/github_runner_arc/defaults/main.yml index a85fd133..d2769cac 100644 --- a/roles/github_runner_arc/defaults/main.yml +++ b/roles/github_runner_arc/defaults/main.yml @@ -186,6 +186,8 @@ github_runner_arc_node_recovery_poll_seconds: 15 github_runner_arc_node_recovery_prune_after_seconds: "" # The label a node must carry, with the value true, to be pruned. Set it on the nodes that may go (for a k3s agent, --node-label on its command line, which it applies again each time it registers), never on a server. github_runner_arc_node_recovery_prune_label: github-runner.exadev.io/removable +# The label regrading puts on a server it has stepped down, with the level as its value, which the watcher never taints: the cluster role's github_runner_cluster_regrade_level_label when that is defined, else github-runner.exadev.com/regrade-level (held together with the role controller's by the same expression). +github_runner_arc_node_recovery_level_label: "{{ github_runner_cluster_regrade_level_label | default('github-runner.exadev.com/regrade-level') }}" # Image the watcher runs. Pulled without a pull Secret, so it must be public. github_runner_arc_node_recovery_image: "ghcr.io/exadev/github-runner-node-recovery:2.31.0" diff --git a/roles/github_runner_arc/meta/argument_specs.yml b/roles/github_runner_arc/meta/argument_specs.yml index 78ca8566..aef1a1dc 100644 --- a/roles/github_runner_arc/meta/argument_specs.yml +++ b/roles/github_runner_arc/meta/argument_specs.yml @@ -371,6 +371,9 @@ argument_specs: type: str default: github-runner.exadev.io/removable description: The label key a node must carry, with the value true, to be pruned. A Kubernetes qualified name. + github_runner_arc_node_recovery_level_label: + type: str + description: The label regrading puts on a server it has stepped down, which the node recovery watcher never taints, from github_runner_cluster_regrade_level_label when defined, else github-runner.exadev.com/regrade-level. github_runner_arc_node_recovery_image: type: str description: Image the node recovery watcher runs, pulled without a pull Secret. diff --git a/roles/github_runner_arc/templates/node-recovery.yaml.j2 b/roles/github_runner_arc/templates/node-recovery.yaml.j2 index 577e6070..487d7032 100644 --- a/roles/github_runner_arc/templates/node-recovery.yaml.j2 +++ b/roles/github_runner_arc/templates/node-recovery.yaml.j2 @@ -1,7 +1,8 @@ -{# The node recovery watcher (see tasks/install_node_recovery.yml and scripts/node-recovery.sh): a ServiceAccount, a ClusterRole allowing get, list and patch on nodes and, only while github_runner_arc_node_recovery_prune_after_seconds is set, delete, its binding, and the Deployment. The namespace is ensured separately, since the platform's other Deployments share it. #} +{# The node recovery watcher (see tasks/install_node_recovery.yml and scripts/node-recovery.sh): a ServiceAccount, a ClusterRole allowing get, list and patch on nodes and, only while github_runner_arc_node_recovery_prune_after_seconds is set, delete, its binding, a Role allowing get on the one Lease that serialises voter swaps and regrading with its binding, and the Deployment. The namespace is ensured separately, since the platform's other Deployments share it. #} {% set name = 'node-recovery' %} {% set prune = github_runner_arc_node_recovery_prune_after_seconds | string | length > 0 %} {% set cluster_name = 'github-runner-node-recovery' %} +{% set lease_role = 'github-runner-node-recovery-lease' %} {% set labels = {'app.kubernetes.io/name': name, 'app.kubernetes.io/part-of': 'github-runner'} %} apiVersion: v1 kind: ServiceAccount @@ -35,6 +36,34 @@ roleRef: kind: ClusterRole name: {{ cluster_name | to_json }} --- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: {{ lease_role | to_json }} + namespace: {{ github_runner_arc_node_recovery_lease_namespace | to_json }} + labels: {{ labels | to_json }} +rules: + # Reading the Lease is how the watcher learns which nodes a swap or a regrade step is moving through. Named, so it can read no other Lease. + - apiGroups: ["coordination.k8s.io"] + resources: ["leases"] + resourceNames: [{{ github_runner_arc_node_recovery_lease_name | to_json }}] + verbs: ["get"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: {{ lease_role | to_json }} + namespace: {{ github_runner_arc_node_recovery_lease_namespace | to_json }} + labels: {{ labels | to_json }} +subjects: + - kind: ServiceAccount + name: {{ name | to_json }} + namespace: {{ github_runner_arc_platform_namespace | to_json }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: {{ lease_role | to_json }} +--- apiVersion: apps/v1 kind: Deployment metadata: @@ -92,6 +121,14 @@ spec: value: {{ github_runner_arc_node_recovery_poll_seconds | string | to_json }} - name: NODE_RECOVERY_ANNOTATION value: {{ github_runner_arc_node_recovery_annotation | to_json }} + - name: NODE_RECOVERY_LEASE_NAMESPACE + value: {{ github_runner_arc_node_recovery_lease_namespace | to_json }} + - name: NODE_RECOVERY_LEASE_NAME + value: {{ github_runner_arc_node_recovery_lease_name | to_json }} + - name: NODE_RECOVERY_SWAP_NODES_ANNOTATION + value: {{ github_runner_arc_node_recovery_swap_nodes_annotation | to_json }} + - name: NODE_RECOVERY_LEVEL_LABEL + value: {{ github_runner_arc_node_recovery_level_label | to_json }} {% if prune %} - name: NODE_RECOVERY_PRUNE_AFTER_SECONDS value: {{ github_runner_arc_node_recovery_prune_after_seconds | string | to_json }} diff --git a/roles/github_runner_arc/vars/main.yml b/roles/github_runner_arc/vars/main.yml index 2bdd2486..6ca12d1f 100644 --- a/roles/github_runner_arc/vars/main.yml +++ b/roles/github_runner_arc/vars/main.yml @@ -13,6 +13,11 @@ github_runner_arc_pull_secret_field_manager: github-runner-pull-secret-renewer # Annotation the node recovery watcher writes alongside the out-of-service taint it applies, recording when it applied it. The watcher removes only a taint carrying this annotation, so an operator's own out-of-service taint is left alone. github_runner_arc_node_recovery_annotation: github-runner.exadev/out-of-service-applied-at +# The Lease that serialises voter swaps and regrading steps (the cluster role's fleet-regrade-core.sh, LEASE_NAMESPACE and LEASE_NAME), and the annotation on it that lists the nodes a swap is moving through, which the role controller writes: the node recovery watcher reads them to leave those nodes alone, and its Role grants get on that one name. +github_runner_arc_node_recovery_lease_namespace: kube-system +github_runner_arc_node_recovery_lease_name: fleet-regrade +github_runner_arc_node_recovery_swap_nodes_annotation: github-runner.exadev.com/swap-nodes + # Whether the runner pods and the platform's Deployments reference a pull Secret at all. With github_runner_arc_image_pull_secret_source none every image they run is public, so the role writes, checks and references none, and removes one an earlier run wrote. github_runner_arc_pull_secret_in_use: "{{ github_runner_arc_image_pull_secret_source != 'none' }}" diff --git a/tests/unit/test_node_recovery_role.py b/tests/unit/test_node_recovery_role.py index 4f5fc8d3..38bae5d2 100644 --- a/tests/unit/test_node_recovery_role.py +++ b/tests/unit/test_node_recovery_role.py @@ -91,6 +91,39 @@ def test_the_binding_still_grants_the_role_to_the_watchers_service_account(self) self.assertEqual(manifests["ClusterRoleBinding"]["subjects"][0]["name"], manifests["ServiceAccount"]["metadata"]["name"]) +class LeaseAccessTest(unittest.TestCase): + """What the watcher may read to know a voter swap or a regrade step is under way: the Lease that serialises both, and nothing else in its namespace.""" + + LEASE_NAME = "fleet-regrade" + LEASE_NAMESPACE = "kube-system" + LEVEL_LABEL = "github-runner.exadev.com/regrade-level" + + def test_the_role_allows_get_on_the_regrade_lease_by_name_and_nothing_else(self) -> None: + for role_vars in ({}, ON): + with self.subTest(role_vars): + role = render(**role_vars)["Role"] + self.assertEqual(role["metadata"]["namespace"], self.LEASE_NAMESPACE) + self.assertEqual(role["rules"], [{"apiGroups": ["coordination.k8s.io"], "resources": ["leases"], "resourceNames": [self.LEASE_NAME], "verbs": ["get"]}]) + + def test_the_role_is_bound_to_the_watchers_service_account_in_the_leases_namespace(self) -> None: + manifests = render() + binding = manifests["RoleBinding"] + self.assertEqual(binding["metadata"]["namespace"], self.LEASE_NAMESPACE) + self.assertEqual(binding["roleRef"], {"apiGroup": "rbac.authorization.k8s.io", "kind": "Role", "name": manifests["Role"]["metadata"]["name"]}) + [subject] = binding["subjects"] + self.assertEqual((subject["kind"], subject["name"], subject["namespace"]), ("ServiceAccount", manifests["ServiceAccount"]["metadata"]["name"], manifests["ServiceAccount"]["metadata"]["namespace"])) + + def test_the_watcher_is_told_which_lease_and_label_to_look_for(self) -> None: + env = container_env(render()) + self.assertEqual( + (env["NODE_RECOVERY_LEASE_NAMESPACE"], env["NODE_RECOVERY_LEASE_NAME"], env["NODE_RECOVERY_SWAP_NODES_ANNOTATION"], env["NODE_RECOVERY_LEVEL_LABEL"]), + (self.LEASE_NAMESPACE, self.LEASE_NAME, "github-runner.exadev.com/swap-nodes", self.LEVEL_LABEL), + ) + + def test_the_level_label_follows_the_cluster_roles_setting(self) -> None: + self.assertEqual(container_env(render(github_runner_cluster_regrade_level_label="example.com/level"))["NODE_RECOVERY_LEVEL_LABEL"], "example.com/level") + + class ValidationTest(unittest.TestCase): """The prune settings' checks in tasks/validate.yml, evaluated by Ansible itself the way the role evaluates them, every case in one run.""" From 643de6b98ec22b525cb1ae07007b0af97f405a30 Mon Sep 17 00:00:00 2001 From: Joseph Mearman Date: Tue, 3 Nov 2026 21:23:52 +0000 Subject: [PATCH 2/2] feat(arc): leave a role swap's nodes and regraded servers alone in node-recovery While the fleet-regrade Lease is unexpired, node-recovery neither taints nor prunes a node its swap-nodes annotation lists or its holder names, and it never taints a node carrying the regrade level label. A swap or a regrade step makes a node silent on purpose, and an out-of-service taint would evict pods the controller is draining deliberately. A taint applied earlier is still cleared once the node is Ready, and a Lease that cannot be read fails the pass before any change. Refs #183 --- README.md | 2 + roles/github_runner_arc/README.md | 4 +- scripts/node-recovery.sh | 34 ++++++++- tests/unit/test_node_recovery.py | 115 +++++++++++++++++++++++++++++- 4 files changed, 148 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index aec28661..ed4ae1cb 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,8 @@ The watcher only acts on a node whose kubelet has gone silent (`Unknown`), never The threshold is also where a stopped node's runner pods are given up, earlier than the five-minute default eviction would. Their containers stopped with the node, so their jobs are lost either way unless the node returns within that time; lower the threshold for faster recovery, or raise it to wait longer for a node that is only asleep. +It also leaves alone the nodes a voter swap or a regrading step is moving through, since those stay silent on purpose: while the Lease `kube-system/fleet-regrade` is unexpired it neither taints nor prunes a node its `github-runner.exadev.com/swap-nodes` annotation lists or its holder names, and it never taints a node carrying the regrade level label. See the role README's Node recovery section. + Pruning is opt-in (`github_runner_arc_node_recovery_prune_after_seconds`, empty by default). When it is set, the watcher also deletes the Node object of a node that has been `Unknown` for that long and carries the `github-runner.exadev.io/removable=true` label (`github_runner_arc_node_recovery_prune_label`), so a device that is never coming back stops being listed. It never deletes an unlabelled node, a node that is a control-plane or etcd member (even if labelled), its own node or a node without a `Ready` condition, and it deletes nothing unless at least three `Ready` nodes carrying the etcd role label would remain, because k3s removes a server's etcd member when its Node is deleted. It sends the delete with the node's `uid` and `resourceVersion` as preconditions so a node that has come back since it was listed is left alone. Turning it on gives the watcher a cluster-wide `delete` on nodes. Its pod is then confined to control-plane nodes, since a token that can delete nodes should not be readable from a worker. A pruned node comes back only when its k3s agent process restarts: a running kubelet does not register itself again once its Node object is gone, so pruning is safe only for nodes that restart their agent when their Node vanishes or that reboot, which the node has to arrange. See the role README's Node recovery section for the details. Its image, `github_runner_arc_node_recovery_image`, is pulled without a pull Secret, so the published package must be public. diff --git a/roles/github_runner_arc/README.md b/roles/github_runner_arc/README.md index 379ee194..c83653da 100644 --- a/roles/github_runner_arc/README.md +++ b/roles/github_runner_arc/README.md @@ -55,7 +55,7 @@ Helm 4 and server-side apply. The role reads `helm version --short` and, on Helm - `github_runner_arc_image_pull_registry`, `github_runner_arc_image_pull_secret_name`, `github_runner_arc_verify_image_pull`: the pull Secret's registry and name, and whether to prove the credential before writing it. - `github_runner_arc_heartbeat_gist_id`: install the fleet-health platform from this host, refreshing this gist. `github_runner_arc_heartbeat_bootstrap_gist`, `github_runner_arc_heartbeat_gist_description` and `github_runner_arc_heartbeat_gist_consumer` control the bootstrap described above. - `github_runner_arc_autoscaler_usable_budget_gi`, `github_runner_arc_autoscaler_floor`: required when a profile sets `autoscale: true` and the platform is installed. The floor must equal the autoscaled profile's `maxRunners`, which every Helm upgrade reverts to. The autoscaler works its ceiling out from the nodes on every poll (see the project README's Autoscaler section); `github_runner_arc_autoscaler_max_ceiling`, optional, is an upper bound on it and must be no lower than the floor. The other `github_runner_arc_autoscaler_*` and `github_runner_arc_heartbeat_*` settings have defaults; see `defaults/main.yml`. -- `github_runner_arc_node_recovery_enabled` (default `true`), `github_runner_arc_node_recovery_after_seconds` (default 120), `github_runner_arc_node_recovery_poll_seconds` (default 15), `github_runner_arc_node_recovery_prune_after_seconds` (default empty, which leaves pruning off), `github_runner_arc_node_recovery_prune_label` (default `github-runner.exadev.io/removable`), `github_runner_arc_node_recovery_image`: the node recovery watcher (see [Node recovery](#node-recovery)). +- `github_runner_arc_node_recovery_enabled` (default `true`), `github_runner_arc_node_recovery_after_seconds` (default 120), `github_runner_arc_node_recovery_poll_seconds` (default 15), `github_runner_arc_node_recovery_prune_after_seconds` (default empty, which leaves pruning off), `github_runner_arc_node_recovery_prune_label` (default `github-runner.exadev.io/removable`), `github_runner_arc_node_recovery_level_label` (default the cluster role's `github_runner_cluster_regrade_level_label`, else `github-runner.exadev.com/regrade-level`), `github_runner_arc_node_recovery_image`: the node recovery watcher (see [Node recovery](#node-recovery)). - `github_runner_arc_role_controller_enabled` (default the cluster role's `github_runner_cluster_role_controller`, itself `false`), `github_runner_arc_role_controller_image`, `github_runner_arc_role_controller_sample_seconds` (default 15), `github_runner_arc_role_controller_voters` (default 3), `github_runner_arc_role_controller_max_servers`, `github_runner_arc_role_controller_election_timeout_ms`, `github_runner_arc_role_controller_pressure_threshold`, `github_runner_arc_role_controller_available_floor_percent`, `github_runner_arc_role_controller_eligible_label`, `github_runner_arc_role_controller_pinned_label`, `github_runner_arc_role_controller_level_label`, `github_runner_arc_role_controller_link_label`, `github_runner_arc_role_controller_probe_configmap`, `github_runner_arc_role_controller_status_configmap`, `github_runner_arc_role_controller_data_dir`: the role controller, which reports which hosts are fit to hold an etcd vote (see [Role controller](#role-controller)). Those that mirror the cluster role (`voters`, `max_servers`, `election_timeout_ms`, `pressure_threshold`, `available_floor_percent`, `eligible_label`, `pinned_label`, `level_label`, `link_label`) take the cluster role's own variable when it is defined. - `github_runner_arc_runner_reaper_enabled` (default `false`), `github_runner_arc_runner_reaper_max_job_seconds` (default 432000, five days), `github_runner_arc_runner_reaper_completed_grace_seconds` (default 300), `github_runner_arc_runner_reaper_schedule` (default every 5 minutes), `github_runner_arc_runner_reaper_dry_run` (default `false`), `github_runner_arc_runner_reaper_image` (default the renewal's image): the runner reaper (see [Runner reaper](#runner-reaper)). - `github_runner_arc_runner_active_deadline_seconds` (default unset): `activeDeadlineSeconds` on every runner pod (see [Runner reaper](#runner-reaper)). @@ -473,6 +473,8 @@ github_runner_arc_orgs: Every host that installs anything also installs a small watcher, the `node-recovery` Deployment in `github_runner_arc_platform_namespace`, so that a node that stops does not leave its pods `Terminating` for ever. ARC waits for a listener's old pod to go before starting a new one, so a listener on a stopped node otherwise stalls its scale set until the pod is force-deleted by hand. Once a node's `Ready` condition has been `Unknown` for `github_runner_arc_node_recovery_after_seconds`, the watcher gives it the `node.kubernetes.io/out-of-service=nodeshutdown:NoExecute` taint of Kubernetes' non-graceful node shutdown, which makes the control plane evict the node's pods and force-delete the terminating ones; it removes the taint when the node reports `Ready` again. It never acts on a node reporting `NotReady`, never on its own node, and never removes a taint it did not apply. Its ClusterRole allows `get`, `list` and `patch` on nodes and nothing else, unless pruning is switched on (below). `github_runner_arc_node_recovery_enabled: false` removes the watcher, its RBAC and any taint it left behind. Its image is pulled without a pull Secret, so it must be public. The repository README's Node recovery section explains the default threshold. +The watcher also leaves alone the nodes a voter swap or a regrading step is moving through, because those are meant to be silent for a while: the host stops one k3s unit and starts the other, or a server runs at etcd-only with no kubelet, and an out-of-service taint would evict pods that the controller is draining on purpose. It reads the Lease `kube-system/fleet-regrade`, which serialises both, through a Role that allows `get` on that one name and nothing else in the namespace. While the Lease is unexpired (its renew time plus its duration has not passed, the rule `lease_state` applies in the cluster role's regrade core) it neither taints nor prunes a node that the Lease's `github-runner.exadev.com/swap-nodes` annotation lists (comma separated) or that its holder names, which is a node name when regrading holds the Lease. A node carrying the regrade level label (`github_runner_arc_node_recovery_level_label`, `github-runner.exadev.com/regrade-level` unless the cluster role is configured otherwise) is never tainted, whether or not any Lease is held. A taint the watcher applied earlier is still removed once such a node reports `Ready`. A Lease that cannot be read fails the pass before it changes anything, since tainting on a guess could cut a swap short. + Pruning is off by default. Setting `github_runner_arc_node_recovery_prune_after_seconds` to a whole number of seconds, greater than `github_runner_arc_node_recovery_after_seconds`, makes the watcher delete the Node object of a node whose `Ready` condition has been `Unknown` that long and which carries the label `github_runner_arc_node_recovery_prune_label` with the value `true`. The variable has no default value, since how long a node may be away before it is given up is the fleet's own decision. Use it for nodes that will not come back for a long time, such as agents on small devices that are unplugged, so that they stop being listed. The label is the node's own to set (for a k3s agent, `--node-label` on its command line, which it applies again every time it registers); a node without it is never deleted, and neither is a node with a `node-role.kubernetes.io/control-plane` or `node-role.kubernetes.io/etcd` label even if it also carries the removable one, because deleting a k3s server's Node removes its etcd member; the pass fails saying so until the label is removed. The watcher's own node and a node with no `Ready` condition are never deleted either. The pass also deletes nothing unless at least three `Ready` nodes carrying the `node-role.kubernetes.io/etcd` label would remain, three being the voters a cluster needs to tolerate losing one (a non-voting agent does not count, nor does a member that is not `Ready`); below that it keeps the node, fails the pass and says why, so a cluster with no etcd members, such as a single-server SQLite one, is never pruned. The floor is a constant in the script, not a setting. It exists because k3s removes a server's etcd member when that server's Node is deleted (`pkg/etcd/member_controller.go` in k3s: `Removing etcd member from cluster due to node delete`, done only for a Node carrying the etcd role label), so a prune must never be the thing that thins the voters. Turning pruning on adds `delete` on nodes to the watcher's ClusterRole. RBAC cannot select nodes by label, so that is a cluster-wide delete on nodes; the script is what narrows it. Because the script's checks are client-side, anyone who can read the pod's ServiceAccount token could call the API directly, so with pruning on the Deployment is confined to control-plane nodes (a required node affinity on `node-role.kubernetes.io/control-plane`, and a toleration for that taint) and not left to land on a lower-trust worker. That needs a control-plane node that can run the pod; with pruning off the pod stays unpinned. The delete is a request to the API carrying the node's `uid` and `resourceVersion` as preconditions (`kubectl delete` cannot send them, so the watcher image also contains `curl`), so a node that came back between being listed and being deleted is left alone. diff --git a/scripts/node-recovery.sh b/scripts/node-recovery.sh index 0df9bad0..9240f695 100755 --- a/scripts/node-recovery.sh +++ b/scripts/node-recovery.sh @@ -3,7 +3,7 @@ # # A node whose kubelet has stopped reporting (Ready condition Unknown) keeps its pods forever: the control plane marks them for deletion once their unreachable toleration runs out, but only the kubelet can confirm a deletion, so they stay Terminating. A replacement that has to wait for the old pod to go, such as an ARC listener or a StatefulSet pod, then never starts. Kubernetes' answer is non-graceful node shutdown: the node.kubernetes.io/out-of-service=nodeshutdown:NoExecute taint makes the control plane evict every pod on the node at once and force-delete the ones already terminating there. # -# This pass applies that taint to each node whose Ready condition has been Unknown for at least NODE_RECOVERY_AFTER_SECONDS, and removes it again once the node reports Ready. It only removes a taint it applied itself, recognised by the NODE_RECOVERY_ANNOTATION annotation it writes with it, so an operator's own out-of-service taint is left alone. A node reporting Ready False is never tainted: its kubelet is alive and still finishes its pods' deletion itself. The node this pass runs on (NODE_RECOVERY_SELF_NODE) is never tainted either. Each change is one JSON patch that first tests the node's resourceVersion, so a node whose status changed after it was read (for example because it came back) is left for the next pass instead of being tainted on stale information. Without the prune pass below it never deletes a Node object, and needs only get, list and patch on nodes. +# This pass applies that taint to each node whose Ready condition has been Unknown for at least NODE_RECOVERY_AFTER_SECONDS, and removes it again once the node reports Ready. It only removes a taint it applied itself, recognised by the NODE_RECOVERY_ANNOTATION annotation it writes with it, so an operator's own out-of-service taint is left alone. A node reporting Ready False is never tainted: its kubelet is alive and still finishes its pods' deletion itself. The node this pass runs on (NODE_RECOVERY_SELF_NODE) is never tainted either. Each change is one JSON patch that first tests the node's resourceVersion, so a node whose status changed after it was read (for example because it came back) is left for the next pass instead of being tainted on stale information. A node a voter swap or regrading is moving through is left alone, because it is meant to be silent for a while (its host stops one k3s unit and starts the other) and tainting it out of service would evict pods the controller is draining on purpose. While the Lease NODE_RECOVERY_LEASE_NAMESPACE/NODE_RECOVERY_LEASE_NAME (the cluster-wide Lease that serialises both) is unexpired, a node is skipped when the Lease's NODE_RECOVERY_SWAP_NODES_ANNOTATION annotation lists it (comma separated) or when its holderIdentity names it, and a node carrying the label NODE_RECOVERY_LEVEL_LABEL, which regrading puts on a server it has stepped down and which therefore has no kubelet, is never tainted whether or not any Lease is held. A node the Lease protects is neither tainted nor pruned (a level-labelled server is already refused by the prune pass), and a taint this pass applied earlier is still removed once the node reports Ready. The Lease is read with a Role that allows get on that one name; a Lease that cannot be read fails the pass before any change, since tainting on a guess could cut a swap short. Without the prune pass below it never deletes a Node object, and needs only get, list and patch on nodes. # # An opt-in second pass deletes a Node object that has stayed Unknown for at least NODE_RECOVERY_PRUNE_AFTER_SECONDS (empty turns it off) and carries the label NODE_RECOVERY_PRUNE_LABEL with the value true, which a node that rejoins by itself, such as a k3s agent, sets on itself when it registers. Deleting a Node removes it, and the pods bound to it, from the cluster; an agent that comes back registers a new Node object. A k3s server's Node is never deleted, because that removes its etcd member: a node that is a control-plane or etcd member is refused even when it carries the label, and the pass fails saying so. Nothing is deleted either unless at least min_etcd_voters Ready nodes carrying the etcd role label would remain (a non-voting agent does not count), so a cluster with fewer voters than quorum needs, or with none, is left alone and the pass fails saying so. An unlabelled node, a node without a Ready condition and the node this pass runs on (NODE_RECOVERY_SELF_NODE) are never deleted. The delete is one API request carrying the node's uid and resourceVersion as preconditions, which kubectl delete cannot send, so a node that changed after it was read (for example because it came back) is left for the next pass. This pass then needs delete on nodes as well as curl, which reaches the API with the pod's ServiceAccount credentials (NODE_RECOVERY_API_URL and NODE_RECOVERY_SERVICEACCOUNT_DIR override where it looks). set -euo pipefail @@ -13,6 +13,10 @@ annotation="${NODE_RECOVERY_ANNOTATION:-github-runner.exadev/out-of-service-appl self_node="${NODE_RECOVERY_SELF_NODE:-}" prune_after="${NODE_RECOVERY_PRUNE_AFTER_SECONDS:-}" prune_label="${NODE_RECOVERY_PRUNE_LABEL:-github-runner.exadev.io/removable}" +lease_namespace="${NODE_RECOVERY_LEASE_NAMESPACE:-kube-system}" +lease_name="${NODE_RECOVERY_LEASE_NAME:-fleet-regrade}" +swap_nodes_annotation="${NODE_RECOVERY_SWAP_NODES_ANNOTATION:-github-runner.exadev.com/swap-nodes}" +level_label="${NODE_RECOVERY_LEVEL_LABEL:-github-runner.exadev.com/regrade-level}" # A cluster needs three voting etcd members to tolerate losing one: with two, losing either leaves one of two, which is not a majority. k3s removes a server's etcd member when its Node is deleted, so the prune pass deletes nothing unless at least this many Ready etcd members would remain. readonly min_etcd_voters=3 taint_key="node.kubernetes.io/out-of-service" @@ -38,6 +42,8 @@ fi log() { echo "[$(date -u +%Y-%m-%dT%H:%M:%SZ)] $*"; } nodes="$(kubectl get nodes -o json)" +# Empty when there is no such Lease. Any other failure (the API unreachable, the Role missing) stops the pass here. +lease="$(kubectl get lease "$lease_name" --namespace "$lease_namespace" --output json --ignore-not-found)" now="$(date -u +%Y-%m-%dT%H:%M:%SZ)" # One JSON object per node that needs a change: its name, the action, how long it has been Unknown, and the patch to send (or, for a delete, the uid and resourceVersion to send as preconditions). @@ -46,6 +52,9 @@ plan="$(jq -c \ --argjson min_voters "$min_etcd_voters" \ --arg prune_after "$prune_after" \ --arg prune_label "$prune_label" \ + --arg level_label "$level_label" \ + --arg swap_annotation "$swap_nodes_annotation" \ + --argjson lease "${lease:-null}" \ --arg annotation "$annotation" \ --arg self "$self_node" \ --arg key "$taint_key" \ @@ -53,6 +62,16 @@ plan="$(jq -c \ --arg now "$now" ' ($now | fromdateiso8601) as $now_s | (if $prune_after == "" then null else ($prune_after | tonumber) end) as $prune_s + # The nodes the Lease protects: none once it has expired (its renew time plus its duration, the rule of lease_state in roles/github_runner_cluster/files/regrade/fleet-regrade-core.sh) or when nobody holds it, otherwise the nodes its annotation lists and its holder, which is a node name when regrading holds it and a name no node has when the role controller does. + | (if $lease == null then [] + else ($lease.spec.holderIdentity // "") as $holder + | ($lease.spec.renewTime // $lease.spec.acquireTime // null) as $renewed + | if $holder == "" or $renewed == null + or (($renewed | sub("\\.[0-9]+Z$"; "Z") | fromdateiso8601) + ($lease.spec.leaseDurationSeconds // 0)) <= $now_s + then [] + else [$holder] + (($lease.metadata.annotations // {})[$swap_annotation] // "" | split(",") | map(gsub("^\\s+|\\s+$"; "") | select(length > 0))) + end + end) as $swapping | [.items[] | select((.metadata.labels // {}) | has("node-role.kubernetes.io/etcd")) | select(((.status.conditions // []) | map(select(.type == "Ready")) | first).status == "True") @@ -66,9 +85,11 @@ plan="$(jq -c \ | ($annotations | has($annotation)) as $ours | (.metadata.labels // {}) as $labels | ($labels | has("node-role.kubernetes.io/control-plane") or has("node-role.kubernetes.io/etcd")) as $server + | ($swapping | index($name) != null) as $in_swap + | ($labels[$level_label] // null) as $level | {op: "test", path: "/metadata/resourceVersion", value: .metadata.resourceVersion} as $test | if $name == $self or $ready == null then empty - elif $prune_s != null and $ready.status == "Unknown" and $labels[$prune_label] == "true" + elif $prune_s != null and $ready.status == "Unknown" and $labels[$prune_label] == "true" and ($in_swap | not) and ($now_s - ($ready.lastTransitionTime | fromdateiso8601)) >= $prune_s then { name: $name, @@ -80,7 +101,9 @@ plan="$(jq -c \ } elif $ready.status == "Unknown" and ($tainted | not) and ($now_s - ($ready.lastTransitionTime | fromdateiso8601)) >= $after then - { + if $in_swap then {name: $name, action: "hold", reason: "the Lease \($lease.metadata.name) names it for a swap or a regrade step"} + elif $level != null then {name: $name, action: "hold", reason: "\($level_label) holds it at \($level)"} + else { name: $name, action: "taint", unknown_for: ($now_s - ($ready.lastTransitionTime | fromdateiso8601)), @@ -90,6 +113,7 @@ plan="$(jq -c \ {op: "add", path: "/spec/taints", value: ($taints + [{key: $key, value: $value, effect: "NoExecute", timeAdded: $now}])} ] } + end elif $ready.status == "True" and $ours then { name: $name, @@ -152,6 +176,10 @@ while IFS= read -r change; do [ -n "$change" ] || continue name="$(jq -r '.name' <<< "$change")" action="$(jq -r '.action' <<< "$change")" + if [ "$action" = hold ]; then + log "Not tainting ${name}: $(jq -r '.reason' <<< "$change")" + continue + fi if [ "$action" = refuse ]; then log "Not deleting ${name}: it carries ${prune_label}=true but is a control-plane or etcd member, and deleting a server's Node removes its etcd member; remove the label" >&2 exit_code=1 diff --git a/tests/unit/test_node_recovery.py b/tests/unit/test_node_recovery.py index a55cceea..4a8ae424 100644 --- a/tests/unit/test_node_recovery.py +++ b/tests/unit/test_node_recovery.py @@ -25,7 +25,7 @@ # What a cluster needs to tolerate losing one voter; the script's own constant is min_etcd_voters. QUORUM_VOTERS = 3 -# Answers `kubectl get nodes -o json` from a file and records every `kubectl patch`; FAKE_PATCH_EXIT sets the patch's exit status. +# Answers `kubectl get nodes -o json` from a file, answers `kubectl get lease` with the Lease in FAKE_LEASE (nothing, as --ignore-not-found does, when there is no such file; FAKE_LEASE_EXIT makes it fail instead) and records every `kubectl patch`; FAKE_PATCH_EXIT sets the patch's exit status. FAKE_KUBECTL = textwrap.dedent( """\ #!/usr/bin/env python3 @@ -34,6 +34,15 @@ if args[:2] == ["get", "nodes"]: sys.stdout.write(open(os.environ["FAKE_NODES"]).read()) sys.exit(0) + if args[:2] == ["get", "lease"]: + with open(os.environ["FAKE_LEASE_CALLS"], "a") as log: + log.write(json.dumps(args) + "\\n") + if int(os.environ.get("FAKE_LEASE_EXIT", "0")): + sys.stderr.write("lease unreadable\\n") + sys.exit(int(os.environ["FAKE_LEASE_EXIT"])) + if os.path.exists(os.environ["FAKE_LEASE"]): + sys.stdout.write(open(os.environ["FAKE_LEASE"]).read()) + sys.exit(0) if args[:2] == ["patch", "node"]: with open(os.environ["FAKE_PATCHES"], "a") as log: log.write(json.dumps({"name": args[2], "argv": args}) + "\\n") @@ -91,6 +100,22 @@ def node( } +LEASE_NAMESPACE = "kube-system" +LEASE_NAME = "fleet-regrade" +SWAP_NODES_ANNOTATION = "github-runner.exadev.com/swap-nodes" +LEVEL_LABEL = "github-runner.exadev.com/regrade-level" +LEASE_DURATION_SECONDS = 900 + + +def lease(holder: str, renewed_seconds_ago: int, swap_nodes: str | None = None, duration: int = LEASE_DURATION_SECONDS) -> dict[str, object]: + """The Lease as the API server stores it, renewed the given time ago.""" + renewed = (datetime.now(timezone.utc) - timedelta(seconds=renewed_seconds_ago)).strftime("%Y-%m-%dT%H:%M:%S.123456Z") + metadata: dict[str, object] = {"name": LEASE_NAME, "namespace": LEASE_NAMESPACE} + if swap_nodes is not None: + metadata["annotations"] = {SWAP_NODES_ANNOTATION: swap_nodes} + return {"apiVersion": "coordination.k8s.io/v1", "kind": "Lease", "metadata": metadata, "spec": {"holderIdentity": holder, "leaseDurationSeconds": duration, "acquireTime": renewed, "renewTime": renewed}} + + UNREACHABLE = {"key": "node.kubernetes.io/unreachable", "effect": "NoExecute"} OURS = {"key": OUT_OF_SERVICE, "value": "nodeshutdown", "effect": "NoExecute"} @@ -119,6 +144,8 @@ def setUp(self) -> None: self.patches_file = self.work / "patches.jsonl" self.current_file = self.work / "current.json" self.deletes_file = self.work / "deletes.jsonl" + self.lease_file = self.work / "lease.json" + self.lease_calls_file = self.work / "lease-calls.jsonl" self.sa_dir = self.work / "serviceaccount" self.sa_dir.mkdir() (self.sa_dir / "token").write_text(TOKEN) @@ -130,6 +157,8 @@ def setUp(self) -> None: "FAKE_PATCHES": str(self.patches_file), "FAKE_CURRENT": str(self.current_file), "FAKE_DELETES": str(self.deletes_file), + "FAKE_LEASE": str(self.lease_file), + "FAKE_LEASE_CALLS": str(self.lease_calls_file), "NODE_RECOVERY_API_URL": "https://api.example.test:6443", "NODE_RECOVERY_SERVICEACCOUNT_DIR": str(self.sa_dir), "NODE_RECOVERY_AFTER_SECONDS": str(AFTER_SECONDS), @@ -138,10 +167,16 @@ def setUp(self) -> None: } def run_pass( - self, nodes: list[dict[str, object]], current: list[dict[str, object]] | None = None, **env: str + self, + nodes: list[dict[str, object]], + current: list[dict[str, object]] | None = None, + held: dict[str, object] | None = None, + **env: str, ) -> tuple[subprocess.CompletedProcess[str], list[dict[str, object]]]: - """Runs one pass over the nodes as listed. The API's own view when the delete arrives is `current`, the same nodes unless given.""" + """Runs one pass over the nodes as listed. The API's own view when the delete arrives is `current`, the same nodes unless given; `held` is the regrade Lease in the cluster, none unless given.""" self.nodes_file.write_text(json.dumps({"items": nodes})) + if held is not None: + self.lease_file.write_text(json.dumps(held)) live = nodes if current is None else current self.current_file.write_text(json.dumps({n["metadata"]["name"]: n["metadata"] for n in live})) result = subprocess.run(["bash", str(SCRIPT)], env={**self.env, **env}, capture_output=True, text=True, check=False) @@ -254,6 +289,80 @@ def test_rejects_a_threshold_that_is_not_a_number(self) -> None: self.assertEqual(patches, []) +class RoleSwapTest(PassCase): + """The nodes a voter swap or regrading is moving through, which the watcher leaves alone: the Lease kube-system/fleet-regrade names them while it is unexpired, and regrading marks a server it has stepped down with a level label.""" + + DEAD = AFTER_SECONDS * 10 + + def silent(self, name: str, **kwargs: object) -> dict[str, object]: + """A node whose kubelet has been silent far longer than the taint threshold.""" + return node(name, "Unknown", self.DEAD, **kwargs) # type: ignore[arg-type] + + def tainted(self, patches: list[dict[str, object]]) -> list[str]: + return [p["name"] for p in patches] # type: ignore[misc] + + def test_leaves_alone_the_nodes_a_held_lease_lists(self) -> None: + result, patches = self.run_pass([self.silent("outgoing"), self.silent("incoming"), self.silent("other")], held=lease("fleet-role", 30, swap_nodes="outgoing,incoming")) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.tainted(patches), ["other"]) + self.assertIn("Not tainting outgoing", result.stdout) + self.assertIn("Not tainting incoming", result.stdout) + + def test_reads_only_the_named_lease_and_trims_the_names_it_lists(self) -> None: + _, patches = self.run_pass([self.silent("outgoing"), self.silent("incoming")], held=lease("fleet-role", 30, swap_nodes=" outgoing , ,incoming ")) + self.assertEqual(patches, []) + [call] = [json.loads(line) for line in self.lease_calls_file.read_text().splitlines()] + self.assertEqual(call[:3], ["get", "lease", LEASE_NAME]) + self.assertEqual(call[call.index("--namespace") + 1], LEASE_NAMESPACE) + + def test_acts_on_a_listed_node_once_the_lease_has_expired(self) -> None: + expired = lease("fleet-role", LEASE_DURATION_SECONDS + 30, swap_nodes="outgoing") + result, patches = self.run_pass([self.silent("outgoing")], held=expired) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.tainted(patches), ["outgoing"]) + + def test_acts_on_a_listed_node_when_the_lease_has_no_holder(self) -> None: + _, patches = self.run_pass([self.silent("outgoing")], held=lease("", 30, swap_nodes="outgoing")) + self.assertEqual(self.tainted(patches), ["outgoing"]) + + def test_leaves_alone_the_node_a_regrade_holder_names(self) -> None: + result, patches = self.run_pass([self.silent("stepping"), self.silent("other")], held=lease("stepping", 30)) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.tainted(patches), ["other"]) + + def test_leaves_alone_a_server_regrading_has_stepped_down(self) -> None: + result, patches = self.run_pass([self.silent("etcd-only", labels={LEVEL_LABEL: "etcd-only", ETCD: "true"}), self.silent("other")]) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.tainted(patches), ["other"]) + self.assertIn(f"Not tainting etcd-only: {LEVEL_LABEL} holds it at etcd-only", result.stdout) + + def test_the_level_label_is_configurable(self) -> None: + _, patches = self.run_pass([self.silent("stepped", labels={"example.com/level": "etcd-only"})], NODE_RECOVERY_LEVEL_LABEL="example.com/level") + self.assertEqual(patches, []) + + def test_still_clears_its_taint_from_a_listed_node_that_is_ready(self) -> None: + back = node("outgoing", "True", 5, taints=[OURS], annotations={ANNOTATION: timestamp(600)}) + result, patches = self.run_pass([back], held=lease("fleet-role", 30, swap_nodes="outgoing")) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.tainted(patches), ["outgoing"]) + self.assertEqual(self.value_at(patches[0]["patch"], "/spec/taints"), []) # type: ignore[arg-type] + + def test_a_lease_that_cannot_be_read_fails_the_pass_before_any_change(self) -> None: + result, patches = self.run_pass([self.silent("outgoing")], FAKE_LEASE_EXIT="1") + self.assertNotEqual(result.returncode, 0) + self.assertEqual(patches, []) + + def test_does_not_delete_a_listed_node_that_would_be_pruned(self) -> None: + env = {"NODE_RECOVERY_PRUNE_AFTER_SECONDS": str(PRUNE_AFTER_SECONDS), "NODE_RECOVERY_PRUNE_LABEL": PRUNE_LABEL} + voters = [node(f"server-{i}", "True", 3600, labels={CONTROL_PLANE: "true", ETCD: "true"}) for i in range(QUORUM_VOTERS)] + outgoing = node("outgoing", "Unknown", PRUNE_AFTER_SECONDS * 10, labels=REMOVABLE) + other = node("other", "Unknown", PRUNE_AFTER_SECONDS * 10, labels=REMOVABLE) + result, patches = self.run_pass([outgoing, other, *voters], held=lease("fleet-role", 30, swap_nodes="outgoing"), **env) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.deleted(), ["other"]) + self.assertEqual(patches, []) + + class NodePruneTest(PassCase): """The opt-in prune pass, which deletes a labelled node that stayed Unknown for NODE_RECOVERY_PRUNE_AFTER_SECONDS."""