Skip to content

[release/1.1] Move ContainerFlags to "commands" package#2472

Merged
dmcgowan merged 1 commit intocontainerd:release/1.1from
stevvooe:backport-2411
Jul 31, 2018
Merged

[release/1.1] Move ContainerFlags to "commands" package#2472
dmcgowan merged 1 commit intocontainerd:release/1.1from
stevvooe:backport-2411

Conversation

@stevvooe
Copy link
Copy Markdown
Member

@stevvooe stevvooe commented Jul 17, 2018

Commit 0551328 exposed the "rootfs"
and "no-pivot" flags for the "containers" command, but it accidentally
removed them for "run" since package-level variables are initialized
before package-level init functions in golang. Hoisting these flags to
a package imported by both commands solves the problem.

Signed-off-by: Felix Abecassis [email protected]
(cherry picked from commit 5dd22a2)
Signed-off-by: Stephen Day [email protected]

@stevvooe stevvooe added this to the 1.1.3 milestone Jul 17, 2018
@dmcgowan dmcgowan changed the title Move ContainerFlags to "commands" package [release/1.1] Move ContainerFlags to "commands" package Jul 17, 2018
@stevvooe
Copy link
Copy Markdown
Member Author

stevvooe commented Jul 18, 2018 •
edited
Loading

For posterity, I thought it might be good to explain what happened here:

  1. 752d253 introduced the --rootfs flag, which was part of 1.0 as a boolean. It moved the flag to a list appended in an init function.
  2. The flag disappeared from 1.1 because 2c9ce2e references the collection before the init function runs, during data structure init.
  3. 5dd22a2 (cherry-picked here) accidentally fixed by changing the initialization order, such that when referenced in run.Command, it is fully initialized. The flag reappeared. It never made it to 1.1, which is why we are backporting.

There may be other places where this is happening. If we decide to control ctr, we'll need basic tests that check which flags are and are not exposed to prevent this from happening.

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Jul 18, 2018

Cherry-picking the fix seems reasonable. Will this also pull in anything else we don't currently support? Like gpus?

To get CI passing we need #2473

@stevvooe
Copy link
Copy Markdown
Member Author

stevvooe commented Jul 18, 2018

@dmcgowan No need to pull in feature changes. This was a clear regression for ctr, albeit it is “not supported”.

I’ll wait for #2473 and rebase.

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Jul 20, 2018

Guess I should have mentioned here, #2473 was merged and this can be rebased.

Related to the features, I wasn't suggesting we pull in a feature change I just wasn't sure if this was adding a new flag for a feature which does not exist in 1.1

@flx42 @stevvooe
Move ContainerFlags to "commands" package
4d7a305 
Commit 0551328 exposed the "rootfs"
and "no-pivot" flags for the "containers" command, but it accidentally
removed them for "run" since package-level variables are initialized
before package-level init functions in golang. Hoisting these flags to
a package imported by both commands solves the problem.

Signed-off-by: Felix Abecassis <[email protected]>
(cherry picked from commit 5dd22a2)
Signed-off-by: Stephen Day <[email protected]>
@stevvooe
Copy link
Copy Markdown
Member Author

stevvooe commented Jul 30, 2018

@dmcgowan Updated.

This won't pull in anything that was already intended to be there. None of this is supported anyways. ;)

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 30, 2018

Codecov Report

Merging #2472 into release/1.1 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/1.1    #2472   +/-   ##
============================================
  Coverage        50.11%   50.11%           
============================================
  Files               75       75           
  Lines             7297     7297           
============================================
  Hits              3657     3657           
  Misses            2965     2965           
  Partials           675      675
Flag Coverage Δ
#linux 50.11% <ø> (ø) ⬆️

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 d284b8c...4d7a305. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Jul 30, 2018

LGTM

1 similar comment
@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Jul 31, 2018

LGTM

@dmcgowan dmcgowan merged commit 4de979a into containerd:release/1.1 Jul 31, 2018
This was referenced Sep 27, 2018
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.1.3

Development

Successfully merging this pull request may close these issues.

6 participants

@stevvooe @dmcgowan @codecov-io @crosbymichael @Random-Liu @flx42