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

Add container_run_and_commit_layer rule #1586

Merged
merged 15 commits into from
Sep 23, 2020
Merged

Conversation

jamiees2
Copy link
Contributor

@jamiees2 jamiees2 commented Aug 8, 2020

The container_run_and_commit rule outputs an image, but sometimes all you want is the layer of changes to the root image.

We had a use-case that wasn't properly covered by container_commit_and_extract where we would like to obtain all the changes to the image file system, but don't have a fixed set of files that are modified. We'd like to be able to reuse the layer, which motivated the creation of this new rule.

We're also planning on replacing all our usages of container_run_and_commit with this rule. Internally, we mostly use it for creating custom base images, but using this rule we can get smaller outputs for that target, allowing us to better utilize our build cache.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jamiees2
Copy link
Contributor Author

jamiees2 commented Aug 8, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 8, 2020
@jamiees2
Copy link
Contributor Author

jamiees2 commented Aug 8, 2020

/assign @alex1545

@jamiees2
Copy link
Contributor Author

/assign @smukherj1

docker/util/run.bzl Outdated Show resolved Hide resolved
docker/util/README.md Outdated Show resolved Hide resolved
@huan086
Copy link

huan086 commented Aug 16, 2020

I've got the rule running, this rule is well and good.

Found a strange behaviour with container_image.

container_image(name = "to_run" ...)
container_run_and_commit_layer(name = "install", image = ":to_run.tar", ...)
container_image(name = "final", layers = [":install"], ...)

When I run the final target, I get a Docker image with both contents from to_run and final and the manifest.json in the image tar is

[
    {
        "Config": "redacted1.json",
        "RepoTags": [
            "bazel/stuff:to_run"
        ],
        ...
    },
    {
        "Config": "redacted2.json",
        "RepoTags": [
            "bazel/staff:final"
        ],
        ...
    }
]

If I skip the container_run_and_commit_layer rule, it similarly results in the both contents in the Docker image.

container_image(name = "to_run" ...)
container_image(name = "final", tars = [":to_run-layer.tar"], ...)

Do you face the same problem? If not, how does your build look like?

Edit: everything is well and good. The problem is that I've saved the Docker image with docker save bazel/stuff rather than docker save bazel/stuff:final 🤦‍♀️

@jamiees2
Copy link
Contributor Author

jamiees2 commented Sep 6, 2020

Hey @alex1545, @smukherj1, is there a chance of getting this reviewed sometime soon? :)

Copy link
Contributor

@alex1545 alex1545 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution and sorry for the delayed reply on this.
Can you also please add a simple test to exercise this new rule? Maybe a test that runs a command to add a new file and then confirming that the expected layer tar is generated containing the new file.

docker/util/commit_layer.sh.tpl Outdated Show resolved Hide resolved
contrib/extract_last_layer.py Outdated Show resolved Hide resolved
docker/util/run.bzl Outdated Show resolved Hide resolved
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jamiees2
Copy link
Contributor Author

Hey @alex1545, I've addressed the PR review comments and added a test to verify that the rule works correctly. Is this good the be merged?

Sorry about the CLA mess, I made a git mistake when merging the upstream master into my branch, which I force reverted.

@alex1545
Copy link
Contributor

Thank you for addressing the comments.
Can you please add your new test targets to the buildkite CI here, here and here.
Also please take a look as to why the test fails on RBE with this error:

ERROR: /workdir/tests/docker/util/BUILD:110:10: in _rule_test_rule rule //tests/docker/util:test_container_commit_layer_rule_impl:
--
  | Traceback (most recent call last):
  | File "/workdir/tests/docker/util/BUILD", line 110, column 10, in _rule_test_rule(name = 'test_container_commit_layer_rule_impl')
  | rule_test(
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/external/bazel_tools/tools/build_rules/test_rules.bzl", line 296, column 17, in _rule_test_rule_impl
  | fail("rule %s generates %s not %s" %
  | Error in fail: rule //tests/docker/util:test_container_commit_layer generates ["test_container_commit_layer-layer.tar"] not ["test_container_commit.build", "test_container_commit_commit-layer.tar"]

@jamiees2
Copy link
Contributor Author

Thanks, sorry about that. I've fixed the test and added the two new test targets to the CI.

@alex1545
Copy link
Contributor

Can you now please look at why this fails on Darwin with this error:

ERROR: /Users/buildkite/builds/bk-imacpro-8/bazel/rules-docker-docker/tests/docker/util/BUILD:100:31: Action tests/docker/util/test_container_commit_layer-layer.tar failed (Exit 1): test_container_commit_layer.build failed: error executing command
--
  | (cd /private/var/tmp/_bazel_buildkite/5e65cabdc7f99fb85376aa4a05c66e92/sandbox/darwin-sandbox/1034/execroot/io_bazel_rules_docker && \
  | exec env - \
  | PATH=/Users/buildkite/Library/Caches/bazelisk/downloads/bazelbuild/bazel-3.5.0-darwin-x86_64/bin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin \
  | bazel-out/darwin-fastbuild/bin/tests/docker/util/test_container_commit_layer.build)
  | Execution platform: @local_config_platform//:host
  |  
  | Use --sandbox_debug to see verbose messages from the sandbox test_container_commit_layer.build failed: error executing command
  | (cd /private/var/tmp/_bazel_buildkite/5e65cabdc7f99fb85376aa4a05c66e92/sandbox/darwin-sandbox/1034/execroot/io_bazel_rules_docker && \
  | exec env - \
  | PATH=/Users/buildkite/Library/Caches/bazelisk/downloads/bazelbuild/bazel-3.5.0-darwin-x86_64/bin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin \
  | bazel-out/darwin-fastbuild/bin/tests/docker/util/test_container_commit_layer.build)
  | Execution platform: @local_config_platform//:host
  |  
  | Use --sandbox_debug to see verbose messages from the sandbox
  | + source bazel-out/darwin-fastbuild/bin/tests/docker/util/image_util.sh
  | + DOCKER=/usr/bin/docker
  | + DOCKER_FLAGS=
  | + [[ -z /usr/bin/docker ]]
  | ++ bazel-out/host/bin/contrib/extract_image_id bazel-out/darwin-fastbuild/bin/external/debian_base/image/image.tar
  | + image_id=6786f6a45544215e9e4411db2318c7f561009b38fcf7c88f2f5f656256ba72f8
  | + /usr/bin/docker load -i bazel-out/darwin-fastbuild/bin/external/debian_base/image/image.tar
  | bazel-out/darwin-fastbuild/bin/tests/docker/util/test_container_commit_layer.build: line 19: /usr/bin/docker: No such file or directory
  | (16:53:46) ERROR: /Users/buildkite/builds/bk-imacpro-8/bazel/rules-docker-docker/tests/docker/util/BUILD:124:15 Action tests/docker/util/test_container_commit_layer-layer.tar failed (Exit 1): test_container_commit_layer.build failed: error executing command
  | (cd /private/var/tmp/_bazel_buildkite/5e65cabdc7f99fb85376aa4a05c66e92/sandbox/darwin-sandbox/1034/execroot/io_bazel_rules_docker && \
  | exec env - \
  | PATH=/Users/buildkite/Library/Caches/bazelisk/downloads/bazelbuild/bazel-3.5.0-darwin-x86_64/bin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin \
  | bazel-out/darwin-fastbuild/bin/tests/docker/util/test_container_commit_layer.build)
  | Execution platform: @local_config_platform//:host

@jamiees2
Copy link
Contributor Author

Oh I ran into this too when running the test locally, for me it was because of this line: https://github.com/bazelbuild/rules_docker/blob/master/WORKSPACE#L24

and my docker was at /usr/local/bin/docker. I changed that line temporarily so that I could run the test

@jamiees2
Copy link
Contributor Author

Ah I see, we also don't run the analogous test for container_run_and_commit - //tests/docker/util:test_container_commit_metadata either anywhere. For now, I'll remove it from OSX since it passed on other platforms, but lmk if you want it removed from the others as well.

@alex1545
Copy link
Contributor

/gcbrun

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alex1545, jamiees2

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

@alex1545 alex1545 merged commit 708e6c7 into bazelbuild:master Sep 23, 2020
@huan086
Copy link

huan086 commented Oct 21, 2020

I upgraded to docker-desktop 2.4.2.0-edge and this rule broke with the error

ERROR: /bazel/elasticsearch/BUILD.bazel:183:31: Action elasticsearch/install_s3_plugin-layer.tar failed (Exit 125) install_s3_plugin.build failed: error executing command bazel-out/k8-fastbuild/bin/elasticsearch/install_s3_plugin.build

Use --sandbox_debug to see verbose messages from the sandbox
+ source bazel-out/k8-fastbuild/bin/elasticsearch/image_util.sh
+ DOCKER=/usr/bin/docker
+ DOCKER_FLAGS=
+ [[ -z /usr/bin/docker ]]
++ bazel-out/host/bin/contrib/extract_image_id bazel-out/k8-fastbuild/bin/elasticsearch/for_plugin_install.tar
+ image_id=50beeeb4436cfc02b264d10dca5bbc6965d7af83e14ccd1feb54306812ea0db4
+ /usr/bin/docker load -i bazel-out/k8-fastbuild/bin/elasticsearch/for_plugin_install.tar
++ /usr/bin/docker run -d --env-file bazel-out/k8-fastbuild/bin/elasticsearch/install_s3_plugin.env 50beeeb4436cfc02b264d10dca5bbc6965d7af83e14ccd1feb54306812ea0db4 sh -c '/usr/share/elasticsearch/bin/elasticsearch-plugin install --batch repository-s3'
docker: Error response from daemon: invalid repository name (50beeeb4436cfc02b264d10dca5bbc6965d7af83e14ccd1feb54306812ea0db4), cannot specify 64-byte hexadecimal strings.
+ id=fea7686c274c8b7bc931589967a344c2dadf9fde3e5e5d95b4761e7c99485269
Loaded image: pbrain/elasticsearch:for_plugin_install

I have to fix it by adding sha256: in commit_layer.sh.tpl line 21

id=$($DOCKER $DOCKER_FLAGS run -d %{docker_run_flags} --env-file %{env_file_path} sha256:"$image_id" %{commands})

@aryeh-looker
Copy link

Is there a way to log what's happening while running?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants