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.

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

Explicitly install both containerd and Docker CLI to handle upgrades #195

Closed
wants to merge 1 commit into from

Conversation

tiborvass
Copy link
Contributor

Signed-off-by: Tibor Vass [email protected]

@tiborvass tiborvass requested a review from thaJeztah October 14, 2020 16:56
@@ -460,7 +456,7 @@ do_install() {
fi
search_command="$pkg_manager list --showduplicates 'docker-ce-cli' | grep '$pkg_pattern' | tail -1 | awk '{print \$2}'"
# It's okay for cli_pkg_version to be blank, since older versions don't support a cli package
cli_pkg_version="$($sh_c "$search_command" | cut -d':' -f 2)"
cli_pkg_version="-$($sh_c "$search_command" | cut -d':' -f 2)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this probably breaks installing older versions

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

SGTM, but not at my computer to test.

@thaJeztah
Copy link
Member

@tianon ptal if you have time <3

@thaJeztah
Copy link
Member

After this, we may be able to address #183 as well

@tianon
Copy link
Contributor

tianon commented Oct 24, 2020

I'm not really in-tune with how folks are using this script, but I know installing old versions used to be really common.

I agree that updating containerd too is good, but I worry that it might be a little risky, for example when we get 1.4 in the repo, will older Docker still be compatible? Should we update Depends on Docker to set an upper bound? (Not sure how well APT handles that, actually)

@seemethere
Copy link
Contributor

Why is this necessary? If users wanted to upgrade their containerd.io version then they'd run a package upgrade

Also if a new install of docker-ce requires a newer version of containerd then it should upgrade through the package manager.

Not entirely sure I see the benefit here.

@thaJeztah
Copy link
Member

for example when we get 1.4 in the repo, will older Docker still be compatible? Should we update Depends on Docker to set an upper bound? (Not sure how well APT handles that, actually)

We discussed that recently, and @cpuguy83 tried that, but unfortunately apt (at least) doesn't support that. Well, it supports restricting what's compatible (Depends containerd.io < 1.4), but when installing, will always install "latest" version of containerd.io, and after that fail (because then it detects the version doesn't match the requirement).

Not sure if there's alternatives to that approach. On the other hand, we discussed if we should forcibly block users from installing newer containerd versions when installing older versions of docker; given that "technically", newer versions should be compatible, we thought it was best to leave "holding back" newer versions as an exercise the the user.

I'm not really in-tune with how folks are using this script,

and

Why is this necessary? If users wanted to upgrade their containerd.io version then they'd run a package upgrade

Yes, that should be the normal way to upgrade docker, containerd (or any package). However, in practice I frequently encounter users using it to upgrade docker as well (or even use it in their dotfiles? 🤷‍♂️ #183 (comment))

The problem with that situation is that (even though a warning is printed and the script is halted for 20 seconds), it "seems" to work (docker is updated), however, users end up with a partial update only (e.g. "engine" updated, but cli not, or containerd not being updated).

but I know installing old versions used to be really common.

Perhaps we need the approach that Rancher took, and have a script for each version ? Not a huge fan from a maintenance perspective thought https://github.com/rancher/install-docker

@tianon
Copy link
Contributor

tianon commented Oct 26, 2020

I mean, I'm still a fan of deprecating the script entirely, which makes the maintenance burden go away completely. :trollface: ❤️

(Really, pushes the burden back onto the docs, which is something needed anyhow.)

@tiborvass
Copy link
Contributor Author

Agree for containerd, but not for docker cli, the CLI is more docker than engine these days. I'll update to not install containerd.io explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants