From a54429cf877e899eb1516e3ee71ae9c6eedd7c5a Mon Sep 17 00:00:00 2001 From: Danila Kiver Date: Thu, 4 Jul 2019 03:58:37 +0300 Subject: Use conmon pidfile in generated systemd unit as PIDFile. By default, podman points PIDFile in generated unit file to non-existent location. As a result, the unit file, generated by podman, is broken: an attempt to start this unit without prior modification results in a crash, because systemd can not find the pidfile of service's main process. Fix the value of "PIDFile" and add a system test for this case. Signed-off-by: Danila Kiver --- docs/podman-generate-systemd.1.md | 4 ++-- pkg/adapter/containers.go | 9 +++++++- pkg/systemdgen/systemdgen.go | 5 ++--- pkg/systemdgen/systemdgen_test.go | 14 ++++++------ test/system/250-generate-systemd.bats | 42 +++++++++++++++++++++++++++++++++++ test/system/helpers.bash | 11 +++++++++ 6 files changed, 72 insertions(+), 13 deletions(-) create mode 100644 test/system/250-generate-systemd.bats diff --git a/docs/podman-generate-systemd.1.md b/docs/podman-generate-systemd.1.md index 09752480d..64e68a69a 100644 --- a/docs/podman-generate-systemd.1.md +++ b/docs/podman-generate-systemd.1.md @@ -39,7 +39,7 @@ ExecStart=/usr/bin/podman start c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f14 ExecStop=/usr/bin/podman stop -t 10 c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc KillMode=none Type=forking -PIDFile=/var/lib/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc.pid +PIDFile=/var/run/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/conmon.pid [Install] WantedBy=multi-user.target ``` @@ -55,7 +55,7 @@ ExecStart=/usr/bin/podman start c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f14 ExecStop=/usr/bin/podman stop -t 1 c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc KillMode=none Type=forking -PIDFile=/var/lib/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc.pid +PIDFile=/var/run/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/conmon.pid [Install] WantedBy=multi-user.target ``` diff --git a/pkg/adapter/containers.go b/pkg/adapter/containers.go index 10720886b..0ea89a72c 100644 --- a/pkg/adapter/containers.go +++ b/pkg/adapter/containers.go @@ -1058,7 +1058,14 @@ func (r *LocalRuntime) GenerateSystemd(c *cliconfig.GenerateSystemdValues) (stri if c.Name { name = ctr.Name() } - return systemdgen.CreateSystemdUnitAsString(name, ctr.ID(), c.RestartPolicy, ctr.Config().StaticDir, timeout) + + config := ctr.Config() + conmonPidFile := config.ConmonPidFile + if conmonPidFile == "" { + return "", errors.Errorf("conmon PID file path is empty, try to recreate the container with --conmon-pidfile flag") + } + + return systemdgen.CreateSystemdUnitAsString(name, ctr.ID(), c.RestartPolicy, conmonPidFile, timeout) } // GetNamespaces returns namespace information about a container for PS diff --git a/pkg/systemdgen/systemdgen.go b/pkg/systemdgen/systemdgen.go index 3d1c31b5d..fb91b3e82 100644 --- a/pkg/systemdgen/systemdgen.go +++ b/pkg/systemdgen/systemdgen.go @@ -2,7 +2,6 @@ package systemdgen import ( "fmt" - "path/filepath" "github.com/pkg/errors" ) @@ -33,11 +32,11 @@ func ValidateRestartPolicy(restart string) error { // CreateSystemdUnitAsString takes variables to create a systemd unit file used to control // a libpod container -func CreateSystemdUnitAsString(name, cid, restart, pidPath string, stopTimeout int) (string, error) { +func CreateSystemdUnitAsString(name, cid, restart, pidFile string, stopTimeout int) (string, error) { if err := ValidateRestartPolicy(restart); err != nil { return "", err } - pidFile := filepath.Join(pidPath, fmt.Sprintf("%s.pid", cid)) + unit := fmt.Sprintf(template, name, restart, name, stopTimeout, name, pidFile) return unit, nil } diff --git a/pkg/systemdgen/systemdgen_test.go b/pkg/systemdgen/systemdgen_test.go index f2f49e750..a0f6a1889 100644 --- a/pkg/systemdgen/systemdgen_test.go +++ b/pkg/systemdgen/systemdgen_test.go @@ -41,7 +41,7 @@ ExecStart=/usr/bin/podman start 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4 ExecStop=/usr/bin/podman stop -t 10 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401 KillMode=none Type=forking -PIDFile=/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.pid +PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid [Install] WantedBy=multi-user.target` @@ -53,7 +53,7 @@ ExecStart=/usr/bin/podman start foobar ExecStop=/usr/bin/podman stop -t 10 foobar KillMode=none Type=forking -PIDFile=/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.pid +PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid [Install] WantedBy=multi-user.target` @@ -61,7 +61,7 @@ WantedBy=multi-user.target` name string cid string restart string - pidPath string + pidFile string stopTimeout int } tests := []struct { @@ -76,7 +76,7 @@ WantedBy=multi-user.target` "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "always", - "/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/", + "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", 10, }, goodID, @@ -87,7 +87,7 @@ WantedBy=multi-user.target` "foobar", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "always", - "/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/", + "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", 10, }, goodName, @@ -98,7 +98,7 @@ WantedBy=multi-user.target` "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "never", - "/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/", + "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", 10, }, "", @@ -107,7 +107,7 @@ WantedBy=multi-user.target` } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := CreateSystemdUnitAsString(tt.args.name, tt.args.cid, tt.args.restart, tt.args.pidPath, tt.args.stopTimeout) + got, err := CreateSystemdUnitAsString(tt.args.name, tt.args.cid, tt.args.restart, tt.args.pidFile, tt.args.stopTimeout) if (err != nil) != tt.wantErr { t.Errorf("CreateSystemdUnitAsString() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/test/system/250-generate-systemd.bats b/test/system/250-generate-systemd.bats new file mode 100644 index 000000000..87ad69774 --- /dev/null +++ b/test/system/250-generate-systemd.bats @@ -0,0 +1,42 @@ +#!/usr/bin/env bats -*- bats -*- +# +# Tests generated configurations for systemd. +# + +load helpers + +# Be extra paranoid in naming to avoid collisions. +SERVICE_NAME="podman_test_$(random_string)" +UNIT_DIR="$HOME/.config/systemd/user" +UNIT_FILE="$UNIT_DIR/$SERVICE_NAME.service" + +function setup() { + basic_setup + mkdir -p "$UNIT_DIR" +} + +function teardown() { + rm -f "$UNIT_FILE" + systemctl --user stop "$SERVICE_NAME" + basic_teardown +} + +@test "podman generate - systemd - basic" { + skip_if_not_systemd + skip_if_remote + + run_podman create $IMAGE echo "I'm alive!" + cid="$output" + + run_podman generate systemd $cid > "$UNIT_FILE" + + run systemctl --user start "$SERVICE_NAME" + if [ $status -ne 0 ]; then + die "The systemd service $SERVICE_NAME did not start correctly!" + fi + + run_podman logs $cid + is "$output" "I'm alive!" "Container output" +} + +# vim: filetype=sh diff --git a/test/system/helpers.bash b/test/system/helpers.bash index 29ef19ecc..1db80f111 100644 --- a/test/system/helpers.bash +++ b/test/system/helpers.bash @@ -236,6 +236,17 @@ function skip_if_remote() { skip "${1:-test does not work with podman-remote}" } +######################### +# skip_if_not_systemd # ...with an optional message +######################### +function skip_if_not_systemd() { + if systemctl --user >/dev/null 2>&1; then + return + fi + + skip "${1:-no systemd or daemon does not respond}" +} + ######### # die # Abort with helpful message ######### -- cgit v1.2.3-54-g00ecf From 37b134054e768317415f1ee0156a422b98656f24 Mon Sep 17 00:00:00 2001 From: Danila Kiver Date: Thu, 4 Jul 2019 21:08:06 +0300 Subject: Use default conmon pidfile location for root containers. The conmon pidfile is crucial for podman-generated systemd units, because these units rely on it for determining service's main process ID. With this change, every container has ConmonPidFile set (at least to default value). Signed-off-by: Danila Kiver --- libpod/runtime_ctr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 9daac161c..683969118 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -236,7 +236,7 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container, restore bo } }() - if rootless.IsRootless() && ctr.config.ConmonPidFile == "" { + if ctr.config.ConmonPidFile == "" { ctr.config.ConmonPidFile = filepath.Join(ctr.state.RunDir, "conmon.pid") } -- cgit v1.2.3-54-g00ecf From c490754ff769f3327fb165fad87baf515210fbac Mon Sep 17 00:00:00 2001 From: Danila Kiver Date: Sat, 6 Jul 2019 01:35:46 +0300 Subject: Add debug information to "generate systemd" test. Signed-off-by: Danila Kiver --- test/system/250-generate-systemd.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/system/250-generate-systemd.bats b/test/system/250-generate-systemd.bats index 87ad69774..20a73c726 100644 --- a/test/system/250-generate-systemd.bats +++ b/test/system/250-generate-systemd.bats @@ -32,7 +32,7 @@ function teardown() { run systemctl --user start "$SERVICE_NAME" if [ $status -ne 0 ]; then - die "The systemd service $SERVICE_NAME did not start correctly!" + die "The systemd service $SERVICE_NAME did not start correctly, output: $output" fi run_podman logs $cid -- cgit v1.2.3-54-g00ecf From 1f435bf92cc5e828cb36e658d695070473a41f96 Mon Sep 17 00:00:00 2001 From: Danila Kiver Date: Sat, 6 Jul 2019 17:41:29 +0300 Subject: Reload systemd daemon on creation of units location dir in tests. Systemd manager drops non-existent directories from the units search path during initialization, thus, creation of UNIT_DIR, if it did not exist before, requres reloading the daemon. Signed-off-by: Danila Kiver --- test/system/250-generate-systemd.bats | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/system/250-generate-systemd.bats b/test/system/250-generate-systemd.bats index 20a73c726..a8eb3b0a6 100644 --- a/test/system/250-generate-systemd.bats +++ b/test/system/250-generate-systemd.bats @@ -12,7 +12,11 @@ UNIT_FILE="$UNIT_DIR/$SERVICE_NAME.service" function setup() { basic_setup - mkdir -p "$UNIT_DIR" + + if [ ! -d "$UNIT_DIR" ]; then + mkdir -p "$UNIT_DIR" + systemctl --user daemon-reload + fi } function teardown() { -- cgit v1.2.3-54-g00ecf From 8fde4194b6a21bc54d83260990e839da9e1fe1e8 Mon Sep 17 00:00:00 2001 From: Danila Kiver Date: Sat, 6 Jul 2019 20:45:26 +0300 Subject: Move skipping systemd tests to early setup. There is no meaning of performing setup/teardown for these tests when we even can not work with systemd. Signed-off-by: Danila Kiver --- test/system/250-generate-systemd.bats | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/system/250-generate-systemd.bats b/test/system/250-generate-systemd.bats index a8eb3b0a6..80199af5f 100644 --- a/test/system/250-generate-systemd.bats +++ b/test/system/250-generate-systemd.bats @@ -11,6 +11,9 @@ UNIT_DIR="$HOME/.config/systemd/user" UNIT_FILE="$UNIT_DIR/$SERVICE_NAME.service" function setup() { + skip_if_not_systemd + skip_if_remote + basic_setup if [ ! -d "$UNIT_DIR" ]; then @@ -26,9 +29,6 @@ function teardown() { } @test "podman generate - systemd - basic" { - skip_if_not_systemd - skip_if_remote - run_podman create $IMAGE echo "I'm alive!" cid="$output" -- cgit v1.2.3-54-g00ecf From 2bfade4391bb6d247ddab2d129d0529471c17063 Mon Sep 17 00:00:00 2001 From: Danila Kiver Date: Sun, 7 Jul 2019 11:20:41 +0300 Subject: Do not hardcode podman binary location in generate systemd. It is not correct to rely on specific location of the podman binary. In most cases it is /usr/bin/podman, but sometimes is not (e.g. in system tests). Use /proc/self/exe instead of hardcoded path. Signed-off-by: Danila Kiver --- pkg/systemdgen/systemdgen.go | 23 ++++++++++++++++++++--- pkg/systemdgen/systemdgen_test.go | 6 +++++- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/pkg/systemdgen/systemdgen.go b/pkg/systemdgen/systemdgen.go index fb91b3e82..06c5ebde5 100644 --- a/pkg/systemdgen/systemdgen.go +++ b/pkg/systemdgen/systemdgen.go @@ -2,16 +2,18 @@ package systemdgen import ( "fmt" + "os" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) var template = `[Unit] Description=%s Podman Container [Service] Restart=%s -ExecStart=/usr/bin/podman start %s -ExecStop=/usr/bin/podman stop -t %d %s +ExecStart=%s start %s +ExecStop=%s stop -t %d %s KillMode=none Type=forking PIDFile=%s @@ -33,10 +35,25 @@ func ValidateRestartPolicy(restart string) error { // CreateSystemdUnitAsString takes variables to create a systemd unit file used to control // a libpod container func CreateSystemdUnitAsString(name, cid, restart, pidFile string, stopTimeout int) (string, error) { + podmanExe := getPodmanExecutable() + return createSystemdUnitAsString(podmanExe, name, cid, restart, pidFile, stopTimeout) +} + +func createSystemdUnitAsString(exe, name, cid, restart, pidFile string, stopTimeout int) (string, error) { if err := ValidateRestartPolicy(restart); err != nil { return "", err } - unit := fmt.Sprintf(template, name, restart, name, stopTimeout, name, pidFile) + unit := fmt.Sprintf(template, name, restart, exe, name, exe, stopTimeout, name, pidFile) return unit, nil } + +func getPodmanExecutable() string { + podmanExe, err := os.Executable() + if err != nil { + podmanExe = "/usr/bin/podman" + logrus.Warnf("Could not obtain podman executable location, using default %s", podmanExe) + } + + return podmanExe +} diff --git a/pkg/systemdgen/systemdgen_test.go b/pkg/systemdgen/systemdgen_test.go index a0f6a1889..e413b24ce 100644 --- a/pkg/systemdgen/systemdgen_test.go +++ b/pkg/systemdgen/systemdgen_test.go @@ -58,6 +58,7 @@ PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635 WantedBy=multi-user.target` type args struct { + exe string name string cid string restart string @@ -73,6 +74,7 @@ WantedBy=multi-user.target` {"good with id", args{ + "/usr/bin/podman", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "always", @@ -84,6 +86,7 @@ WantedBy=multi-user.target` }, {"good with name", args{ + "/usr/bin/podman", "foobar", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "always", @@ -95,6 +98,7 @@ WantedBy=multi-user.target` }, {"bad restart policy", args{ + "/usr/bin/podman", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401", "never", @@ -107,7 +111,7 @@ WantedBy=multi-user.target` } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := CreateSystemdUnitAsString(tt.args.name, tt.args.cid, tt.args.restart, tt.args.pidFile, tt.args.stopTimeout) + got, err := createSystemdUnitAsString(tt.args.exe, tt.args.name, tt.args.cid, tt.args.restart, tt.args.pidFile, tt.args.stopTimeout) if (err != nil) != tt.wantErr { t.Errorf("CreateSystemdUnitAsString() error = %v, wantErr %v", err, tt.wantErr) return -- cgit v1.2.3-54-g00ecf