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

Fix #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix #1

wants to merge 2 commits into from

Conversation

grooverdan
Copy link

few small fixes. Some details in the commit message, email response coming too.

The datavolume needs to match the mkdir in the ExecStartPre.

All 3 MARIADB_{USER,PASSWORD,DATABASE} are required to create
a user and database.

Correct PODMAN_SYSTEMD_UNIT=%n

Because this is a type=notify service, podman passes to conmon to
to systemd the MAINPID. With this systemd knows about
the process conmon and its MAINPID, status there is no need for cidfile
based logic.

Remove TimeoutStopSec=70, since https://jira.mariadb.org/browse/MDEV-14705
there should be enough interaction over the type=notify service
to keep systemd adviced of progress during slow shutdowns so there isn't
a timeout.

Remove --sdnotify=conmon to revert back to the default "container".
MariaDB sends a READY so there's no need for conmon to fake it.
NotifyAccess=all seem to imply that any process in the process group
can send the READY=1.

Couldn't see reason for cgroups=no-conmon. --cgroups=split might be
a better option if you go with SendSIGKILL=no. This behaviour under
system ensure's there is only one process the cgroup. Otherwise the
default cgroups setting is ok.
@eriksjolund
Copy link
Owner

Hi @grooverdan, thanks for reaching out and finding the bugs!

I wonder if you could split out these bug fixes

cp -r mariadb*@.* ~/.config/systemd/user
--volume %h/mariadb-data-unix.%i:/var/lib/mysql:Z

and the same bug fix in [email protected]:

--volume %h/mariadb-data-tcp.%i:/var/lib/mysql:Z   

into a separate PR. I could then merge that PR directly. (If you want to, I could also write the PR).
The reason for separating out those changes is that they are obvious bug fixes.
For the rest of the changes I would need to study them a bit more first.

I think the line

Environment=PODMAN_SYSTEMD_UNIT=%n-%i

is probably correct. At least, it comes
directly from podman systemd generate --template.

[test@laptop ~]$ podman generate systemd --name --new --template nginxtest | grep Environment
Environment=PODMAN_SYSTEMD_UNIT=%n-%i
[test@laptop ~]$ podman generate systemd --name --new --help | grep -- --template
      --template                  Make it a template file and use %i and %I specifiers. Working only for containers
[test@laptop ~]$ podman --version
podman version 4.0.0-rc5
[test@laptop ~]$ 

It would be good to stay as close to the Podman default configuration as possible. Here is an example that shows how Podman 4.0.0-rc5 generates a systemd unit file:

[test@laptop ~]$ podman run --rm -d --name nginxtest docker.io/library/nginx
4f63df20801644739d7be2dabfcdff8f44097bdf42b1814d8d2206954716e4cc
[test@laptop ~]$ podman generate systemd --name --new --template nginxtest
# [email protected]
# autogenerated by Podman 4.0.0-rc5
# Mon Feb 14 14:10:35 CET 2022

[Unit]
Description=Podman container-nginxtest.service for %I
Documentation=man:podman-generate-systemd(1)
Wants=network-online.target
After=network-online.target
RequiresMountsFor=%t/containers

[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n-%i
Restart=on-failure
TimeoutStopSec=70
ExecStartPre=/bin/rm -f %t/%n.ctr-id
ExecStart=/usr/bin/podman run --cidfile=%t/%n.ctr-id --cgroups=no-conmon --rm --sdnotify=conmon --replace -d --name nginxtest-%i docker.io/library/nginx
ExecStop=/usr/bin/podman stop --ignore --cidfile=%t/%n.ctr-id
ExecStopPost=/usr/bin/podman rm -f --ignore --cidfile=%t/%n.ctr-id
Type=notify
NotifyAccess=all

[Install]
WantedBy=default.target
[test@laptop ~]$ podman --version
podman version 4.0.0-rc5
[test@laptop ~]$ 

I understand that the changes you suggest might still be needed. It would be good to try to document the reasons for each change that drift away from the default. Right now I don't have a good understanding of how systemd and podman stops and restart containers. I realize that I need to study this a bit.

In the best of worlds it would be possible to use Quadlet. The advantage is that the config files would be smaller and look cleaner. To use Quadlet I guess one need to stay close to the defaults, though.

When I read
https://docs.podman.io/en/latest/markdown/podman-run.1.html#sdnotify-container-conmon-ignore
--sdnotify=container seems to be the best match as to what we need. As this value is also the default, the --sdnotify option can be left out (just as you have done in this PR).

@grooverdan
Copy link
Author

Starting/stopping services - above linked issue.

Environment=PODMAN_SYSTEMD_UNIT given its use in pkg/autoupdate/autoupdate.go as the unit name I suspect the default is wrong, but I've never used the autoupdate.

Quadlet looks really cool. Thanks, I hadn't come across this.

I probably don't have time to neatly reorganise this into multiple PRs or investigate the autoupdate more.

eriksjolund added a commit that referenced this pull request Feb 15, 2022
The GitHub PR
#1
contains a many different changes.

Relevant commits:
grooverdan@746bd11
grooverdan@73a00b0

Let's seperate out the changes that are obvious bugs.

Signed-off-by: Erik Sjölund <[email protected]>
eriksjolund added a commit that referenced this pull request Feb 15, 2022
The GitHub PR
#1
contains a many different changes.

Relevant commits:
grooverdan@746bd11
grooverdan@73a00b0

Let's seperate out the changes that are obvious bugs.

Signed-off-by: Erik Sjölund <[email protected]>
eriksjolund added a commit that referenced this pull request Feb 15, 2022
The GitHub PR
#1
contains a many different changes.

Relevant commits:
grooverdan@746bd11
grooverdan@73a00b0

Let's seperate out the changes that are obvious bugs.

Signed-off-by: Erik Sjölund <[email protected]>
eriksjolund added a commit that referenced this pull request Feb 15, 2022
The GitHub PR
#1
contains a many different changes.

Relevant commits:
grooverdan@746bd11
grooverdan@73a00b0

Let's separate out the changes that are obvious bugs.

Signed-off-by: Erik Sjölund <[email protected]>
eriksjolund added a commit that referenced this pull request Feb 15, 2022
* Daniel Black (@grooverdan)
  provided bug fixes for the Unix service in
  #1
  Fix similar bugs for the TCP service.

* Fix incorrect directory name for the drop-in
  [email protected]/override.conf

* Let the container names for the two different
  templated services have their own value space
  (--name mariadb-unix-%i and
  --name mariadb-tcp-%i)

Signed-off-by: Erik Sjölund <[email protected]>
eriksjolund added a commit that referenced this pull request Feb 15, 2022
* Daniel Black (@grooverdan) provided bug fixes
  for the Unix socket service in
  #1
  Fix similar bugs for the TCP service.

* Fix incorrect directory name for the drop-in
  [email protected]/override.conf

* Let the container names for the two different
  templated services have their own value space
  (--name mariadb-unix-%i and
  --name mariadb-tcp-%i)

Signed-off-by: Erik Sjölund <[email protected]>
eriksjolund added a commit that referenced this pull request Feb 15, 2022
…container-selinux v2.178.0

* container-selinux v2.178.0 (released 11 Feb 2022) adds support for
  using --security-opt label=enable for Unix sockets

Fixes #1

Signed-off-by: Erik Sjölund <[email protected]>
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.

2 participants