podman play should honour both ENTRYPOINT and CMD from image #8666

cruwe · 2020-12-09T16:54:36Z

Assigning the entrypoint to command may be in line with the Kubernetes command and args pattern, but breaks normal behaviour when creating the container without passing any of those.

I have WIPed this, because I am testing a larger deployment, am uneasy about what to find next and don't want to mess up history with one-liners

openshift-ci-robot · 2020-12-09T16:55:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cruwe
To complete the pull request process, please assign baude after the PR has been reviewed.
You can assign the PR to them by writing /assign @baude in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2020-12-09T16:57:25Z

I'm not opposed but we should make this conditional, so it's not a breaking change. Maybe a --use-cmd flag?

cruwe · 2020-12-09T17:27:21Z

I am starting to wonder: Am I missing some important context or intent which I do not know about?

cruwe · 2020-12-09T18:20:36Z

I think I now got the intent. I am not satisfied with the verbosity, but I think the code should closely mirror the spec to prevent misunderstandings.

What is the project's position on duplication vs readability?

mheon · 2020-12-09T18:21:35Z

I am a firm believer in readability being the first consideration (I am admittedly biased by the amount of code review I do 😄 )

zhangguanzhang · 2020-12-10T01:19:02Z

pkg/specgen/generate/kube/kube.go

+		s.Command = nil
+		s.Command = append(s.Command, containerYAML.Args...)


I think these two lines are easier to read when combined into the following line:

s.Command = containerYAML.Args

put a comment there to clarify the intent

ah, got it ... yes, you're right

zhangguanzhang · 2020-12-10T01:19:29Z

pkg/specgen/generate/kube/kube.go

+		s.Command = nil
 		s.Command = append(s.Command, containerYAML.Args...)


I think these two lines are easier to read when combined into the following line:

s.Command = containerYAML.Args

zhangguanzhang · 2020-12-10T01:20:45Z

and code need go fmt

cruwe · 2020-12-10T07:10:32Z

and code need go fmt

that I do not get. I did and the IDE does at any commit. Nothing changed. Could you point me in the direction?

cruwe · 2020-12-10T07:25:57Z

pkg/specgen/generate/kube/kube.go

 	// doc https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#notes
-	if len(containerYAML.Args) != 0 {
+
+	if len(containerYAML.Command) == 0 && len(containerYAML.Args) == 0 {


btw, I am pretty sure that the initialization with []string{} from L109 is not necessary. It runs fine here without and s.ContainerBasicConfig.Command and .Entrypoint, which in theory should suffer from the same problem, are initialized just fine.

cruwe · 2020-12-10T07:33:30Z

pkg/specgen/generate/kube/kube.go

+		s.Command = nil
+		s.Command = append(s.Command, containerYAML.Args...)


ah, got it ... yes, you're right

zhangguanzhang · 2020-12-10T09:21:45Z

you could right click the changed files., and choose the gofmt，or use gofmt -w command

cruwe · 2020-12-14T08:36:07Z

Without exerting undue pressure, I'd like to point out that this PR fixes a regression:

Take for instance postgresql or redis from the docker-library: Both use the docker-entrypoint.sh -> exec "$@" pattern to do some chores at start-up time before doing a fork-replace into the main application. Starting the containers as a pod, it is reasonable (and defined by the standard mentioned as a comment to the code), that the container will start without having to explicitly pass command and args.

This behaviour is shown by docker, by podman run, by kubernetes and openshift, as also byh podman play kube at version 2.1 and i strongly suspect the versions before, although I have not tested that. It stopped with podman v2.2 and the code I am PRing against.

We use the podman play feature a lot and I think highly of the idea to replace the moving and often elusive target of docker-compose.yml with the most sensible idea of k8s pod specs. Accordingly, I am most interested to get this code reviewed and eventually merged.

Please, tell me what I can do to have the process sped up. Thank you very much for your time and consideration.

zhangguanzhang · 2020-12-14T08:53:17Z

you cloud see this for more detail : https://github.com/containers/podman/pull/8666/checks?check_run_id=1529080539

your code need to go fmt, and git commits need to rebase to 1 commit.
ci will continue to run after that.

cruwe · 2020-12-14T10:15:44Z

you cloud see this for more detail : https://github.com/containers/podman/pull/8666/checks?check_run_id=1529080539

you code need to go fmt, and git commits need to rebase to 1 commit.
ci will continue to run after that.

@zhangguanzhang : Thanks!

cruwe · 2020-12-15T17:58:42Z

I am willing to work on the failing tests. I am not particularly fluent in golang and absolutely new to ginko, so I'll definitely need some guidance.

However, before I start working, could we reach consensus on the general idea to honour entrypoint and command from the image and be strict about the the k8s specs? @zhangguanzhang , @mheon, could you help me reaching that consensus?

mheon · 2020-12-15T18:01:46Z

This behavior cannot be default, because it would be a breaking chance from the way things are now. I am OK with doing it, but it will need to be gated behind a flag.

daiaji · 2020-12-15T18:04:05Z

This did cause the yaml I exported using the podman generate kube command to be unable to deploy using the podman play kube command.

cruwe · 2020-12-15T20:32:39Z

Hi,

I already feared that there might be a severe misunderstanding present here, perhaps on my part, I do not know. I am not trying to implement a feature here, I observed an undocumented breaking change, assumed that to be an unintended side-effect of some major refactoring and attempted, not least because I am/was relying on the feature, to come up with a fix.

Consider the following pod definition:

---
apiVersion: v1
kind: Pod
metadata:
  name: try
spec:
  containers:
    - name: try
      image: docker.io/library/redis:6

The image comes with "pre-packaged" ENTRYPOINT and CMD.

On podman v2.1 (please disregard the naming of my binaries), it starts just fine. The version is

$ ./bin/podman-v2.1 version
Version:      2.1.1
API Version:  2.0.0
Go Version:   go1.13.8
Git Commit:   9f6d6ba0b314d86521b66183c9ce48eaa2da1de2
Built:        Tue Dec 15 19:28:07 2020
OS/Arch:      linux/amd64

and, the container starting fine, I have nothing to tell here.

On podman v2.2, it constantly crashes (without any logs, which I would supply otherwise), because the new play disregards the CMD statement from the image, if nothing has been passed from the pod-spec:

On

Version:      2.2.1
API Version:  2.1.0
Go Version:   go1.13.8
Git Commit:   a0d478edea7f775b7ce32f8eb1a01e75374486cb
Built:        Tue Dec 15 19:24:20 2020
OS/Arch:      linux/amd64

podman inspect yileds

$ ./bin/podman-v2.2 inspect try_pod-try \
| jq '.[] | {"args": .Args, "cmd": .Config.Cmd, "entrypoint": .Config.Entrypoint }'
{
  "args": [
    "docker-entrypoint.sh"
  ],
  "cmd": [
    "docker-entrypoint.sh"
  ],
  "entrypoint": "docker-entrypoint.sh"
}

That certainly can never start any redis.

So, I still do not understand if this behaviour is really intentional (please bear with me should I lack understanding).

Is the new behaviour intentional and canonical? Then, I'd politely ask to mention a breaking change in the Changelogs, still politely, even if I needlessly spent hours chasing my crashing pods in my testing environment. In addition, it should then be documented that users MUST pass command and args in their pod-spec, even if they think both could be taken from the image. Futhermore, the comment referring to the kubernetes specification in the generate code should be removed, because the code the does not implement the k8s specs, which, for instance, demand that ENTRYPOINT and CMD are taken from the image when nothing has been passed.

If it is not intended to behave different than the k8s spec specifies, then I fail (perhaps I am thinking wrong here?) to understand why a correct behaviour should be gated behind a flag, when the incorrect behaviour is the default.

I hope I did not let emotions run here, I certainly do not want to offend. I (still?) firmly believe the behaviour of v2.2 is buggy and I am ready to stand corrected should someone explain to me disregarding CMD from the image is correct.

In any case, thank you for your patience. (I am on CET, so I will not be active any more for my instance of today.)

mheon · 2020-12-15T20:36:08Z

OK, this is not the impression I got from the initial PR - sorry. Yes, we did do a major rewrite of play kube logic for 2.2.x, so this could well be a regression fix. If it is, there is no need to add a flag.

If this is a regression, we need a test to catch it in the future; would you be willing to write one?

cruwe · 2020-12-16T08:28:10Z

If this is a regression, we need a test to catch it in the future; would you be willing to write one?

Hi, there already are tests, which do not catch at least one case described in the k8s spec. I'll give it a try and try to work in parallel to what and how @zhangguanzhang solved it in July.

Cheers!

zhangguanzhang · 2020-12-17T01:36:33Z

try ro change https://github.com/containers/podman/blob/master/test/e2e/play_kube_test.go#L1248-L1267 to:

	It("podman play kube deployment more than 1 replica test correct command", func() {
		var i, numReplicas int32
		numReplicas = 5
		deployment := getDeployment(withReplicas(numReplicas))
		err := generateKubeYaml("deployment", deployment, kubeYaml)
		Expect(err).To(BeNil())

		kube := podmanTest.Podman([]string{"play", "kube", kubeYaml})
		kube.WaitWithDefaultTimeout()
		Expect(kube.ExitCode()).To(Equal(0))

		podNames := getPodNamesInDeployment(deployment)
		correctCmd := "[" + strings.Join(defaultCtrArg, " ") + "]"
		correctEntrypoint := defaultCtrCmd
		for i = 0; i < numReplicas; i++ {
			inspect := podmanTest.Podman([]string{"inspect", getCtrNameInPod(&podNames[i]), "--format", "'{{ .Config.Cmd }}'"})
			inspect.WaitWithDefaultTimeout()
			Expect(inspect.ExitCode()).To(Equal(0))
			Expect(inspect.OutputToString()).To(Equal(correctCmd))
			inspect = podmanTest.Podman([]string{"inspect", getCtrNameInPod(&podNames[i]), "--format", "'{{ .Config.Entrypoint }}'"})
			inspect.WaitWithDefaultTimeout()
			Expect(inspect.ExitCode()).To(Equal(0))
			Expect(inspect.OutputToString()).To(Equal(correctEntrypoint))
		}
	})

by the way,maybe not need so much tests

zhangguanzhang · 2020-12-17T01:38:30Z

test/e2e/play_kube_test.go

+		if err != nil {
+			panic(err)
+		}


Suggested change

if err != nil {

panic(err)

}

Expect(err).To(BeNil())

zhangguanzhang · 2020-12-17T01:39:16Z

test/e2e/play_kube_test.go

+		if err != nil {
+			panic(err)
+		}


Suggested change

if err != nil {

panic(err)

}

Expect(err).To(BeNil())

zhangguanzhang · 2020-12-17T01:41:25Z

and need rebase the commits to one commit

cruwe · 2020-12-17T13:04:31Z

@zhangguanzhang : Expect(err).To(BeNil()) is much nicer, thanks.

I changed the deployment test to test replica count. The idea is, that pod structure has already been tested at pod level, so at deployment level, I suggest to test what's new / different there and that's replica count.

cruwe

This did cause the yaml I exported using the podman generate kube command to be unable to deploy using the podman play kube command.
@daiaji: Could you give an example? I'd like to have a look.

cruwe · 2020-12-16T19:45:01Z

test/e2e/play_kube_test.go

@@ -1349,6 +1358,8 @@ spec:
 	})

 	// Deployment related tests
