-
Couldn't load subscription status.
- Fork 238
Add Upgrade Functionality #1683
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
|
Build failed. ❌ unit-test FAILURE in 9m 52s |
|
Build failed. ❌ unit-test FAILURE in 5m 22s |
|
Build failed. ❌ unit-test FAILURE in 5m 32s |
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.
Thanks for working on this, @silverhadch !
There are a few different use-cases for handling updates to Toolbx containers. #580 is about a minimal command line interface banner. #1358 is about rebasing an existing container over an image for a new operating system version, which is a variant of rebasing over an image for the same OS version, and they are both related to #873
So, we need to think through how all those different use-cases will fit together. eg., the mechanism to show a CLI banner overlaps with the changes in this pull request. Do you have any ideas for that? If nothing else, it can impact the name of the new command. :)
By the way, the Git commits in the feature branch for this PR needs some fixing, because there's a merge commit within the branch itself.
doc/toolbox-upgrade.1.md
Outdated
| toolbox\-upgrade - Upgrade packages in Toolbx containers | ||
|
|
||
| ## SYNOPSIS | ||
| **toolbox upgrade** [*--all* | *-a*] [*--container* | *-c* *CONTAINER*] |
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.
Wouldn't it be better to be able to toolbox upgrade CONTAINER, without the --container option? It's less typing, and the pattern is already used in the create, enter, rm and rmi commands.
We would still need the --all option.
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
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.
I meant that we shouldn't have the --container at all, and just use the positional argument:
$ toolbox upgrade CONTAINERThe --container is used by the run command, because the command to be run takes up the positional argument; and it's used by the create and enter commands only for backwards compatibility with old POSIX shell implementation and is no longer documented in their manual pages.
@martymichal made this change when writing the Go implementation because it's a lot more natural to type toolbox create CONTAINER or toolbox enter CONTAINER, than toolbox create --container CONTAINER or toolbox enter --container CONTAINER; and in this case it lets you specify multiple containers with toolbox upgrade CONTAINER1 CONTAINER2.
So, let's not introduce a --container option here, and let's just stick to the new style.
src/cmd/upgrade.go
Outdated
| {"command -v apk", "sudo apk update && sudo apk upgrade", "apk"}, | ||
| {"command -v emerge", "sudo emerge --sync && sudo emerge -uDN @world", "emerge"}, | ||
| {"command -v slackpkg", "sudo slackpkg update && sudo slackpkg upgrade-all", "slackpkg"}, | ||
| {"command -v swupd", "sudo swupd update", "swupd"}, |
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.
It will be better to encode the package manager metadata in the image. That way any image that's not built into Toolbx can easily specify its own metadata without requiring changes to the Toolbx code, a new Toolbx release, etc..
One option is to express these through the image's labels. eg., com.github.containers.toolbox.package-manager.update="sudo dnf --assumeyes update".
If the label is missing, we error out by saying that the container doesn't support updates. With passing time, as the newer images propagate, this will happen less and less. If the labels are there, we can directly try the command without the command -v ... step.
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.
Also done
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.
Thanks a lot!
I realized that I should have written toolbx, not toolbox for the name of the label. So: com.github.containers.toolbx.package-manager.update.
I should have checked my copy-paste. My apologies for the trouble!
Labels are annoying to migrate, so let's go with the new name of the project from the beginning.
|
I agree it would be better if we use metadata instead. Will work on this soon. |
5d98d38 to
403527c
Compare
|
Build failed. ❌ unit-test FAILURE in 2m 16s |
|
Build failed. ❌ unit-test FAILURE in 2m 17s |
|
@debarshiray So the Other use cases are out of scope for this PR and will be implemented later. This PR focuses purely on the updating functionality. Planned future PRs:
|
|
Build failed. ✔️ unit-test SUCCESS in 2m 23s |
|
@debarshiray |
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.
I applied the suggestions. Now the upgrade command reads the update command from the container’s labels and no longer requires the --container flag if the container name is passed as a positional argument.
Thanks for updating the pull request, @silverhadch !
Other use cases are out of scope for this PR and will be implemented later. This PR focuses purely on the updating functionality.
Ok. I spent some time thinking about the name of the new command. I agree that upgrade is more appropriate than rebase for the command introduced by this pull request.
It would be nice if you take a look again. Since its slowing down progress in KDE Kontainer since we added a lot of Code for Downstream Toolbox Features which these PRs are trying to upstream.
My apologies. Two things got in the way.
I had to switch focus to understanding the impact of some security issues that got marked as critical and high. They eventually needed a new release, and some downstream paperwork.
I am leaving for a two week vacation tomorrow, which meant that I had a ton of miscellaneous things to wrap up before leaving.
However, I didn't want to leave you hanging for another two weeks. So, while I didn't get the chance to try out and test your PR, I think it's headed in the right direction. I will get it merged once I am back, and then into a release.
In the meantime, some thoughts:
Would you be motivated to write some tests? I suppose writing tests to actually update the container will be difficult until we update all the images, but at least you can start with some tests for the error paths.
You can update the built-in images for Arch Linux and Ubuntu at images/arch and images/ubuntu. /cc @Foxboron @Jmennius
The Fedora and RHEL images are more complicated because they are built elsewhere. We will update them once this PR is ready and merged.
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 09s |
|
@debarshiray |
|
Build failed. ✔️ unit-test SUCCESS in 2m 14s |
|
Build failed. ✔️ unit-test SUCCESS in 2m 55s |
|
@debarshiray |
|
Build failed. ✔️ unit-test SUCCESS in 2m 11s |
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
25d04bd to
61e54a6
Compare
|
Build failed. ✔️ unit-test SUCCESS in 2m 07s |
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.
Thanks for continuing to update your branch, and especially the tests!
Other than some nitpicky details, I have one comment about prompting the user. See below.
|
|
||
| @test "upgrade(Arch): Upgrade Arch container" { | ||
| create_distro_container arch latest arch-toolbox-test | ||
| assert_success |
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 it really necessary to use assert_success here? It's supposed to be used to check the exit code of the command invoked by the run helper.
When we only need to check that a command succeeded, there's no need to use run or check anything, because Bats sets set -e for all tests and the test will automatically fail.
| } | ||
|
|
||
| if !upgradeAll && upgradeContainer == "" { | ||
| return errors.New("must specify either --all or a container name") |
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.
We should show:
Error: missing argument for "upgrade"
Run 'toolbox --help' for usage.
... as we do for the other commands. eg., see src/cmd/rm.go
| toolbox-upgrade - Upgrade packages in Toolbx containers | ||
|
|
||
| ## SYNOPSIS | ||
| **toolbox upgrade** [*--all* | *-a*] [*--container* | *-c* *CONTAINER*] [*CONTAINER*] |
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.
Don't you want to support specifying multiple containers by name:
$ toolbox upgrade CONTAINER1 CONTAINER2The rm and rmi commands support that.
|
|
||
| inspectedcontainer, err := podman.InspectContainer(container) | ||
| if err != nil { | ||
| return errors.New("Failed to inspect Metadata.") |
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 error should be:
fmt.Errorf("failed to inspect container %s", container)
... as is used elsewhere in the code base.
| labels := inspectedcontainer.Labels() | ||
| pkgline := labels["com.github.containers.toolbox.package-manager.update"] | ||
| if pkgline == "" { | ||
| return errors.New("'com.github.containers.toolbox.package-manager.update' Label not set in Containers Metadata.") |
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.
We need an error message that a user, who doesn't know about the internal implementation details, can understand and suggests how to move forward. :)
Something like:
Error: container CONTAINER does not support "upgrade"
Recreate it with a newer image or file a bug.
| "", | ||
| "", | ||
| 0, | ||
| []string{"sh", "-c", pkgline}, |
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.
I was thinking about this over the past few weeks in the context of the NVIDIAScape security issue, and it struck me that someone can trick a user into using a malicious image that contains a harmful command in its c.gh.containers.toolbx.package-manager.update label.
It will be wise to prompt the user with the command that will be run before running it. The user can use the --assumeyes if they want to override the prompt. Look for the askForConfirmation function for an existing example of this.
This makes me wonder if we should have variants of the c.gh.containers.toolbx.package-manager.update label that use an assume yes option for the package manager and don't. It will be bad if a dnf --assumeyes upgrade breaks the container without giving the user a way to opt out of it as usual.
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.
Immediately after I submitted the comment, it struck me that a malicious image can already trick Toolbx into doing bad things.
Such as, the entry point of Toolbx containers rely on the mount(8) binary. So, if there's an image that has replaced that binary with something else, then toolbox enter can lead to bad things.
We could re-implement the parts of mount(8) that Toolbx needs in Go to avoid this, but then someone can replace the bash(1) binary with something else, and so on.
So, I suppose, the security argument doesn't really hold?
Even then, maybe we should offer a prompt and show what's about to happen just for the sake of politeness? Just like we do before downloading an image.
I am still wondering if this means we should have a prompt before the package manager is invoked, or after it's invoked but before the packages are downloaded and changed, or both.
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.
Probably the best approach would be to tell the user the update command then prompt them and if the user wants via a flag skip it but by default they will be told about it.
And to be honest we should probably have a vetted compatibility list like distrobox.
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.
Probably the best approach would be to tell the user the update command then prompt them and if the user wants via a flag skip it but by default they will be told about it.
You mean prompting the user both before and after invoking the package manager? ie., prompting the user that dnf upgrade will be called, then prompting the user to proceed with what dnf upgrade wants to do?
I suspect we could avoid the first prompt and only inform the user that dnf upgrade is going to be run, if we are sure that it won't generate a lot of network traffic. Otherwise, it will be bad for users with limited Internet connectivity.
I am beginning to think that having too many prompts is annoying, and it's better than the alternative of potentially rudely surprising the users by not letting them have a say.
And to be honest we should probably have a vetted compatibility list like distrobox.
How do you want to implement that?
The first step for declaring full support for an OS is to run the CI on hosts with that OS, which isn't a trivial task. Do you want to skip the upgrade command for those that are on the compatibility list?
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.
I think it makes sense to have a confirmation prompt before running the package manager if the command has some kind of --assumeyes argument. If it doesn't, then confirmation can happen after invocation.
| FROM docker.io/library/archlinux:base-devel | ||
|
|
||
| LABEL com.github.containers.toolbox="true" \ | ||
| com.github.containers.toolbox.package-manager.update="sudo pacman -Syu --noconfirm" \ |
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 issue is that this is not going to work that reliably. pacman is going to prompt for some resolutions and that will lock things up. You could run something along the lines of sudo sh -c 'yes | pacman -Syu --noconfirm' but you might end up in a situation where the keyring is sufficiently outdated for this to work.
sudo sh -c 'sudo pacman -Sy archlinux-keyring; yes | pacman -Su --noconfirm'
or similar could work but there is no guarantees as Arch Linux is not built around the assumptions that unattended upgrades are supported.
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.
I see, interesting. Thanks for that insight, @Foxboron
In that case, my vote is to not add the label to the arch-toolbox image, and leave it as explicitly unsupported. It's better to not advertise things to users that are not meant to work.
|
@debarshiray @Foxboron |
toolbox upgrade: Add package manager detection and upgrade capability
This PR introduces a new
toolbox upgradecommand that:Key Features
--container) and all containers (--all)Supported Package Managers
✔️ dnf/microdnf/yum (RHEL/Fedora)
✔️ apt (Debian/Ubuntu)
✔️ pacman (Arch)
✔️ zypper (openSUSE)
✔️ apk (Alpine)
✔️ And 5 others (xbps, emerge, slackpkg, swupd)
Usage Examples
Implementation Details
runCommandinfrastructureThis provides users with a simple, unified way to keep all their toolbox containers updated regardless of distro.