Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

machine: Address some QEMU TODOs #21517

Merged
merged 3 commits into from
Feb 22, 2024
Merged

machine: Address some QEMU TODOs #21517

merged 3 commits into from
Feb 22, 2024

Conversation

jakecorrenti
Copy link
Member

@jakecorrenti jakecorrenti commented Feb 5, 2024 •
edited
Loading

Does this PR introduce a user-facing change?

Addresses the following TODOs that were found in the podman 5 machine refactor code:

  • We no longer care about the image provenance in InspectInfo, so remove the Image field
  • Address whether providerRm failures should be a hard error
  • Address whether a failure to remove machine files should be a hard error
  • Implement QEMU-specific file removal
  • Remove system connections first and remove machine config files last, this allows us to "see" the machine still in podman machine list if there was an error removing any prior files
`ConfigPath` and `Image` are not longer provided to the user when they execute `podman machine inspect`. Users can also no longer use `{{ .ConfigPath }}` or `{{ .Image }}` when doing `podman machine inspect --format`

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 5, 2024
baude
baude reviewed Feb 5, 2024
View reviewed changes
pkg/machine/qemu/machine.go Outdated Show resolved Hide resolved
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 1b0accbfeac56d0903fc5122f968fd0606a69936. @martinpitt, @jelly, @mvollmer please check.

@jakecorrenti jakecorrenti force-pushed the fix-qemu-todos branch 2 times, most recently from 07d068e to 0ff9659 Compare February 6, 2024 13:55
@baude baude force-pushed the machine-dev-5 branch 4 times, most recently from 27cf4ef to 63a4a34 Compare February 6, 2024 18:10
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024
@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024
@baude baude force-pushed the machine-dev-5 branch 3 times, most recently from 34d58dc to 53056fe Compare February 7, 2024 15:19
@jakecorrenti jakecorrenti force-pushed the fix-qemu-todos branch 3 times, most recently from a2d054b to d445228 Compare February 8, 2024 20:12
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2024
Luap99
Luap99 reviewed Feb 13, 2024
View reviewed changes
cmd/podman/machine/rm.go Show resolved Hide resolved
pkg/machine/vmconfigs/machine.go Outdated
@@ -185,28 +185,32 @@ func (mc *MachineConfig) Remove(saveIgnition, saveImage bool) ([]string, func()
}

mcRemove := func() error {
if err := connection.RemoveConnections(mc.Name, mc.Name+"-root"); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we return right away here it will not delete the other things on error which is bad

This here should really use some of the multierror packages that we use and just store all errors and then as last step remove it, i.e. declare a errs []error slice at the top then append all error there instead of returning. Then at the function end return errorhandling.JoinErrors(errs).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that existed, thanks Paul. Should be fixed as well.

Luap99
Luap99 reviewed Feb 13, 2024
View reviewed changes
cmd/podman/machine/rm.go Outdated
if err := providerRm(); err != nil {
logrus.Errorf("failed to remove virtual machine from provider for %q", vmName)
return fmt.Errorf("failed to remove virtual machine from provider for %q: %v", vmName, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the one open question is do we want to call genericRm() even when this failed? I am not yet familiar with the differences between both but my gut tells me yes. So I would recommend to collect/log the errors like done elsewhere.
@baude WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

providerRM deals with provider specific files. So for QEMU we would remove the QMP Monitor pid file, for example. genericRM is where we delete the ignition file, config file, and image. I'm fine with logging the error and then doing a hard error on genericRM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would report the error either way but do both. BTW Ignore ENOTEXIST errors on missing, (You can just logrus.Debug them.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VMFile.Delete() ignores ENOTEXIST when reporting the error so all good on that front

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@TomSweeneyRedHat
Copy link
Member

Changes LGTM
One test still kicking up it's heals.

@baude baude added the 5.0 label Feb 20, 2024
@jakecorrenti
Update machine files rm order and add QEMU rm
d68240b 
Changes the order in which the machine-specific files are removed in
`Remove()`. Removes the system connections first, then removes the
`configPath` last. `configPath` is removed last, because in the case of
an error with any of the previous files, the removal can be attempted
again since the machine still "exists".

Made the errors in `Remove` hard errors instead of soft errors.

Added the implementation for the QEMU-specific file removal.

[NO NEW TESTS NEEDED]

Signed-off-by: Jake Correnti <[email protected]>
@jakecorrenti
Remove image provenance from machine inspect
be9aba7 
We don't care about the provenance of the machine image, so this is no
longer applicable to have when displaying info.

Signed-off-by: Jake Correnti <[email protected]>
@jakecorrenti
Stop using rm'd inspect format templates in tests
d7f7f07 
Fixes the "machine rm --save-ignition --save-image" test so that it no longer
uses the `{{ .Image }}` format string.

Fixes the "init should cleanup on failure" test so that it no longer
uses the `{{ .Image }}` and `{{ .ConfigPath }}` format strings.

Signed-off-by: Jake Correnti <[email protected]>
@jakecorrenti
Copy link
Member Author

@TomSweeneyRedHat PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm
/hold # Until @mheon give it the thumbs up. I'm not sure about timing/packaging at the moment.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2024
@mheon
Copy link
Member

mheon commented Feb 22, 2024

I have no objection.
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 36d8e27 into containers:main Feb 22, 2024
92 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 23, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@baude baude baude left review comments

@rhatdan rhatdan rhatdan left review comments

Assignees

@TomSweeneyRedHat TomSweeneyRedHat

Labels
5.0 approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jakecorrenti @Luap99 @TomSweeneyRedHat @mheon @rhatdan @baude @openshift-merge-robot