+	// FIXME: these just double the pod tests, replacing pod with deplotment
+	// FIXME: what's the point?


The deployment tests just mirror the tests for pods. Are these really necessary? I'd just omit them - is there any case why a deployment (containing a pod) should behave differently than an pod?

zhangguanzhang · 2020-12-21T01:41:42Z

test/e2e/play_kube_test.go

+		Expect(containerCmd).NotTo(Equal(imgCmd))
+
+		Expect(containerEntrypoint).To(Equal(strings.Join(overrideCmd, " ")))
+		Expect(containerCmd).To(Equal(nilArray))


a nit

Suggested change

Expect(containerCmd).To(Equal(nilArray))

Expect(len(containerCmd)).To(Equal(0))

zhangguanzhang · 2020-12-21T12:17:49Z

/retest

bziemons · 2020-12-21T14:59:57Z

pkg/specgen/generate/kube/kube.go

+	if len(containerYAML.Command) == 0 && len(containerYAML.Args) == 0 {
+		s.Entrypoint = imageData.Config.Entrypoint
+		s.Command = imageData.Config.Cmd
+	}
+
+	if len(containerYAML.Command) != 0 && len(containerYAML.Args) == 0 {
+		s.Entrypoint = containerYAML.Command
+		s.Command = nil
 	}
+
+	if len(containerYAML.Command) == 0 && len(containerYAML.Args) != 0 {
+		s.Entrypoint = imageData.Config.Entrypoint
+		s.Command = containerYAML.Args
+	}
+
+	if len(containerYAML.Command) != 0 && len(containerYAML.Args) != 0 {
+		s.Entrypoint = containerYAML.Command
+		s.Command = containerYAML.Args
+	}


Hi! can we boil this down to a simple "set entrypoint/command from imageData and override from containerYAML when set" ? I haven't seen this PR before I started fixing this and this would be my patch of this file: https://github.com/containers/podman/compare/master...bziemons:patch-kube-play-8428.patch

I also want to emphasize that the if imageData != nil && imageData.Config != nil { above is probably there for a reason and accessing imageData.Config without it is probably not a good idea.

agree with @bziemons, line: 132,142 need the the imageData != nil && imageData.Config != nil

Hi! can we boil this down to a simple "set entrypoint/command from imageData and override from containerYAML when set" ? I haven't seen this PR before I started fixing this and this would be my patch of this file: https://github.com/containers/podman/compare/master...bziemons:patch-kube-play-8428.patch

Yes, we can do that. It is functionally equivalent and less code, but it lacks the clarity in mirroring the k8s specs that I wished to express. I feel that your proposition warrants some line of comments to establish that link, but apart from that, I wouldn't complain.

I also want to emphasize that the if imageData != nil && imageData.Config != nil { above is probably there for a reason and accessing imageData.Config without it is probably not a good idea.

I fear I lost the context here. Are you referring to L115 before setting s.WorkDir = imageData.Config.WorkingDir? I did not notice that, but I supsect it is not necessary, because imageData is populated with imageData, err := newImage.Inspect(ctx) at L110. Any failure to get the imageData returns there.

In addition, I cannot imagine how to start any container without having both imageData and imageData.Config. Accordingly, the corresponding check, if it really is necessary, is misplaced when setting single values and should be performed at the "global" level somewhar at libpod/image/image.go, Inspect or when examining the return value of it's invocation at L110.

I also want to emphasize that the if imageData != nil && imageData.Config != nil { above is probably there for a reason and accessing imageData.Config without it is probably not a good idea.

I fear I lost the context here

Ok, now I get it. That part, if it is really necessary, about which I am yet not sure, should error much higher in the chain.

I agree with you and the if should have gotten a comment from its writer about why it is needed. Even if it isn't needed it wouldn't make sense to make the code more confusing about it (I know that wasn't your intention ;) ). Thanks for accepting my feedback though

More specifically, - use both `ENTRYPOINT` and `CMD` from image when neither `command` nor `args` have been supplied the pod spec, - use the `command` and only the `command` when `command` has been passed in the pod spec, ignoring both `ENTRYPOINT` and `CMD` from the image, - run the `ENTRYPOINT` from the image with `args` from the pod spec when only `args` have been passed and - override both when both `command` and `args` are present in the spec. Necessarily, - adapt pod tests to test the k8s spec documented at the k8s web-docs, - remove assertions already tested at pod level from deployments and - shift focus to replica count when testing deployments. In passing, - remove unnecessary `s.Command` initialization. Signed-off-by: Christopher J. Ruwe <[email protected]>

haircommander · 2020-12-22T18:17:42Z

An alternative fix is here #8807 (I totally didn't see this before implementing it, sorry if I have stepped on some toes,,,)

cruwe · 2020-12-23T12:11:55Z

Fine. I am really disappointed how this has been handled.

rhatdan · 2020-12-23T13:20:36Z

@cruwe Thanks for your PR. I am sorry about the conflicting PRs.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2020

jwhonce requested a review from baude December 9, 2020 18:24

zhangguanzhang reviewed Dec 10, 2020

View reviewed changes

cruwe force-pushed the cjr/fix-entrypoint-cmd-from-image branch from 38e0811 to 60dd501 Compare December 10, 2020 07:33

cruwe commented Dec 10, 2020

View reviewed changes

cruwe force-pushed the cjr/fix-entrypoint-cmd-from-image branch from 60dd501 to e290287 Compare December 14, 2020 10:15

cruwe marked this pull request as draft December 14, 2020 10:36

cruwe changed the title ~~WIP: podman play should honour both ENTRYPOINT and CMD from image~~ podman play should honour both ENTRYPOINT and CMD from image Dec 14, 2020

cruwe marked this pull request as ready for review December 14, 2020 10:36

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2020

cruwe force-pushed the cjr/fix-entrypoint-cmd-from-image branch from e290287 to ec70d33 Compare December 15, 2020 14:35

cruwe force-pushed the cjr/fix-entrypoint-cmd-from-image branch 2 times, most recently from 64880f3 to c778388 Compare December 16, 2020 19:43

zhangguanzhang reviewed Dec 17, 2020

View reviewed changes

cruwe force-pushed the cjr/fix-entrypoint-cmd-from-image branch from c778388 to 06fabdb Compare December 17, 2020 13:00

cruwe commented Dec 17, 2020

View reviewed changes

This was referenced Dec 20, 2020

Podman 2.2.1 rootless: cannot play podman-generated YAML file #8710

Closed

Use entrypoint and cmd for play kube #8789

Closed

zhangguanzhang reviewed Dec 21, 2020

View reviewed changes

PinkJohnOfUs mentioned this pull request Dec 21, 2020

podman play kube bin/sh syntax error #8428

Closed

bziemons reviewed Dec 21, 2020

View reviewed changes

cruwe force-pushed the cjr/fix-entrypoint-cmd-from-image branch from 06fabdb to 950af8a Compare December 21, 2020 21:45

cruwe mentioned this pull request Dec 22, 2020

podman play kube (rootless) does not set command and args as specfied at the k8s specs #8803

Closed

cruwe force-pushed the cjr/fix-entrypoint-cmd-from-image branch from 950af8a to bcda8e9 Compare December 22, 2020 15:22

cruwe closed this Dec 23, 2020

cruwe deleted the cjr/fix-entrypoint-cmd-from-image branch December 23, 2020 12:12

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 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

		s.Command = nil
		s.Command = append(s.Command, containerYAML.Args...)

	Expect(containerCmd).To(Equal(nilArray))
	Expect(len(containerCmd)).To(Equal(0))

podman play should honour both ENTRYPOINT and CMD from image #8666

podman play should honour both ENTRYPOINT and CMD from image #8666

Conversation

cruwe commented Dec 9, 2020

openshift-ci-robot commented Dec 9, 2020

mheon commented Dec 9, 2020

cruwe commented Dec 9, 2020 • edited Loading

cruwe commented Dec 9, 2020

mheon commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangguanzhang commented Dec 10, 2020

cruwe commented Dec 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangguanzhang commented Dec 10, 2020

cruwe commented Dec 14, 2020

zhangguanzhang commented Dec 14, 2020 • edited Loading

cruwe commented Dec 14, 2020

cruwe commented Dec 15, 2020

mheon commented Dec 15, 2020

daiaji commented Dec 15, 2020

cruwe commented Dec 15, 2020

mheon commented Dec 15, 2020

cruwe commented Dec 16, 2020

zhangguanzhang commented Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangguanzhang commented Dec 17, 2020

cruwe commented Dec 17, 2020

cruwe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangguanzhang commented Dec 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cruwe Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Dec 22, 2020 • edited Loading

cruwe commented Dec 23, 2020 • edited Loading

rhatdan commented Dec 23, 2020

cruwe commented Dec 9, 2020 •

edited

Loading

zhangguanzhang commented Dec 14, 2020 •

edited

Loading

zhangguanzhang commented Dec 17, 2020 •

edited

Loading

cruwe Dec 22, 2020 •

edited

Loading

haircommander commented Dec 22, 2020 •

edited

Loading

cruwe commented Dec 23, 2020 •

edited

Loading