Follow up on the virtualMachine cpus/memory review - #558
Draft
jandubois wants to merge 5 commits into
Draft
Conversation
The reconciler populated the status of any HostInfo it was handed, so an object created under another name looked as authoritative as the singleton, and it rewrote an unchanged status on every convergence pass. Report memory as a quantity so clients can compare it against spec.virtualMachine.memory without converting units, and publish a zero reading rather than hiding it: zero means detection failed, and the App webhook then enforces no ceiling. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
…sion The cpu default is clamped to the host count while an explicit request above it is rejected, so log the clamp instead of silently changing what was asked for. Warn when host detection reads zero: that leaves the matching ceiling unenforced, so an App can ask for more than the host has. Two comments described paths that admission forecloses. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
Clearing a property records a nil, which a merge patch sends as a null to remove the field. The create path reused that map, so creating the App and clearing a property in one command put a null in the create body, which the schema rejects. Nothing surfaced it, because the mutating webhook overwrites the null first. Copying the map also keeps the create's required-field defaults out of the patch that follows. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
The unit tests cover the template writer and the admission defaults separately, but nothing checked that a value set on the App reaches the template ConfigMap the LimaVM controller consumes. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
jandubois
force-pushed
the
address-vm-review-findings
branch
from
July 14, 2026 20:33
b6a4018 to
94339a3
Compare
environment.md described a Lima template cpu default that no longer exists, cmd_app.md advertised a --cpus shorthand the CLI never grew, and api_app.md omitted virtualMachine. HostInfo had no documentation at all, despite existing to serve the GUI. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
jandubois
force-pushed
the
address-vm-review-findings
branch
from
July 14, 2026 22:06
94339a3 to
c67f1fb
Compare
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
1 participant
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.
Follow-ups to the review of the merged virtualMachine cpus/memory work. Five commits, each self-contained.
The
HostInforeconciler populated the status of any object it was handed, and rewrote an unchanged status on every convergence pass. Its memory is now a quantity, so clients can compare it againstspec.virtualMachine.memorywithout converting units, and a zero reading is published rather than hidden: zero means detection failed, and the App webhook then enforces no ceiling.rdd setput a JSON null in the App create body when a property was cleared as the App was created. The CRD schema rejects it; only the mutating webhook overwriting the null first kept it from surfacing.The cpu clamp is now logged instead of silently changing what was asked for, and two comments that described paths admission forecloses are corrected. The reconciler, the validator boundaries, and the create path all gained the tests they lacked. The design docs described a Lima template default that no longer exists and a
--cpusflag the CLI never grew;HostInfohad no documentation at all.RDD_VM_CPUSis deliberately still parsed per admission request. Moving it intoNewAppDefaulterwould return the error fromRegisterWithManager, but the daemon marks the control plane ready before it registers controllers and only logs a registration failure, so a typo would leave a daemon reporting healthy while running no controllers. That hole already exists (a malformed embeddedk3s-versions.jsonreaches it today) and wants its own fix.