remove default "containerd.toml" config file by thaJeztah · Pull Request #215 · docker/containerd-packaging

thaJeztah · 2021-01-21T21:23:46Z

This configuration file was used to disable the CRI plugin to work around an
issue where CRI was automatically listening for remote connections.

That problem was resolved, so we no longer need a customized configuration.

Addresses containerd/containerd#4956

thaJeztah · 2021-01-21T21:24:09Z

@cpuguy83 @oldthreefeng @tiborvass PTAL

cpuguy83

LGTM

oldthreefeng · 2021-01-22T01:02:46Z

LGTM

thaJeztah · 2021-01-22T15:08:49Z

whoops; missed a reference to that file in the rpm spec; pushing a fix now

This configuration file was used to disable the CRI plugin to work around an issue where CRI was automatically listening for remote connections. That problem was resolved, so we no longer need a customized configuration. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2021-01-25T13:20:43Z

 %{_unitdir}/containerd.service
-%{_sysconfdir}/containerd
 %{_mandir}/man*/*
-%config(noreplace) %{_sysconfdir}/containerd/config.toml


This is the only part I was looking at if this could have side-effects (file and directory no longer marked as "config"). I think it should have no consequences (package no longer knows of this path, so won't interfere with it), but if someone knows if this is a concern, let me know.

@thaJeztah removing this does have some impact on rpm users who are currently overwriting the rpm supplied config.toml with their own. This being removed above, causes the user supplied override config.toml file also to be deleted, when they upgrade to a rpm version with this change. rpm moves the file to.rpmsave/.rpmnew after upgrading to the new version with the above change.

Can we just continue shipping an empty config.toml with just version=2 without the disabled cri plugin config ?

@ravilr hmmm... yeah, we could partially revert that change if there's no other way; do you know if rpms allow for the change to be "ignored" (as in: don't touch the existing file?)

according to https://rpm-packaging-guide.github.io/#files, %config(noreplace) exists to protect local modifications to a config file, so that local edits to config files won't get lost during a subsequent upgrade.
But, since the file is being removed from rpm, that doesn't apply here.

IMO, if the intention is to get rid of disabling of cri plugin, then rpm packaging here should continue shipping an empty config.toml with %config(noreplace).

thaJeztah · 2021-01-25T13:21:04Z

Updated; and all green now; @cpuguy83 @tianon PTAL 🤗

tianon · 2021-01-25T21:52:26Z

Nice! 👍

Debian has a conffiles thing that might interfere here (https://www.debian.org/doc/manuals/maint-guide/dother.en.html#conffiles; by default it marks anything installed to /etc as a "conffile" which then needs extras to properly remove/manage).

We probably need a <package>.maintscript file that includes rm_conffile /etc/containerd/config.toml (https://manpages.debian.org/buster/debhelper/dh_installdeb.1.en.html, https://manpages.debian.org/buster/dpkg/dpkg-maintscript-helper.1.en.html).

thaJeztah · 2021-01-26T18:41:11Z

We probably need a .maintscript file that includes rm_conffile /etc/containerd/config.toml (https://manpages.debian.org/buster/debhelper/dh_installdeb.1.en.html, https://manpages.debian.org/buster/dpkg/dpkg-maintscript-helper.1.en.html).

@tianon IIUC, you mean "add a script that removes the config file when installing the new package"? I wonder if we should do that; even though we provided a "default", we also marked the file as noreplace, which (IIUC) means that a user could have added their own custom configuration file there (in which case we wouldn't overwrite it). If we would do a "remove" of that file when installing the package, we would remove their custom config, right?

Or do you mean the "reverse"; prevent the default behavior of .deb from removing the file, now that we no longer list it (and mark it with noreplace ? (that was my initial concern with removing that line)

tianon · 2021-01-28T19:16:38Z

Discussed in maintainers meeting with @thaJeztah @cpuguy83; the config file we were installing is very benign and shouldn't cause problems for any existing users if it doesn't get cleaned up (and the chance of breaking changes to that config file syntax are low), so this is purely a perfectionism / cleanliness concern and should be relatively harmless to ignore.

In other words, we do not need to include rm_conffile and it should be fine to just leave alone and stop adding for new installs.

zhsj · 2021-01-30T13:17:37Z

fwiw, rm_conffile in dpkg-maintscript-helper will not remove conffile if it has been modified by users. dpkg has md5sum of the conffile which is used when removing.

zhsj · 2021-01-30T13:27:06Z

fwiw, rm_conffile in dpkg-maintscript-helper will not remove conffile if it has been modified by users. dpkg has md5sum of the conffile which is used when removing.

ha, seems i misread the above discussion... (just to ignore that)

thaJeztah · 2021-02-16T09:40:00Z

ha, seems i misread the above discussion... (just to ignore that)

No worries; thanks for looking! Yes, we discussed for a bit, and thought it was not worth the extra complexity to clean up the file (which is mostly harmless)

thaJeztah · 2021-02-16T09:40:31Z

@silvin-lubecki @tiborvass ptal; this one good to go?

silvin-lubecki

LGTM

thaJeztah · 2021-03-04T15:10:55Z

@tiborvass ptal

fuweid

LGTM(nb)

thaJeztah · 2021-03-08T11:40:26Z

Given that this fix is not critical, and it's been pending for a while, I think it should be ok to merge this after #223 has been merged and released (keeping the diff for that release small, as it's a security release, so better to not include too many other changes)

kzys · 2021-04-22T18:41:56Z

@thaJeztah #223 has been merged and it has been released as Docker Engine 20.10.6. So I think we can merge this PR now.

thaJeztah · 2021-04-22T19:27:35Z

Ah, yes, let's get this one in 👍

This reverts commit 37e406c. Relates to docker#215 (comment) > removing this does have some impact on rpm users who are currently overwriting > the rpm supplied config.toml with their own. This being removed above, causes > the user supplied override config.toml file also to be deleted, when they upgrade > to a rpm version with this change. rpm moves the file to.rpmsave/.rpmnew after > upgrading to the new version with the above change. Reverting this change (at least temporarily until we figure out an alternative) is the safest option for now. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2021-05-19T12:35:11Z

Opened #233 to (temporarily) revert while we figure out an approach for #215 (comment)

thaJeztah mentioned this pull request Jan 21, 2021

Why not use a real containerd default config with yum to aviod ERROR: runtime failed: rpc error: code = Unimplemented desc = unknown service runtime.v1alpha2.RuntimeService containerd/containerd#4956

Closed

cpuguy83 previously approved these changes Jan 22, 2021

View reviewed changes

thaJeztah dismissed cpuguy83’s stale review via e7bc510 January 22, 2021 15:09

thaJeztah force-pushed the remove_containerd_config branch from d5add94 to e7bc510 Compare January 22, 2021 15:09

thaJeztah force-pushed the remove_containerd_config branch from e7bc510 to 37e406c Compare January 22, 2021 15:24

thaJeztah commented Jan 25, 2021

View reviewed changes

silvin-lubecki approved these changes Feb 17, 2021

View reviewed changes

fuweid approved these changes Mar 5, 2021

View reviewed changes

thaJeztah merged commit e0ff9a3 into docker:master Apr 22, 2021

thaJeztah deleted the remove_containerd_config branch April 22, 2021 19:27

ravilr mentioned this pull request May 14, 2021

prepare release v1.4.5 #232

Merged

thaJeztah mentioned this pull request May 19, 2021

Revert "remove default "containerd.toml" config file" #233

Merged

Uh oh!

Conversation

thaJeztah commented Jan 21, 2021

Uh oh!

thaJeztah commented Jan 21, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

oldthreefeng commented Jan 22, 2021

Uh oh!

thaJeztah commented Jan 22, 2021

Uh oh!

thaJeztah Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

ravilr May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 19, 2021

Choose a reason for hiding this comment

Uh oh!

ravilr May 20, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 25, 2021

Uh oh!

tianon commented Jan 25, 2021

Uh oh!

thaJeztah commented Jan 26, 2021

Uh oh!

tianon commented Jan 28, 2021

Uh oh!

zhsj commented Jan 30, 2021

Uh oh!

zhsj commented Jan 30, 2021

Uh oh!

thaJeztah commented Feb 16, 2021

Uh oh!

thaJeztah commented Feb 16, 2021

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 4, 2021

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 8, 2021

Uh oh!

kzys commented Apr 22, 2021

Uh oh!

thaJeztah commented Apr 22, 2021

Uh oh!

thaJeztah commented May 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ravilr May 14, 2021 •

edited

Loading