Repository navigation
CKS: reconcile node capacity after live resize - #14223
andrijapanicsb wants to merge 12 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14223 +/- ##
============================================
+ Coverage 20.45% 20.70% +0.25%
- Complexity 21415 21841 +426
============================================
Files 6487 6495 +8
Lines 589303 590990 +1687
Branches 72245 72509 +264
============================================
+ Hits 120525 122368 +1843
+ Misses 455643 455299 -344
- Partials 13135 13323 +188
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:
|
andrijapanicsb
changed the title
|
@ACSHomeBot package G |
Packaging results:
Test packages are available at: |
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.82% |
| Branch coverage | 20.20% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
40b50b0 to
7e5a95a
Compare
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Queued for worker G. Jobs ahead: 0. |
|
Package building started on worker G. |
Packaging results:
Test packages are available at: |
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Queued for worker G. Jobs ahead: 0. |
|
Package building started on worker G. |
Packaging results:
Test packages are available at: |
andrijapanicsb
changed the title
|
@blueorangutan package |
|
@andrijapanicsb a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19660 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
| } | ||
|
|
||
| private void scaleKubernetesClusterOffering(KubernetesClusterNodeType nodeType, ServiceOffering serviceOffering, | ||
| protected void scaleKubernetesClusterOffering(KubernetesClusterNodeType nodeType, ServiceOffering serviceOffering, |
There was a problem hiding this comment.
@andrijapanicsb , please deal with the sonar warning here and revert the catch all further on in the method. I'd suggest you instruct your AI to use 'clean coding principles' as standard skill.
There was a problem hiding this comment.
I dont see any sonar warnings, but let me address your concerns
|
The technical findings are fair, and I’ve addressed them. The remark about instructing “my AI”, however, is unnecessary. Please keep the review focused on concrete issues in the code. I’m contributing here as an operator because the existing live-resize path could successfully resize a VM while Kubernetes continued to advertise the old resources. Whatever the original implementation’s scope, operators still had to complete the Kubernetes-side work manually. A successful VM resize is not, by itself, a successful Kubernetes resize. There is some irony in focusing on a non-developer’s use of AI when that contribution is closing this end-to-end gap. Clean coding principles are a fair requirement; complete behaviour and end-to-end testing deserve the same scrutiny. Please judge the implementation and its verification—not the contributor’s background or authoring tools. |
|
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Completed. Packaging results. |
Packaging results:
Test packages are available at: |
|
[SF] Trillian test result (tid-17260)
|
Pearl1594
left a comment
Pearl1594
left a comment
There was a problem hiding this comment.
This is a good improvement. Thanks @andrijapanicsb . I've provided some comments. Also, there might be a little clash with #14138
|
|
||
| private boolean isReady(JsonObject status) { | ||
| JsonArray conditions = status.getAsJsonArray("conditions"); | ||
| for (JsonElement condition : conditions) { JsonObject item = condition.getAsJsonObject(); if ("Ready".equals(item.get("type").getAsString()) && "True".equals(item.get("status").getAsString())) return true; } |
There was a problem hiding this comment.
new lines - for better readability
| return new NodeCapacitySnapshot(spec.has("unschedulable") && spec.get("unschedulable").getAsBoolean(), cksCordon, | ||
| guest == null ? 0 : guest.cpu, guest == null ? 0 : guest.memoryKiB, parseCpuMillis(capacity.get("cpu").getAsString()), | ||
| parseMemoryBytes(capacity.get("memory").getAsString()), parseCpuMillis(allocatable.get("cpu").getAsString()), | ||
| parseMemoryBytes(allocatable.get("memory").getAsString()), isReady(status)); |
There was a problem hiding this comment.
| return new NodeCapacitySnapshot(spec.has("unschedulable") && spec.get("unschedulable").getAsBoolean(), cksCordon, | |
| guest == null ? 0 : guest.cpu, guest == null ? 0 : guest.memoryKiB, parseCpuMillis(capacity.get("cpu").getAsString()), | |
| parseMemoryBytes(capacity.get("memory").getAsString()), parseCpuMillis(allocatable.get("cpu").getAsString()), | |
| parseMemoryBytes(allocatable.get("memory").getAsString()), isReady(status)); | |
| return new NodeCapacitySnapshot(spec.has("unschedulable") && spec.get("unschedulable").getAsBoolean(), | |
| cksCordon, | |
| guest == null ? 0 : guest.cpu, | |
| guest == null ? 0 : guest.memoryKiB, | |
| parseCpuMillis(capacity.get("cpu").getAsString()), | |
| parseMemoryBytes(capacity.get("memory").getAsString()), | |
| parseCpuMillis(allocatable.get("cpu").getAsString()), | |
| parseMemoryBytes(allocatable.get("memory").getAsString()), | |
| isReady(status)); |
Something like this for better readability - as it has some expressions and its hard to read. Thanks
| JsonObject node = new JsonParser().parse(executeControl(access, "sudo /opt/bin/kubectl get node " + nodeName(vm) + " -o json")).getAsJsonObject(); | ||
| JsonObject annotations = node.getAsJsonObject("metadata").has("annotations") ? node.getAsJsonObject("metadata").getAsJsonObject("annotations") : null; | ||
| boolean cksCordon = annotations != null && annotations.has(RESIZE_ANNOTATION) && cluster.getUuid().equals(annotations.get(RESIZE_ANNOTATION).getAsString()); | ||
| JsonObject spec = node.has("spec") ? node.getAsJsonObject("spec") : new JsonObject(); JsonObject status = node.getAsJsonObject("status"); |
There was a problem hiding this comment.
new line for status - as sonar has pointed as well
| JsonObject annotations = node.getAsJsonObject("metadata").has("annotations") ? node.getAsJsonObject("metadata").getAsJsonObject("annotations") : null; | ||
| boolean cksCordon = annotations != null && annotations.has(RESIZE_ANNOTATION) && cluster.getUuid().equals(annotations.get(RESIZE_ANNOTATION).getAsString()); | ||
| JsonObject spec = node.has("spec") ? node.getAsJsonObject("spec") : new JsonObject(); JsonObject status = node.getAsJsonObject("status"); | ||
| JsonObject capacity = status.getAsJsonObject("capacity"); JsonObject allocatable = status.getAsJsonObject("allocatable"); |
There was a problem hiding this comment.
same here for allocatable
| } | ||
| return StringUtils.defaultString(result.second()); | ||
| } | ||
| private String nodeName(UserVm vm) { String name = StringUtils.lowerCase(vm.getHostName()); if (!NODE_NAME.matcher(name).matches()) throw new CloudRuntimeException("Invalid Kubernetes node name for live resize"); return name; } |
| String node = nodeName(vm); | ||
| executeControl(access, "sudo /opt/bin/kubectl annotate node " + node + " " + RESIZE_ANNOTATION + "=" + cluster.getUuid() + " --overwrite"); | ||
| executeControl(access, "sudo /opt/bin/kubectl cordon " + node); | ||
| while (System.currentTimeMillis() < deadline) { |
There was a problem hiding this comment.
This loop is repeated five times in this class, in cordonIfNeeded, verifyGuestResources, restartKubelet, waitForKubernetesResources and restoreSchedulability. Each one checks a condition, sleeps and throws a phase-specific failure when the deadline passes. Can we extract one helper, for example pollUntil(deadline, condition, phase, vm, message), and use it in all five?
| @Override | ||
| public boolean isKubernetesResourcesCurrent(NodeCapacitySnapshot snapshot, ServiceOffering target) { | ||
| GuestResources guest = new GuestResources(snapshot.getGuestOnlineCpuCount(), snapshot.getGuestMemoryKiB()); | ||
| return guestMatchesTarget(guest, target) && kubernetesMatchesGuest(snapshot, snapshot); |
There was a problem hiding this comment.
kubernetesMatchesGuest(snapshot, snapshot) compares the snapshot with itself, so its "allocatable grew" checks always pass. Can we fix that? Also, can we rename this method to something like isKubernetesNodeAlreadyResized ?
| return new GuestResources(Long.parseLong(values[0]), Long.parseLong(values[1])); | ||
| } | ||
|
|
||
| private boolean guestMatchesTarget(GuestResources observed, ServiceOffering target) { |
There was a problem hiding this comment.
nit: for the sake of understanding (in future as well) can we add a small comment for guestMatchesTarget and kubernetesMatchesGuest on what it does. Thanks
| } | ||
|
|
||
| private NodeCapacitySnapshot nodeSnapshot(KubernetesCluster cluster, UserVm vm, NodeAccess access, GuestResources guest) { | ||
| JsonObject node = new JsonParser().parse(executeControl(access, "sudo /opt/bin/kubectl get node " + nodeName(vm) + " -o json")).getAsJsonObject(); |
There was a problem hiding this comment.
JsonParser().parse(..) is deprecated, can we use the right one
| } | ||
| return StringUtils.defaultString(result.second()); | ||
| } | ||
| private String nodeName(UserVm vm) { String name = StringUtils.lowerCase(vm.getHostName()); if (!NODE_NAME.matcher(name).matches()) throw new CloudRuntimeException("Invalid Kubernetes node name for live resize"); return name; } |
There was a problem hiding this comment.
nodeName calls StringUtils.lowerCase(vm.getHostName()) and passes the result to the regex. A null host name causes an NPE. Worth adding a null check to make it defensive.
ok, then please keep to clean code principles for future submissions. (This is not personally to you Andrija and stating things like this is very necessary in this project. Also not personally to you. I do not know what AI you use to generate your code, hence "your" AI!) |
Context
#13226 added live compute-offering changes for CKS nodes, building on #12975. The VM could gain CPU/RAM while Kubernetes still reported the old capacity. Operators had to activate guest resources and restart
kubeletmanually. #14213 tracks that gap.Changes
CKS now activates and verifies the added guest CPU/RAM, restarts
kubeletwhen needed, and waits for the Node to report the new capacity and allocatable resources before completing the scale operation.Nodes are handled one at a time without draining resident Pods. An existing operator cordon is preserved. If reconciliation fails after resize, the node stays CKS-cordoned; retry finishes that node before continuing with the others.
QA also exposed a pre-existing cluster CPU/RAM accounting bug, not a regression from this PR. Totals are now calculated from the configured per-node-type offerings and counts, so the API and UI reflect mixed control/worker offerings correctly.
Functional and regression testing
Test environment: Cloud-managed KVM, isolated network, published packages from
f92e52d736b9.VPC/direct-access and multi-control HA were not tested. Dedicated etcd live resize remains unsupported. The separate existing
nodeids-only scale-in issue found during QA is tracked in #14458, with its fix in #14459.UI evidence
Successful scale task and correct aggregate capacity:
Scale request and result after Refresh
Control RAM: 2048 → 3072 MiB; worker unchanged at 3 CPU / 4096 MiB.
After Refresh: 6 CPU / 7168 MiB.