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 apiv2 will create containers with incorrect commands #7409

Conversation

zhangguanzhang
Copy link
Collaborator

@zhangguanzhang zhangguanzhang commented Aug 22, 2020

Fixes: #7235 (comment)

$  podman inspect docker.io/zhangguanzhang/travis --format '{{.Config.Entrypoint}}'
[travis]

use the apiv2 to create the container

$  podman system  service --time 0 tcp:127.0.0.1:8081 &
$ curl http://localhost:8081/v1.40/containers/create?name=test-api2 -X POST \
  -H "Content-Type: application/json" \
  --data '{"Image":"docker.io/zhangguanzhang/travis","Cmd":["echo","param1"]}'

check the output

$ curl -s http://localhost:8081/v1.40/containers/test-api2/json | jq .Path
"" <--- wrong, should be `travis`
$ curl -s http://localhost:8081/v1.40/containers/test-api2/json | jq .Args
null <--- wrong, should be `["echo","param1"]`

and run command is incorrect

$ podman start test-api2
test-api2
$ podman logs test-api2
param1

if not use apiv2 to run this

$ podman container create --name cli-run  docker.io/zhangguanzhang/travis echo param1
$ podman start cli-run
cli-run
$ podman logs cli-run
unknown command echo <-- it will run `travis echo param1`, it is correct

after fix

$ ./bin/podman system  service --time 0 tcp:127.0.0.1:8081 &
$ curl http://localhost:8081/v1.40/containers/create?name=test-api2 \
  -X POST -H "Content-Type: application/json"\
   --data '{"Image":"docker.io/zhangguanzhang/travis","Cmd":["echo","param1"]}'
$ curl -s http://localhost:8081/v1.40/containers/test-api2/json | jq .Config.Entrypoint
[
  "travis"
]
$ curl -s http://localhost:8081/v1.40/containers/test-api2/json | jq .Config.Cmd
[
  "echo",
  "param1"
]
$ curl -s http://localhost:8081/v1.40/containers/test-api2/json | jq .Path
"travis"
$ curl -s http://localhost:8081/v1.40/containers/test-api2/json | jq .Args
[
  "echo",
  "param1"
]
$ 

Signed-off-by: zhangguanzhang [email protected]

@TomSweeneyRedHat
Copy link
Member

The tests are failing with a real failure.

if c.config.Command == nil {
// If the user only sets the entrypoint
// should ignore the image's command
if c.config.Entrypoint == nil && c.config.Command == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct - we should not use image command if entrypoint was set by the user. This check needs to be above the c.config.Entrypoint == nil check about to make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct - we should not use image command if entrypoint was set by the user. This check needs to be above the c.config.Entrypoint == nil check about to make sense.

thanks for reminding

if c.config.Command == nil {
// If the user only sets the entrypoint
// should ignore the image's command
if c.config.Entrypoint == nil && c.config.Command == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I've not had a enough tea today, but the comment doesn't seem to match the code? I'm not sure we want this change....

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or to put it another way, we only consider setting Cmd when entrypoint is not set by the user

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, maybe we could shorten lines 486->488 to:

Set CMD in the container to the default configuration only if ENTRYPOINT is not set by the user.

I think that better describes it, thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It had done,but I only changed the comment. after this, the ci reported an error. . .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've restarted the test. I think it's an intermittent CI error that @edsantiago has been chasing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@zhangguanzhang zhangguanzhang force-pushed the apiv2-create-ctr-with-invalid-entrypoint branch from 276e69f to 88ed130 Compare August 22, 2020 14:15
@zhangguanzhang
Copy link
Collaborator Author

zhangguanzhang commented Aug 22, 2020

The general idea is fine, but there are some detailed problems in the CI, I will try to fix them

@zhangguanzhang zhangguanzhang force-pushed the apiv2-create-ctr-with-invalid-entrypoint branch from 88ed130 to 41a712f Compare August 22, 2020 14:24
@zhangguanzhang
Copy link
Collaborator Author

@mheon @TomSweeneyRedHat PTAL

@rhatdan
Copy link
Member

rhatdan commented Aug 23, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, zhangguanzhang

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2020
@zhangguanzhang zhangguanzhang force-pushed the apiv2-create-ctr-with-invalid-entrypoint branch from 41a712f to fa6ba68 Compare August 24, 2020 15:07
@zhangguanzhang
Copy link
Collaborator Author

I only changed the comment, but ci reported an error. . .

@QiWang19
Copy link
Contributor

LGTM

@QiWang19
Copy link
Contributor

@mheon @TomSweeneyRedHat PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM, but would like @mheon to review to ensure his concern was addressed.

@TomSweeneyRedHat
Copy link
Member

and happy green test buttons fwiw.

@mheon
Copy link
Member

mheon commented Aug 26, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2020
@openshift-merge-robot openshift-merge-robot merged commit f99954c into containers:master Aug 26, 2020
@zhangguanzhang zhangguanzhang deleted the apiv2-create-ctr-with-invalid-entrypoint branch August 28, 2020 01:59
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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 this pull request may close these issues.

[APIv2] Create container does not initialize WorkingDir and few other things
7 participants