hyperv: fix machine rm -r #22140
hyperv: fix machine rm -r #22140
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/cherry-pick v5.0 |
|
@baude: once the present PR merges, I will cherry-pick it on top of v5.0 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
1 similar comment
|
Ephemeral COPR build failed. @containers/packit-build please check. |
this pr fixes two hyperv bugs. previous podman 5 versions of hyperv failed to actually remove the vm from hyperv when machine rm -f was called. also fixes an annoying bug where removal of the hyperv ignition entries were failing because this can only be done (with the current api) when the vm is running. new api in latest libhvee fixes this. Signed-off-by: Brent Baude <[email protected]>
|
Cockpit tests failed for commit a6ffb56. @martinpitt, @jelly, @mvollmer please check. |
| // because the vm is down at this point, we cannot query hyperv for these key value pairs. | ||
| // therefore we blindly iterate from 0-50 and delete the key/value pairs. hyperv does not | ||
| // raise an error if the key is not present | ||
| // | ||
| for i := 0; i < 50; i++ { | ||
| // this is a well known "key" defined in libhvee and is the vm name | ||
| // plus an index starting at 0 | ||
| key := fmt.Sprintf("%s%d", vm.ElementName, i) | ||
| if err := vm.RemoveKeyValuePairNoWait(key); err != nil { |
There was a problem hiding this comment.
Ok not like I undersatnd the windows code so consider this non blocking.
First where does this 50 came from? How do we know this is enough or wont change?
Second why is it our task to remove hyper registry keys here, I would think this should be done by the vm.Remove() call in libhvee?
There was a problem hiding this comment.
the 50 is related to the number of 1023 byte chunks needed to describe the ignition file. windows registry values can only be 1023 bytes long. So, we had to pick a number because you cannot query the vm for how many keys it has when the vm is down ... that is why we had the error before
as for libhvee, it was meant to be a generic library, not something strictly for podman ... i would not object to long term having this code live in there and it just be an API call. But for now, and 5.0.1 plans, this fixes things and puts it behind us.
|
LGTM |
|
/lgtm |
openshift-merge-bot
bot
merged commit 5c39ddc
into
containers:main
|
@baude: #22140 failed to apply on top of branch "v5.0": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
stale-locking-app
bot
added
the
locked - please file new issue/PR
this pr fixes two hyperv bugs. previous podman 5 versions of hyperv failed to actually remove the vm from hyperv when machine rm -f was called.
also fixes an annoying bug where removal of the hyperv ignition entries were failing because this can only be done (with the current api) when the vm is running. new api in latest libhvee fixes this.
Does this PR introduce a user-facing change?