Skip to content

set args value of process if args is nil#2544

Merged
estesp merged 1 commit intocontainerd:masterfrom
kadisi:process_args
Aug 22, 2018
Merged

set args value of process if args is nil#2544
estesp merged 1 commit intocontainerd:masterfrom
kadisi:process_args

Conversation

@kadisi
Copy link
Copy Markdown
Contributor

@kadisi kadisi commented Aug 13, 2018

Signed-off-by: kadisi [email protected]

if i create container with this commond:

ctr c create -c ./ctr.json -t docker.io/library/busybox:latest busybox

and ctr.json content is :

{
  "ociVersion": "1.0.1",
  "process": {
    "terminal": true,
    "user": {
      "uid": 0,
      "gid": 0
    },
    "args": [
      "sleep",
      "100000"
    ],
    ... 
    ...
}

the args is "sleep", "100000"

and then start container
ctr task start busybox

i found the args in container status (/run/containerd/io.containerd.runtime.v1.linux/default/busybox/config.json) is sh

{
  "ociVersion": "1.0.1",
  "process": {
    "terminal": true,
    "user": {
      "uid": 0,
      "gid": 0
    },
    "args": [
      "sh"
    ],
   ...
}

i think if the args is not nil in the runtime-specific spec config file, we should use the spec config file args values , not image config`s cmd and entrypoint.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 13, 2018 •
edited
Loading

Codecov Report

Merging #2544 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2544   +/-   ##
=======================================
  Coverage   44.59%   44.59%           
=======================================
  Files          95       95           
  Lines       10007    10007           
=======================================
  Hits         4463     4463           
  Misses       4822     4822           
  Partials      722      722
Flag Coverage Δ
#linux 48.4% <ø> (ø) ⬆️
#windows 41.54% <ø> (ø) ⬆️
Impacted Files Coverage Δ
oci/spec_opts_unix.go 17.79% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f42445...7dae566. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Aug 13, 2018

This is a very specific case. I think we shouldn't run any Opts on the spec if the user is providing one via ctr. I think this change would be better in the ctr code, as it's specific to ctr usage.

@kadisi
Copy link
Copy Markdown
Contributor Author

kadisi commented Aug 14, 2018

@crosbymichael , but if the content of spec config file which the user provided is Incomplete， such as lack of env, capabilities.rlimits， but we still think it is ok ???

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Aug 14, 2018

if you provide a spec that is incomplete, then it's incomplete. The whole point of --config on run is to explicitly set the container's config because you know what you are doing and you don't want any modifications made to it.

@kadisi
Copy link
Copy Markdown
Contributor Author

kadisi commented Aug 15, 2018

@crosbymichael PTAL

crosbymichael
crosbymichael reviewed Aug 20, 2018
View reviewed changes
Comment thread oci/spec_opts_unix.go Outdated
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael Aug 20, 2018

Choose a reason for hiding this comment

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

can you remove the whitespace and then this LGTM

@kadisi
Copy link
Copy Markdown
Contributor Author

kadisi commented Aug 22, 2018

@crosbymichael PTAL, thank you

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Aug 22, 2018

LGTM

estesp
estesp approved these changes Aug 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 830363a into containerd:master Aug 22, 2018
@thaJeztah thaJeztah mentioned this pull request Aug 23, 2018
Merged
@lifubang lifubang mentioned this pull request Aug 27, 2018
Merged
@kadisi kadisi deleted the process_args branch March 14, 2019 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crosbymichael crosbymichael crosbymichael left review comments

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kadisi @codecov-io @crosbymichael @estesp