-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add podman machine os upgrade command #27886
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Note to reviewers: We have absolutely no way to test this command at this point. |
6f5eb5b to
26394a4
Compare
|
Cockpit tests failed for commit 26394a4. @martinpitt, @jelly, @mvollmer please check. |
26394a4 to
63f99c3
Compare
63f99c3 to
11c00be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested manually on linux:
- Init using old image
- First machine upgrade succeeds
- after a reboot, a secondary upgrade fails with
Error: failed to parse ostree origin - Is this the preexisting issue with the new 6.1 OS not yet on quay? But plain init doesn't have output, so I'd assume it should be the same
- plain
podman machine init
- no output, since it's on latest.
- forced client lower version (client 6.0, machine-os 6.1
- Error: client version 6.1.0 is older than your machine version 6.0.0-dev: upgrade your client
- This message should be flipped, ie Error: client version 6.0.0-dev is older than your machine version 6.1.0 : upgrade your client
I believe these are the larger test cases, but if you need anything more specific.
28dc900 to
ea43274
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
c117a54 to
3460fa0
Compare
|
Tested again, all comments were addressed and functionality LGTM. |
|
LGTM |
l0rd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upgrade logic is perfect. I have a few comments on minor things that may be considered as post-merge follow up:
- We should mention that WSL isn't supported in the doc
- The command documentation could be improved (see comments below)
- The changes to
go.modshouldn't be needed - Do you know why the bloat approved label is needed?
- We should fail earlier if the provider is WSL
- We should fail earlier if the provider is Hyper-V and the user is trying to restart without having the privileges to restart (not running as admin, not member of hyper-v admin group)
- There are no machine tests and I understand that's not possible in this PR but will they be added later? Eventually uncommenting the tests for
os apply commandtoo? - Rather than having two separate commands (
machine os applyandmachine os upgrade) I find it simpler to have one unique command (machine update). The unique command would have the functionalities of bothapply(specify a specific image) and upgrade (get the latest image if none is specified). I have checked a few packages managers (dnf, brew, winget) and they usually allow to specify a version in theirupgrade/updatecommand.
| var upgradeCmd = &cobra.Command{ | ||
| Use: "upgrade [options] [MACHINE]", | ||
| Short: "Upgrade machine os", | ||
| Long: "Upgrade the machine operating system to a newer version", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this description, if I have an old CLI, I may still think that the machine will be updated. We can probably add another sentence that says that the new version will match the client major and minor version. And also I would find it useful to mention that the command machine os apply can update to a specific version.
|
|
||
| var upgradeCmd = &cobra.Command{ | ||
| Use: "upgrade [options] [MACHINE]", | ||
| Short: "Upgrade machine os", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For apply we have: "Apply an OCI image to Podman Machine's OS". Compared to that, the cases of machine and os are inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected.
cmd/podman/machine/os/upgrade.go
Outdated
| ) | ||
|
|
||
| var upgradeCmd = &cobra.Command{ | ||
| Use: "upgrade [options] [MACHINE]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For apply this is apply [options] IMAGE [NAME] where [NAME] is the corresponding machine name. Here it's called [MACHINE].
cmd/podman/machine/os/upgrade.go
Outdated
| flags.StringVarP(&opts.format, formatFlagName, "f", "", "suppress output except for specified format. Implies -n") | ||
| _ = upgradeCmd.RegisterFlagCompletionFunc(formatFlagName, completion.AutocompleteNone) | ||
| restartFlagName := "restart" | ||
| flags.BoolVar(&opts.restart, restartFlagName, false, "Restart after upgrade") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For apply the help is Restart VM to apply changes, for consistency here we could use Restart VM to upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure!
3460fa0 to
ca512c6
Compare
mtrmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a drive-by, the dependency changes caught my eye. I didn’t read ~anything else.
mtrmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A ~full review now, with the disclaimer that I never tried running it or studied the bootc CLI.
| Check if an update is available but the response will be in JSON format. Note this | ||
| exposes a boolean field that indicates if an update is available. | ||
|
|
||
| Note: using a format option implies a dry-run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not documented in the option’s primary paragraph, and I think it is critical to include.
(I’m also not sure it is a good idea — will we never have users trying to run the actual upgrade in a script, and needing a structured output?
Or are users wanting to trigger an actual upgrade expected to use the API directly? In that case it’s a bit curious — but still plausible — that the “check for upgrades” users are given a CLI with JSON output.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea here is for consumers to be able to "check" if there is an upgrade (consider PD). And then the actual application of the update is done with the value there. That said, I'm asking PD to try this once it merges and they can provide feedback where we might make a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, i missed adding dry-run to the format description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then the actual application of the update is done with the value there.
… for GUIs, note that things (in particular registry contents) can change between the “check” and the final update.
I suppose such callers would use os upgrade --format json to make an upgrade decision, and then os apply to actually upgrade to exactly that version — does that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, this is simply a mechanism to tell users that an update is available.
| } | ||
|
|
||
| args = []string{"bootc", "upgrade"} | ||
| default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of machineInfo is left not updated — is it worth documenting which fields are set when?
Arguably letting users experiment might work fine … but they will only see the cross-version upgrade about two times a year, so that might be a surprise.
Implements automatic OS upgrade functionality for Podman machines that requires no user input beyond running the command. The upgrade logic automatically determines the appropriate upgrade path using a three-way comparison between client version, machine version, and OCI registry: * When the client version is older than the machine version, no action is taken and an error is returned. * When the client version matches the machine version, the OCI registry is queried to check for in-band updates by comparing image digests. This handles minor, patch level, and updates oci image use cases. * When the client version is newer than the machine version, the machine is upgraded to match the client's major.minor version. * No manual image selection or version specification required. The command supports dry-run mode and JSON (only) output format for automation. Signed-off-by: Brent Baude <[email protected]>
ca512c6 to
aba2df7
Compare
|
LGTM on my end. I think all comments are addressed? |
Implements automatic OS upgrade functionality for Podman machines that requires no user input beyond running the command. The upgrade logic automatically determines the appropriate upgrade path using a three-way comparison between client version, machine version, and OCI registry:
The command supports dry-run mode and JSON (only) output format for automation.
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?