-
Notifications
You must be signed in to change notification settings - Fork 2.9k
use bootc for os apply #27875
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
use bootc for os apply #27875
Conversation
| OCI image. | ||
| ``` | ||
| $ podman machine os apply quay.io/podman/machine-os:5.4 | ||
| $ podman machine os apply quay.io/podman/machine-os:6.1 |
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.
why do we point the docs to a version that does not yet even exists?
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.
good question ... it was a funny position . i didnt want to say 5.8 to 6.0 to set a precedent that it would work, because i am not sure it would. I didn't really want to talk about this in the past, because well ... things have changed.
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 also, i realized, this will need ta good scrubbing once upgrade is in place and the transports were added in so we have another opportunity to clean this up.
| if exists { | ||
| fmt.Println("Pulling from", "containers-storage"+":", imageWithTransport) | ||
| dir, err := os.MkdirTemp("", pathSafeString(imageWithTransport)) |
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 to be clear to we no longer plan to support loading from a local image or do you want to add this back later?
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.
that would be later ... i want to get upgrade in so we have a full solution. then we need to figure out how to add transports to this command. bootc doesn't use URIs, it uses --transport instead. so the question is do we allow the URIs to continue and then do a split on it OR do we mimic bootc and add a command line option? The latter is cleaner and ensures we don't have confusion of transport URI names.
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.
good question, I don't have a strong opinion. Structurally speaking podman is all about using these transport URI strings for things like pull, push, etc... so it could be weird why only one command does not allow it.
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.
Structurally speaking podman is all about using these transport URI strings for things like pull, push, etc... so it could be weird why only one command does not allow it.
+1
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.
initially because i thought the transports would be sufficiently different and that in itself is not good. i can do a comparison once im ready to do that work. i def want to be able to pull it from containers-storage so it will not be forgotten.
pkg/machine/os/ostree.go
Outdated
| } | ||
|
|
||
| ostreeCli := []string{"rpm-ostree", "--bypass-driver", "rebase", fmt.Sprintf("ostree-unverified-image:%s", imageWithTransport)} | ||
| ostreeCli := []string{"bootc", "switch", image} |
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.
oh and just noticed if you repush might as well rename the variable
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.
done
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.
Tested on arm64 windows using HyperV
| args := []string{"podman", "machine", "os", "apply", image} | ||
|
|
||
| if err := machine.LocalhostSSH(m.VM.SSH.RemoteUsername, m.VM.SSH.IdentityPath, m.VMName, m.VM.SSH.Port, args); err != nil { | ||
| if err := machine.LocalhostSSHShellForceTerm(m.VM.SSH.RemoteUsername, m.VM.SSH.IdentityPath, m.VMName, m.VM.SSH.Port, args); err != nil { |
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.
Is this change needed for the bootc command to work or is it a refactoring?
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.
that change specifically is so the bootc status(pulling, applying) can be seen by the user; without the use of the ssh terminal, the user would never see it.
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.
LGTM, I'm okay with holding off on transport support on a later PR.
Instead of using rpm-ostree, we now use bootc for os apply. the implementation is a little murky right now and will require some cleanup to implement bootc's transports. for now, we only support oci images from registries. once we have an upgrade command, the transports can be added and the docs for apply can be ammended to be more clear. Fixes: RUN-3836 Signed-off-by: Brent Baude <bbaude@redhat.com>
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| ``` | ||
|
|
||
| Update the specified Podman machine to latest Podman 5.3 bootable OCI image. | ||
| Update the specified Podman machine to latest Podman 6.1 bootable OCI image. |
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.
| Update the specified Podman machine to latest Podman 6.1 bootable OCI image. | |
| Update the specified Podman machine to the latest Podman 6.1 bootable OCI image. |
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.
If you take the note suggestion, drop 6.1 from this line too.
| than the client. Unexpected results may occur. | ||
|
|
||
| Update the default Podman machine to the most recent Podman 5.4 bootable | ||
| Update the default Podman machine to the most recent Podman 6.1 bootable |
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.
| Update the default Podman machine to the most recent Podman 6.1 bootable | |
| **NOTE:** The functionality to rebase itself to the content of an OCI image is only available starting with Podman 6.1. | |
| Update the default Podman machine to the most recent Podman bootable |
Feel free to tweak the note line if you think it can be worded better.
|
@TomSweeneyRedHat i created RUN-3963 to circle back and clean up things once os-upgrade is done and the story is clearer. |
Luap99
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.
LGTM
Instead of using rpm-ostree, we now use bootc for os apply. the implementation is a little murky right now and will require some cleanup to implement bootc's transports. for now, we only support oci images from registries.
once we have an upgrade command, the transports can be added and the docs for apply can be ammended to be more clear.
Fixes: RUN-3836
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?