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

spread, tests: add CentOS support #6168

Merged
merged 10 commits into from Nov 20, 2018

Conversation

@bboozzoo
Copy link
Contributor

bboozzoo commented Nov 19, 2018

Add CentOS 7.5 to the spread suite.

Apply tweaks to snap-mgmt to discard the mount ns before we remote the mounts, but after stopping snap services. This was a partial workaround for https://access.redhat.com/articles/3128691 The relevant patch is 012b61d and I can drop it if needed.

Add data/sysctl/99-snap.conf which enables lazy mount detach.

@Conan-Kudo can you please take a look at the packaging bits? Could we apply that to the EPEL package?

bboozzoo added 6 commits Sep 28, 2018
…t CentOS 7

Add CentOS 7 to the shared Fedora RPM spec. Problems identified while building
rpm:

- outdated selinux-policy, this should be fixed in RHEL 7.6, see
  https://bugzilla.redhat.com/show_bug.cgi?id=1574383

- hardened build with static linking fails, (snap-exec and snap-update-ns),
  expecting RHEL 7.6 to be affected, reported to CentOS
  https://bugs.centos.org/view.php?id=15333

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…unt ns

Split stopping of snap services and snap mount units. Make sure to call
snap-discard-ns for removed snaps to clean up the mount namespace that may
contain bind mounted objects from the rootfs. Make sure that units from services
are stopped before respective mount units.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…L 7.4+

Enable lazily unmounting mounts in other namespaces that have not received the
propagated unmount when a mount point directory is removed.

See:
  RHBZ#1247935
  https://access.redhat.com/articles/3128691

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Nov 19, 2018

@bboozzoo We might want to give that sysctl file a name that indicates it's rhel7 specific, so people don't think it should be generally installed at a glance.

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Nov 19, 2018

@bboozzoo Is the workaround necessary if the sysctl is applied already? I would think not?

@bboozzoo

This comment has been minimized.

Copy link
Contributor Author

bboozzoo commented Nov 19, 2018

@bboozzoo Is the workaround necessary if the sysctl is applied already? I would think not?

Let me drop the snap-mgmt patch locally and run the whole test suite again.

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Nov 19, 2018

@bboozzoo Also, if we enable the Continuous Release (cr) repository, we'll have the EL7.6 content available in our CI. So, we don't need to disable SELinux for CentOS.

If we have yum-utils installed in our CentOS image, it can be enabled with yum-config-manager --enable cr.

bboozzoo added 4 commits Nov 20, 2018
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo

This comment has been minimized.

Copy link
Contributor Author

bboozzoo commented Nov 20, 2018

@Conan-Kudo I tried that earlier and got this:

Error: Package: snapd-selinux-2.36-2.el7.noarch (epel-testing)
           Requires: selinux-policy-base >= 3.13.1-229.el7_6.5
           Installed: selinux-policy-targeted-3.13.1-229.el7.noarch (@cr)
               selinux-policy-base = 3.13.1-229.el7

Is the package in RHEL proper built with different %dist ?

@zyga
zyga approved these changes Nov 20, 2018
Copy link
Contributor

zyga left a comment

LGTM with some comments inline.

I didn't notice I had older review pending for this pull request. Some comments are for the now-outdated source.

local unit
unit="$1"
# ensure its really a snap mount unit or systemd unit
grep -q 'What=/var/lib/snapd/snaps/' "/etc/systemd/system/$unit" || grep -q 'X-Snappy=yes' "/etc/systemd/system/$unit"

This comment has been minimized.

Copy link
@zyga

zyga Nov 20, 2018

Contributor

I'm -0.25 on the X-Snappy name but +1.25 on the helper function.

This comment has been minimized.

Copy link
@zyga

zyga Nov 20, 2018

Contributor

Is the second condition needed because of mount units? Could you please split this for readability?

if grep -q '...'; then  # snapd-managed mount unit
   return 0
fi
if grep -q '...'; then  # snapd-managed system service, timer or socket.
  return 0
fi
return 1
@@ -121,6 +79,8 @@ purge() {
if [ -d /run/snapd/ns ]; then
if [ "$(find /run/snapd/ns/ -name "*.mnt" | wc -l)" -gt 0 ]; then
for mnt in /run/snapd/ns/*.mnt; do
name=$(echo "$mnt" | sed -e 's/snap\.//' -e 's/\.mnt$//')
@libexecdir@/snap-discard-ns "$name" || true

This comment has been minimized.

Copy link
@zyga

zyga Nov 20, 2018

Contributor

If it helps we could easily add an --all command line option to snap-discard-ns.

if [[ "$SPREAD_SYSTEM" = fedora-* || "$SPREAD_SYSTEM" = amazon-* ]]; then
EXTRA_NC_ARGS=""
fi
case "$SPREAD_SYSTEM" in

This comment has been minimized.

Copy link
@zyga

zyga Nov 20, 2018

Contributor

+1

# we never test on core because the transition can only happen on "classic" we
# disable on ppc64el because the downloads are very slow there Fedora, openSUSE,
# Arch, CentOS are disabled at the moment as there is something fishy going on
# and the snapd service gets terminated during the process.

This comment has been minimized.

Copy link
@zyga

zyga Nov 20, 2018

Contributor

Curious to know what happened here, no need to block on this though.

# symlinks /snap to $SNAP_MOUNT_DIR themselves
ln -sf $SNAP_MOUNT_DIR /snap
fi
case "$SPREAD_SYSTEM" in

This comment has been minimized.

Copy link
@zyga

zyga Nov 20, 2018

Contributor

This really feels like a helper is needed. Feel free to do in a follow up if you want.

. "$TESTSLIB/classic.sh"
enable_classic_support
if [[ "$SPREAD_SYSTEM" == fedora-* || "$SPREAD_SYSTEM" == arch-* ]]; then
rm -f /snap
fi
case "$SPREAD_SYSTEM" in

This comment has been minimized.

Copy link
@zyga

zyga Nov 20, 2018

Contributor

Another thing for the helper to handle.

@bboozzoo bboozzoo changed the title spread, tests, snap-mgmt: add CentOS support spread, tests: add CentOS support Nov 20, 2018
Copy link
Contributor

stolowski left a comment

I cannot judge the rpm packaging bits, but spread test changes look good to me. +1

Copy link
Contributor

sergiocazzolato left a comment

Wellcome Centos :)
Thanks for this change @bboozzoo

@sergiocazzolato sergiocazzolato merged commit fff32f0 into snapcore:master Nov 20, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Nov 20, 2018

@bboozzoo I think that's actually a bug in CentOS, because custom DistTag changes in the release for a specific package are supposed to be mimicked by CentOS as well.

@bboozzoo

This comment has been minimized.

Copy link
Contributor Author

bboozzoo commented Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.