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

--sdnotify default value from the "podman create" man page is "container" but "conmon" is used when running "podman generate systemd ..." #15029

Closed
eriksjolund opened this issue Jul 21, 2022 · 17 comments · Fixed by #15043
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@eriksjolund
Copy link
Contributor

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

--sdnotify has the default value container according to the podman create man page but the value conmon is used instead when running podman generate systemd ...

Default is **container**, which means allow the OCI runtime to proxy the socket into the

Steps to reproduce the issue:

  1. Run the commands
    $ podman create --rm --name test1 ghcr.io/eriksjolund/socket-activate-echo
    4ec5ea126989c6926ed9ce05fcdf713209413b2dd083984c5ea40c80e15d4877
    $ podman generate systemd --name --new test1 | grep sdnotify
    	--sdnotify=conmon \
    $ 
    

Describe the results you received:

	--sdnotify=conmon \

Describe the results you expected:

If container is the default value, I would expect the result to be

	--sdnotify=container \

Additional information you deem important (e.g. issue happens only occasionally):

Output of podman version:

Client:       Podman Engine
Version:      4.1.1
API Version:  4.1.1
Go Version:   go1.18.3
Built:        Wed Jun 22 18:17:44 2022
OS/Arch:      linux/amd64

Output of podman info --debug:

host:
  arch: amd64
  buildahVersion: 1.26.1
  cgroupControllers:
  - cpu
  - io
  - memory
  - pids
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: conmon-2.1.0-2.fc36.x86_64
    path: /usr/bin/conmon
    version: 'conmon version 2.1.0, commit: '
  cpuUtilization:
    idlePercent: 99.91
    systemPercent: 0.03
    userPercent: 0.05
  cpus: 16
  distribution:
    distribution: fedora
    variant: workstation
    version: "36"
  eventLogger: journald
  hostname: asus
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 1071
      size: 1
    - container_id: 1
      host_id: 4511264
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 1071
      size: 1
    - container_id: 1
      host_id: 4511264
      size: 65536
  kernel: 5.18.11-200.fc36.x86_64
  linkmode: dynamic
  logDriver: journald
  memFree: 2152271872
  memTotal: 7578664960
  networkBackend: netavark
  ociRuntime:
    name: crun
    package: crun-1.4.5-1.fc36.x86_64
    path: /usr/bin/crun
    version: |-
      crun version 1.4.5
      commit: c381048530aa750495cf502ddb7181f2ded5b400
      spec: 1.0.0
      +SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +CRIU +YAJL
  os: linux
  remoteSocket:
    path: /run/user/1071/podman/podman.sock
  security:
    apparmorEnabled: false
    capabilities: CAP_CHOWN,CAP_DAC_OVERRIDE,CAP_FOWNER,CAP_FSETID,CAP_KILL,CAP_NET_BIND_SERVICE,CAP_SETFCAP,CAP_SETGID,CAP_SETPCAP,CAP_SETUID,CAP_SYS_CHROOT
    rootless: true
    seccompEnabled: true
    seccompProfilePath: /usr/share/containers/seccomp.json
    selinuxEnabled: true
  serviceIsRemote: false
  slirp4netns:
    executable: /usr/bin/slirp4netns
    package: slirp4netns-1.2.0-0.2.beta.0.fc36.x86_64
    version: |-
      slirp4netns version 1.2.0-beta.0
      commit: 477db14a24ff1a3de3a705e51ca2c4c1fe3dda64
      libslirp: 4.6.1
      SLIRP_CONFIG_VERSION_MAX: 3
      libseccomp: 2.5.3
  swapFree: 7574908928
  swapTotal: 7578054656
  uptime: 31h 46m 39.4s (Approximately 1.29 days)
plugins:
  log:
  - k8s-file
  - none
  - passthrough
  - journald
  network:
  - bridge
  - macvlan
  volume:
  - local
registries:
  search:
  - registry.fedoraproject.org
  - registry.access.redhat.com
  - docker.io
  - quay.io
store:
  configFile: /home/test159/.config/containers/storage.conf
  containerStore:
    number: 9
    paused: 0
    running: 1
    stopped: 8
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /home/test159/.local/share/containers/storage
  graphRootAllocated: 407822663680
  graphRootUsed: 350593806336
  graphStatus:
    Backing Filesystem: xfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Using metacopy: "false"
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 12
  runRoot: /run/user/1071/containers
  volumePath: /home/test159/.local/share/containers/storage/volumes
version:
  APIVersion: 4.1.1
  Built: 1655914664
  BuiltTime: Wed Jun 22 18:17:44 2022
  GitCommit: ""
  GoVersion: go1.18.3
  Os: linux
  OsArch: linux/amd64
  Version: 4.1.1

Package info (e.g. output of rpm -q podman or apt list podman):

podman-4.1.1-2.fc36.x86_64
@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 21, 2022
@mheon
Copy link
Member

mheon commented Jul 21, 2022

I believe that this is a deliberate change to generated unit files, and we will override any user-specified setting as well. I don't recall the reasoning for that, though. @vrothberg would know.

@eriksjolund
Copy link
Contributor Author

eriksjolund commented Jul 21, 2022

I thought default value by definition meant that the option can be left out from the command-line whenever you need to specify that value. How it currently works does not match that. If I add --sdnotify=container I get another result.

Edit

Comparison of using --sdnotify=container and leaving it out

$ podman create -q --rm --name ctr --sdnotify=container ghcr.io/eriksjolund/socket-activate-echo
8928f7df3702fdf612f25699cd73693d2be1423a32a7399d9b2a3b61d9b53345
$ podman generate systemd --name --new ctr > /tmp/with
$ podman container rm ctr
8928f7df3702fdf612f25699cd73693d2be1423a32a7399d9b2a3b61d9b53345
$ podman create -q --rm --name ctr ghcr.io/eriksjolund/socket-activate-echo
461312ee6be3c9c5431a8fa6cef5b6c0aa68b9705168d4812dc0d1cd19eb6ddf
$ podman generate systemd --name --new ctr > /tmp/without
--- /tmp/without	2022-07-22 07:47:56.167326337 +0200
+++ /tmp/with	2022-07-22 07:47:31.798918966 +0200
@@ -1,6 +1,6 @@
 # container-ctr.service
 # autogenerated by Podman 4.1.1
-# Fri Jul 22 07:47:56 CEST 2022
+# Fri Jul 22 07:47:31 CEST 2022
 
 [Unit]
 Description=Podman container-ctr.service
@@ -18,11 +18,11 @@
 	--cidfile=%t/%n.ctr-id \
 	--cgroups=no-conmon \
 	--rm \
-	--sdnotify=conmon \
 	-d \
 	--replace \
 	-q \
-	--name ctr ghcr.io/eriksjolund/socket-activate-echo
+	--name ctr \
+	--sdnotify=container ghcr.io/eriksjolund/socket-activate-echo
 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

@vrothberg
Copy link
Member

podman generate systemd overrides the --sdnotify flag only when it has not been set directly by the user. The reasoning for that is portability: almost no container workload sends notify messages, so unit files would not work as systemd would wait for a ready message that just never comes. That's why generate systemd sets --sdnotify=conmon to let conmon send the ready message once the container has started.

If the user created the container with --sdnotify=foo, generate systemd will not change the flag as Podman must assume that the user knows what they're doing.

@eriksjolund, shall we add that to the docs of generate systemd?

@vrothberg
Copy link
Member

I am impressed by the level of detail, @eriksjolund. Thanks for all the great contributions!

@eriksjolund
Copy link
Contributor Author

eriksjolund commented Jul 22, 2022

Thanks @vrothberg!

To me it seems conmon is currently the default value.

Wouldn't the most proper fix be to set a container object datafield for this directly (when podman create ... is run)?
The decision to make conmon the default value is taken at a later point (when podman generate systemd .. is run).

Currently it works like this

$ podman create --rm --name test55 --sdnotify=conmon ghcr.io/eriksjolund/socket-activate-echo
8f97c9ce252cb3a601dcb522e01d91014f0eb1f8b5745b002b1f82cded2ed495
$ podman create --rm --name test56 --sdnotify=container ghcr.io/eriksjolund/socket-activate-echo
5b79f69dce8defc9abff901806edeb1759d1638e122cbe0af3561e016eac4db7
$ podman create --rm --name test57 ghcr.io/eriksjolund/socket-activate-echo
753c18ab7d0cbd8b438c390c43bd7f3c8a3f31d590165119fce28755e70b96da
$ podman container inspect test55| grep -i notif
                    "--sdnotify=conmon",
$ podman container inspect test56| grep -i notif
                    "--sdnotify=container",
$ podman container inspect test57| grep -i notif
$ 

A sketch of how it could work instead

(edited)

$ podman container inspect test55| grep -i notif
                    "--sdnotify=conmon",
                    "SdNotify=conmon",
$ podman container inspect test56| grep -i notif
                    "--sdnotify=container",
                    "SdNotify=container",
$ podman container inspect test57| grep -i notif
                    "SdNotify=conmon",
$ 

@vrothberg
Copy link
Member

To me it seems conmon is currently the default value.

Oh, that is good news (and a sane default)! I didn't double-check in the code.

Wouldn't the most proper fix be to set a container object datafield for this directly (when podman create ... is run)?
The decision to make conmon the default value is taken at a later point (when podman generate systemd .. is run).

So you would enforce setting --sdnotify=conmon in the units if the user didn't specify it explicitly? Sounds good to me!

@eriksjolund
Copy link
Contributor Author

So you would enforce setting --sdnotify=conmon in the units if the user didn't specify it explicitly? Sounds good to me!

Yes, that is how it works right now.

But there is currently no way to extract the value with the command podman container inspect -f ....
For instance a container name can be extracted like this

$ podman container inspect -f {{.Name}} test57
test57

If there would be a datafield for sdnotify it would be possible to run something like

$ podman container inspect -f {{.SdNotify}} test57
conmon

I think it would good to have such a command when supporting user questions in internet forums.

I made an edit to #15029 (comment) regarding
the sketch for output from the podman container inspect commands.

@eriksjolund
Copy link
Contributor Author

An easy fix for this issue would be to just change the documentation

from

Default is container,

to

Default is conmon,

The other part, introducing
podman container inspect -f {{.SdNotify}}
would be nice to have but if it's tricky to implement it's not that important.

@vrothberg
Copy link
Member

An easy fix for this issue would be to just change the documentation

SGTM! Interested in opening a PR?

@Luap99
Copy link
Member

Luap99 commented Jul 22, 2022

The current default in the code is container. The sdnotify socket always gets the MAINPID from conmon and the ready message should be send from the container.
Outside of systemd it doesn't matter because NOTIFY_SOCKET is not set and nothing is waiting for the ready message.
With systemd we cannot use container by default because it would just think that the service never started correctly if the container process never sends the ready message.

You can remove the --sdnotify=container option from the sdnotify bats test to confirm:
bats -f container test/system/260-sdnotify.bats

@eriksjolund
Copy link
Contributor Author

eriksjolund commented Jul 22, 2022

An easy fix for this issue would be to just change the documentation

SGTM! Interested in opening a PR?

At the moment I'm a bit too confused about how it works. (I thought conmon was the default but not anymore after reading the comment from @Luap99)

@vrothberg
Copy link
Member

vrothberg commented Jul 22, 2022

I am less confused now :) As mentioned above, I thought the default was "container".

@eriksjolund, should we improve the docs of generate systemd?

@Luap99
Copy link
Member

Luap99 commented Jul 22, 2022

@eriksjolund see test/system/260-sdnotify.bats for examples how the different modes work.

But basically if NOTIFY_SOCKET env is not set then there is nothing to do.
If ignore, NOTIFY_SOCKET is also not used
If conmon, MainPID is conmon and READY will be send by conmon
If container, MainPID is still conmon but READY should be send by the container process

@eriksjolund
Copy link
Contributor Author

@eriksjolund, should we improve the docs of generate systemd?

I gave it a try, see linked PR.

When doing that I noticed that the value ignore is mapped to conmon.

@eriksjolund
Copy link
Contributor Author

When doing that I noticed that the value ignore is mapped to conmon.

I noticed

--sdnotify=ignore gets mapped to --sdnotify=ignore

--sdnotify ignore gets mapped to the default value

I created an issue for that

@rhatdan
Copy link
Member

rhatdan commented Jul 25, 2022

@vrothberg PTAL

eriksjolund added a commit to eriksjolund/podman that referenced this issue Jul 26, 2022
* Document why the default value for --sdnotify is overridden.
  Some was included text from
  containers#15029 (comment)

* Document that --sdnotify=ignore is overridden.

Fixes containers#15029

Co-authored-by: Valentin Rothberg <[email protected]>
Co-authored-by: Tom Sweeney <[email protected]>
Signed-off-by: Erik Sjölund <[email protected]>
@vrothberg
Copy link
Member

#15043

mheon pushed a commit to mheon/libpod that referenced this issue Jul 26, 2022
* Document why the default value for --sdnotify is overridden.
  Some was included text from
  containers#15029 (comment)

* Document that --sdnotify=ignore is overridden.

Fixes containers#15029

Co-authored-by: Valentin Rothberg <[email protected]>
Co-authored-by: Tom Sweeney <[email protected]>
Signed-off-by: Erik Sjölund <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants