Incremental load support for commit and extract.

[uri-canva]

container_run_and_commit and container_run_and_extract don't support the intermediate format because they were moved from https://github.com/GoogleContainerTools/base-images-docker.

This modifies them to support that.

See #1384.

[uri-canva]

/assign @alex1545

[uri-canva]

As I'm trying to use this patch in our repo I'm noticing there's a couple of incompatibilities between the way these and the other rules are used that is making the code very finnicky, I will smooth out the difference in a second commit, but the tradeoff for that is that the interface will be very incompatible. It might be worth having that as a separate PR.

[uri-canva]

Note this isn't ready to merge because it doesn't update all the other rules in the repo to use the new interface, but I'd like some initial feedback on the approach before continuing with that.

[smukherj1]

Taking a brief look, I feel like this change could be simplified a lot by keeping the existing commit.sh.tpl and extract.sh.tpl scripts and just making a couple of changes to the incremental_load.sh.tpl to support your changes. This way all the existing rules wouldn't need to be modified to pipe in the extra flags. I feel this mode of build and then maybe commit or extract and then run is making the semantics of container_image too complicated. Further, it goes against the general principle that rules in //container' never require the docker` executable.

[uri-canva]

Yes, that makes sense: use incremental load to just load the image, then run the separate script to do the extracting or committing.

I tried it as my first approach but I couldn't get it to work: templating it so the the two ended up into one script only required a lot of changes, and making it two different scripts was hard to make correct because it required expressing the side effect of having the image loaded as a side effect of the first script, rather than having an output produced as usual.

Let me try again now that I have a working approach, might be able to do something. I didn't know //container was supposed to not use docker, is that documented somewhere? Maybe there's other things I don't know which I should know.

[uri-canva]

Done, now with fewer changes in container/.

[uri-canva]

@smukherj1 What you do think of this approach?

[smukherj1]

This approach looks good! Could you please look into the CI failures (example here

I'm going to launch the GCB tests as well...

[smukherj1]

/gcbrun

[HackAttack]

Any update on this? I have a quite heavy image (50 GB of intermediate tarballs) built using the “build image with source layer, run_and_extract” pattern, and if I understand this issue correctly, this PR would eliminate those.

[uri-canva]

No this only cuts it in half: it removes the need for the tarballs on the read side, but it still produces tarballs on the write side. Sorry we've been using this patch for a while now and it's working fine, but I haven't had time to clean it up, I'll come back to it.

[uri-canva]

Also see my comment in #1384 (comment): it's unlikely we'll ever be able to remove the need for the tarball on the write side since docker does not expose any APIs to output layers, so we've been changing our usage of docker_rules to minimise use of container_run_and_* rules. It is possible to get very far with container_image rules and https://github.com/GoogleContainerTools/distroless/tree/54afbf9c067b0f35d8f5f3fbeae94b4daa2f5dbe/package_manager.

[gravypod]

So, this is a fun one. We have a circular dep here. The package install tests are attempting to see if we can reproducible install a package from apt into a container using docker run. To do this we depend on a package called ubuntu1604 which is downloaded from here. This package has a bazel workspace in it. It has this BUILD file under //ubuntu1604:

...
container_image(
    name = "ubuntu1604_vanilla",
    env = UBUNTU_ENV,
    tars = ["@ubuntu1604_tar//file"],
)
...

This change modifies the way container_image is implemented so it seems like we have some circular dep thing here. Not sure as to the cause but this is what I'd investigate.

This is done in this repo: https://github.com/GoogleContainerTools/base-images-docker

Maybe this is the culprit: https://github.com/GoogleContainerTools/base-images-docker/blob/master/WORKSPACE#L33-L38

[uri-canva]

TODO: the new code has a trap in output_logfile so it cannot be concatenated to incremental_load.sh since that has a trap too without handling multiple traps.

[uri-canva]

Turns out the test failure might not have been related to the PR at all:
#2068 (comment)
#2068 (comment)

but now I can't run the tests locally because of the transitions not working on macOS.

[uri-canva]

And we're back, this time //tests/docker/util:test_extracted_file is failing: https://buildkite.com/bazel/rules-docker-docker/builds/6193#4331b881-4955-47be-a9cf-f362e2f0f799/228-309.
I tried to run it in a container, with the docker socket mounted, but it couldn't resolve the toolchain through the transitions, can someone help me figure out what's wrong?

[laurynaslubys]

And we're back, this time //tests/docker/util:test_extracted_file is failing: https://buildkite.com/bazel/rules-docker-docker/builds/6193#4331b881-4955-47be-a9cf-f362e2f0f799/228-309. I tried to run it in a container, with the docker socket mounted, but it couldn't resolve the toolchain through the transitions, can someone help me figure out what's wrong?

Which version of bazel are you running? This repo seems to not work with 5.x. You can try 4.0.0 as that's the one used in CI.

[uri-canva]

Great catch, that was exactly the issue. I was able to run the test locally and it passes, so maybe it's another test caching / flakiness issue on CI?

[uri-canva]

TODO: the new code has a trap in output_logfile so it cannot be concatenated to incremental_load.sh since that has a trap too without handling multiple traps.

Done by removing those traps, the only thing that is leftover in each case is a file created with mktemp containing the log output so hopefully not a big deal since it should be removed by the tmp dir cleaner eventually anyway.

[uri-canva]

This is the error from the build:

bazel-out/k8-fastbuild-ST-6390b2712f3f/bin/tests/docker/util/test_container_extract.build: line 338: fg: no job control
bazel-out/k8-fastbuild-ST-6390b2712f3f/bin/tests/docker/util/test_container_extract.build: line 343: fg: no job control

not sure where it's coming from since I couldn't find any related fg in the repo, any of the dependencies or bazel itself.

[uri-canva]

ping @linzhp @uhthomas

[amir-f]

was this merged ? container_run_and_commit is really slow on large containers

[uri-canva]

It wasn't merged, but we're still using it internally. I stopped updating the PR because it wasn't getting any traction.

[uhthomas]

Sorry for the lack of activity. This seems like a useful change which I'm sure we'd be happy to add.

[uri-canva]

@lamcw you recently had to update this patch, can you update this PR with the changes? I've given you access to my fork.

[uri-canva]

Oh, can't reopen the PR off a recreated or force pushed branch, will have to open a new PR.

[uri-canva]

@uhthomas opened #2208.