Skip to content

Conversation

@Sharpie
Copy link
Member

@Sharpie Sharpie commented Aug 20, 2022

The deb_family_systemd_volume logic hardcoded a
--volume /sys/fs/cgroup:/sys/fs/cgroup:ro flag when provisioning
Debian or Ubuntu containers in order to allow SystemD to run.

However, this mount is no longer sufficient when the docker host
is running a Kernel with CGroupsV2 and a SystemD version that defaults
to using systemd.enableUnifiedCgroupHierarchy=true:

docker/for-mac#6073

Ubuntu 22.04 fits these criteria.

In these conditions, --cgroupns=host -v /sys/fs/cgroup:/sys/fs/cgroup:rw
must be used. However, attempting to pass these flags to docker_exp
via docker_run_opts causes docker run to fail due to a conflict
with the hardcoded mount from deb_family_systemd_volume:

stderr:docker: Error response from daemon: Duplicate mount point: /sys/fs/cgroup.

This commit removes the deb_family_systemd_volume logic as:

  • CGroup mounts must be configured for any OS family using SystemD, not
    just Debian and Ubuntu.

  • The user should be able to exercise full control over mount flags
    via docker_run_opts.

@Sharpie Sharpie requested a review from a team as a code owner August 20, 2022 17:02
The `deb_family_systemd_volume` logic hardcoded a
`--volume /sys/fs/cgroup:/sys/fs/cgroup:ro` flag when provisioning
Debian or Ubuntu containers in order to allow SystemD to run.

However, this mount is no longer sufficient when the docker host
is running a Kernel with CGroupsV2 and a SystemD version that defaults
to using `systemd.enableUnifiedCgroupHierarchy=true`:

  docker/for-mac#6073

Ubuntu 22.04 fits these criteria.

In these conditions, `--cgroupns=host -v /sys/fs/cgroup:/sys/fs/cgroup:rw`
must be used. However, attempting to pass these flags to `docker_exp`
via `docker_run_opts` causes `docker run` to fail due to a conflict
with the hardcoded mount from `deb_family_systemd_volume`:

```
stderr:docker: Error response from daemon: Duplicate mount point: /sys/fs/cgroup.
```

This commit removes the `deb_family_systemd_volume` logic as:

 - CGroup mounts must be configured for any OS family using SystemD, not
   just Debian and Ubuntu.

 - The user should be able to exercise full control over mount flags
   via `docker_run_opts`.
@Sharpie Sharpie force-pushed the remove-deb_family_systemd_volume branch from 4ddef81 to b88830d Compare August 20, 2022 17:03
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@39665ef). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head d1908fb differs from pull request most recent head b88830d. Consider uploading reports for the commit b88830d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage        ?   77.46%           
=======================================
  Files           ?        2           
  Lines           ?      142           
  Branches        ?        0           
=======================================
  Hits            ?      110           
  Misses          ?       32           
  Partials        ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

Hello! 👋

This pull request has been open for a while and has had no recent activity. We've labelled it with attention-needed so that we can get a clear view of which PRs need our attention.

If you are waiting on a response from us we will try and address your comments on a future Community Day.

Alternatively, if it is no longer relevant to you please close the PR with a comment.

Please note that if a pull request receives no update for 7 after it has been labelled, it will be closed. We are always happy to re-open pull request if they have been closed in error.

@h0tw1r3
Copy link
Contributor

h0tw1r3 commented Feb 9, 2023

Is this necessary? Ran into the same issue using the docker provisioner with the docker daemon in a nested ubuntu 22 container:

---
default:
  provisioner: docker
  images:
    - litmusimage/debian:11
  vars: "docker_run_opts: ['--tmpfs=/run', '--tmpfs=/run/lock', '-v=/sys/fs/cgroup/puppet.slice:/sys/fs/cgroup/puppet.slice:rw', '--cgroupns=host', '--cgroup-parent=puppet.slice']"

@chelnak
Copy link
Contributor

chelnak commented Feb 10, 2023

Did setting those vars in the config work for you?

@h0tw1r3
Copy link
Contributor

h0tw1r3 commented Feb 10, 2023

@chelnak Indeed. Adding a parent cgroup (which docker will auto-create) and mounting the parent as read-write ends up creating a docker-* specific cgroup underneath puppet.slice for each container without interfering with the hosts cgroup.

@chelnak chelnak closed this Mar 8, 2023
@chelnak chelnak reopened this Mar 8, 2023
@jordanbreen28
Copy link
Contributor

jordanbreen28 commented Mar 21, 2023

@Sharpie Nice one! could you rebase off main & that should re-kick the CI too.
This change should solve some problems, so hoping to get it in soon!

@jordanbreen28
Copy link
Contributor

jordanbreen28 commented Mar 21, 2023

Just after taking a further look into this, looks like there is still some references to deb_family_systemd_volume in provision/tasks/docker.rb

deb_family_systemd_volume = if (image =~ %r{debian|ubuntu}) && (image !~ %r{debian8|ubuntu14})

Feel free to lift the changes out of my fork from here :-)

@jordanbreen28
Copy link
Contributor

Closing in favour of #203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants