ikke-t / podman-container-systemd

creates systemd files and creates containers using podman
118 stars 44 forks source link

Conditionally set cid- and pidfile #14

Closed cfelder closed 4 years ago

cfelder commented 4 years ago

I am using this role on EL 7 and 8.

On EL 7 %T is not supported by systemd.unit whereas %t is supported.

Therefore I propose to use %t on EL7.

I remember a thread where you (@ikke-t) discussed why he changed form %t to %T. Nevertheless this fix is just applied on el7 and the default is unchanged.

Best Christian

ikke-t commented 4 years ago

Did you btw check what works as user and with root? I recall there is a chance systemd guides to use a location where user doesn't have write permissions in some case.

So it would be nice to have this tested both as a user and as root, while following the logs for possible denial of writing of such file.

cfelder commented 4 years ago

@ikke-t: you're right. This doesn't work for rootless.

Jun 22 12:02:11 centos-7 podman[74458]: Error: error opening cidfile //run/lighttpd-container-pod.service-cid

As %T is not available on EL7 I would simply hard-code cidpid_base to /tmp/%n- for those systems as default value.

What do you think?

ikke-t commented 4 years ago

I think it would be safer to have it in /var/tmp, so it survives reboots if the info is needed for something?

cfelder commented 4 years ago

But from man 5 systemd.unit:

"%T" This is either /tmp or the path "$TMPDIR", "$TEMP" or "$TMP" are set to.

cfelder commented 4 years ago

and I don't think this information is useful after reboot. The old processes will be dead and new ones will be spawned creating new files. Even %t is usually on tmpfs.

cfelder commented 4 years ago

imho can be squashed

ikke-t commented 4 years ago

thanks