Reexecute stacker build inside lxc-usernsexec, do not reexecute inter… by eusebiu-constantin-petu-dbk · Pull Request #137 · project-stacker/stacker

eusebiu-constantin-petu-dbk · 2021-03-04T14:05:42Z

…nal go commands, closes #111

tych0 · 2021-03-04T23:35:39Z

cmd/build.go

+			return err
+		}

+		cmd := []string{


Here instead of duplicating all the arguments, can we replace os.Args[0] and add --internal-userns as Args[1] instead? This way when people add new arguments they won't also have to change this code.

Sure, haven't thought about this way. Thanks!

tych0 · 2021-03-04T23:36:13Z

cmd/build.go

+		return err
+	}
 	builder := stacker.NewBuilder(&args)
 	return builder.BuildMultiple([]string{ctx.String("stacker-file")})


How does this work if someone invokes recursive-build?

Yes, I forgot about recursive builds. Thank you!

tych0 · 2021-03-04T23:36:48Z

container/container.go

 	return nil
 }

+func Run(userCmd []string, msg string) error {


Is this used anywhere?

Will remove it, thanks

tych0 · 2021-03-04T23:41:04Z

Overall this looks great, thanks for working on it. If nothing else, it lets us get rid of 150 lines of terrible re-exec code!

A couple of small nits above and unfortunately, I added another re-exec command today, so this will need a rebase to account for that too.

Finally, it seems like we could split this into two commits: 1. adding the global re-exec and 2. removing the use of internal_go.go which may be easier for future readers/reviewers.

eusebiu-constantin-petu-dbk · 2021-03-05T17:01:24Z

Thanks for the review, I have updated it according to your review, I hope I understand correctly the split commit part, if not, I can update it again.

Also, I'm not sure if I have to remove the whole internal-go.go file, or we leave the test commands there, like I did.

Thanks again,
Sebi

tych0 · 2021-03-05T18:01:49Z

The code itself looks good, but I'm not seeing it actually use the --internal-userns flag anywhere as far as I can tell:

tycho    3527831  0.0  0.0  14980 10928 pts/3    Ss   09:51   0:00          \_ -bash
tycho    3528576 13.4  0.0 1392936 22688 pts/3   Sl+  09:58   0:00              \_ stacker --storage-type=overlay build
tycho    3528641  0.0  0.0  24576  5180 ?        Ss   09:58   0:00                  \_ /home/tycho/stacker/stacker internal test /tmp/foo/roots /tmp/stacker_test_run581706474
tycho    3528642  0.0  0.0   1340     4 ?        Ss   09:58   0:00                      \_ init
tycho    3528647  0.0  0.0  11896  2852 ?        Ss   09:58   0:00                          \_ /bin/sh -xe /stacker/.stacker-run.sh
tycho    3528648  0.0  0.0  23028  1408 ?        S    09:58   0:00                              \_ /usr/bin/coreutils --coreutils-prog-shebang=sleep /usr/bin/sleep 200s

with the stacker file:

test:
    from:
        type: oci
        url: base:centos
    run: |
        sleep 200s

I would expect to see another subprocess in the middle there with an --internal-userns argument somewhere?

It might be useful to add the testcase from #116 (comment) in another commit in this series, I think that will ensure that things work correctly.

tych0 · 2021-03-05T18:03:18Z

One other nit: the subject line of the commits is pretty long (and thus rendered strangely by github); I find it is generally worth following the 50 character limit (with a 72 char limit for the message body). More details here: https://chris.beams.io/posts/git-commit/

eusebiu-constantin-petu-dbk · 2021-03-08T11:08:18Z

Hello, this is my output, I can't understand why it behaves like this on your machine:

 2000 pts/0    Ss     0:00  |   \_ bash
 2554 pts/0    Sl+    0:00  |   |   \_ ./stacker --debug build -f tycho.yaml
 2559 pts/0    S+     0:00  |   |       \_ lxc-usernsexec -m u:0:1000:1 -m u:1:100001:65535 -m g:0:1000:1 -m g:1:100001:65535 -- /home/peusebiu/unpriv/stacker --internal-userns --debug build -f tycho.yam
 2560 pts/0    Sl+    0:04  |   |           \_ /home/peusebiu/unpriv/stacker --internal-userns --debug build -f tycho.yaml
 2577 ?        Ss     0:00  |   |               \_ /home/peusebiu/unpriv/stacker internal test /home/peusebiu/unpriv/roots /tmp/stacker_test_run289777360
 2580 ?        Ss     0:00  |   |                   \_ init
 2581 ?        Ss     0:00  |   |                       \_ /bin/sh -xe /stacker/.stacker-run.sh
 2582 ?        S      0:00  |   |                           \_ /usr/bin/coreutils --coreutils-prog-shebang=sleep /usr/bin/sleep 200s

I will add the test and modify the comment. Thanks.

eusebiu-constantin-petu-dbk · 2021-03-08T12:13:06Z

Can you please run @test "clean of unpriv overlay works", because I don't have a >5.8 kernel, and I'm not sure it ran in the CI:

ok 24 clean of unpriv overlay works # skip need newer kernel for unpriv overlay

tych0 · 2021-03-09T15:10:51Z

Yeah, I think the check for overlay kernels is actually broken by this patch somehow. I'll take a look.

I think it's worth adding the clean test in a separate commit (or perhaps the first commit, with a closes #xyz for the bug it came from) since hopefully does fix that bug.

tych0 · 2021-03-09T15:27:57Z

Yeah, I think the check for overlay kernels is actually broken by this patch somehow.

Looks like it's this hunk:

@@ -193,7 +50,7 @@ func doTestsuiteCheckOverlay(ctx *cli.Context) error {
                return errors.Wrapf(err, "couldn't make rootfs dir for testsuite check")
        }
 
-       err = container.RunInternalGoSubcommand(config, []string{"check-overlay"})
+       err = overlay.CanDoOverlay(config)
        if err != nil {
                log.Infof("%s", err)
                os.Exit(50)

This test still needs to be run inside a userns, as only root in a userns owning the mountns can do a mount().

Before running stacker builds commands, setup a user namespace with lxc-usernsexec and rerun the stacker builds commands given on the command line. This way everything will run in a user namespace therefore we can reexec internal-go commands without setting up a user namespace.

No need to reexec internal-go functions as they are already run in the global user namespace. Refactored them into their corresponding files. Added a test case for cleaning overlay builds.

tych0 · 2021-03-09T16:45:51Z

Ok, I pushed the fix for the testsuite-check-overlay command (and also dropped some unused stuff from the first patch). I also dropped the clean test from the series, since it doesn't actually fix things right now. I'm working on that now, though.

tych0 · 2021-03-09T21:39:31Z

Ok, I think with this last commit this is ready to go. @peusebiu thoughts? Assuming it looks good to you, we can go ahead and merge and see what happens :)

Various operations stacker can do (e.g. clean) rely on modifying/deleting files which are potentially owned by users in the current user's subuid allocation, which cannot be modified outside of a user namespace. To fix this, let's run all operations inside a user namespace, so that root in that userns can do whatever modifications it needs to do. Since we're now running chroot in a userns, we need to propagate stdin to the inner execution, hence that chunk of the diff. We can also drop the wrapper testsuite-check-overlay/check-overlay wrapper, since things are automatically run in a userns for us now. Closes: project-stacker#116 Closes: project-stacker#111 Signed-off-by: Tycho Andersen <tycho@tycho.pizza>

eusebiu-constantin-petu-dbk · 2021-03-10T08:56:44Z

Nice, I missed some obvious things, thanks for patching them :D.
Looks good to me. Let's see, hope for the best.

tych0 · 2021-03-10T14:45:17Z

No problem, thanks for your work on this!

eusebiu-constantin-petu-dbk requested review from andaaron and tych0 March 4, 2021 14:06

tych0 reviewed Mar 4, 2021

View reviewed changes

eusebiu-constantin-petu-dbk force-pushed the 111 branch 6 times, most recently from dd1080b to 5847040 Compare March 5, 2021 15:19

eusebiu-constantin-petu-dbk force-pushed the 111 branch from 5847040 to 52959e5 Compare March 8, 2021 11:39

eusebiu-constantin-petu-dbk added 2 commits March 9, 2021 08:36

Remove the use of internal-go.go

4c468d4

No need to reexec internal-go functions as they are already run in the global user namespace. Refactored them into their corresponding files. Added a test case for cleaning overlay builds.

tych0 force-pushed the 111 branch from 52959e5 to 4c468d4 Compare March 9, 2021 16:42

tych0 force-pushed the 111 branch 2 times, most recently from 0cb543b to 387c392 Compare March 9, 2021 23:05

tych0 merged commit fbf7850 into project-stacker:master Mar 10, 2021

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#11

Merged

Conversation

eusebiu-constantin-petu-dbk commented Mar 4, 2021

Uh oh!

tych0 Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

eusebiu-constantin-petu-dbk Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

tych0 Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

eusebiu-constantin-petu-dbk Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

tych0 Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

eusebiu-constantin-petu-dbk Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

tych0 commented Mar 4, 2021

Uh oh!

eusebiu-constantin-petu-dbk commented Mar 5, 2021

Uh oh!

tych0 commented Mar 5, 2021

Uh oh!

tych0 commented Mar 5, 2021

Uh oh!

eusebiu-constantin-petu-dbk commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eusebiu-constantin-petu-dbk commented Mar 8, 2021

Uh oh!

tych0 commented Mar 9, 2021

Uh oh!

tych0 commented Mar 9, 2021

Uh oh!

tych0 commented Mar 9, 2021

Uh oh!

tych0 commented Mar 9, 2021

Uh oh!

eusebiu-constantin-petu-dbk commented Mar 10, 2021

Uh oh!

tych0 commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eusebiu-constantin-petu-dbk commented Mar 8, 2021 •

edited

Loading