Need a way to add tags to container layers targets.

[atchouprakov]

We are adding tags=["no-cache"] to containers to avoid upload these to bazel remote cache. But we couldn't find a way to propagate tag to the layers targets.

[nlopezgi]

Hi @atchouprakov,
Thanks for opening this issue. Its possible that we are not properly propagating the tags attribute in all the rules in this repo (probably rules in layer.bzl - https://github.com/bazelbuild/rules_docker/blob/master/container/layer.bzl is not propagating it). It should be a matter of passing them through properly (Example PR: #534). I'd be happy to review a PR fixing this!

[ishikhman]

This will be fixed in Bazel 1.0, just add a flag --experimental_allow_tags_propagation to your build command.
Please see more details and report issues if there are any here: #8830

[sha1n]

@ishikhman I tried using --experimental_allow_tags_propagation after tagging some container_image and container_pull rules with no-cache and I do see that it affects the JoinLayers actions, but it definitely doesn't affect the generic GUNZIP actions that I believe are coming from SkyLib and are not specific to rules_docker.

Is this expected?

I tested with Bazel 1.0.0, 1.0.1 and 1.1.0.

CC @nlopezgi

[nlopezgi]

Hi @sha1n ,
No, I don't think this is expected, could you figure out if the actions are indeed in skylib and if so open an issue there? If it turns out otherwise we can see if there is action needed here. Let's reopen until we have more info.

[sha1n]

@nlopezgi I'm pretty sure the GUNZIP and GZIP action mnemonics I saw are the ones implemented here:

rules_docker/skylib/zip.bzl

Line 24 in a98bc3b

mnemonic = "GZIP",

and here:

rules_docker/skylib/zip.bzl

Line 37 in a98bc3b

mnemonic = "GUNZIP",

That file is a part of this repo, so I believe it's not really a part of the bigger skylib. I saw these downloading both in the terminal and I also ran bazel with --profile and opened it in chrome tracing to make sure that it is indeed downloading/uploading.

Let me know if you want me to collect more info.

[nlopezgi]

I'm not sure why those would not respect tags. We can either try to pass tags explicitly (I'll be happy to review a PR you send to do so), or you can open up an issue in bazel repo to try to figure out why thse actions in particular are not working as others with the latest version of Bazel.

[sha1n]

I wouldn't mind sending a PR or comment on bazelbuild/bazel#8830, but I would love to hear what @ishikhman has to say. Just to make sure we're not missing anything.

[ishikhman]

I'm not sure why those would not respect tags. We can either try to pass tags explicitly (I'll be happy to review a PR you send to do so), or you can open up an issue in bazel repo to try to figure out why thse actions in particular are not working as others with the latest version of Bazel.

me neither - it seems like a bug, or a case that we missed during implementation.

@sha1n please do :)

[sha1n]

@ishikhman @nlopezgi I created a simple repository that contains a basic workspace and a readme that describes the issue and how to reproduce it. I included a chrome tracing profile I took on my machine with all the relevant details.

You can check it out here: https://github.com/sha1n/docker-no-remote-cache-issue-repro

@ishikhman I don't want to generate too much noise, so let me know if you want me to comment on bazelbuild/bazel#8830 as well.

Thanks!

[nlopezgi]

Hi @sha1n ,
please do comment on bazelbuild/bazel#8830 to get this noticed by someone that can take action to resolve.

[nlopezgi]

I just noticed this might be related to #1265. In both cases the behavior of the gzip actions is not the expected one. My running hypothesis is that local_tool does not work properly with tags. The solution is likely going to require folding in gzip into the docker toolchain, as is the case for xz (see https://github.com/bazelbuild/rules_docker/blob/master/toolchains/docker/toolchain.bzl#L27).

[darl]

btw GUNZIP spans (and corresponding cache operations) in profile come from container_pull rule, not a container_image

[nlopezgi]

oh, container_pull is a repo rule. I think that does not work with the tags mechanism at all.

[nlopezgi]

In any case, I created #1272 that should fix this for Starlark rules (not for repo rules). Please give it a try and see if that addresses your issue.

[sha1n]

Thanks @darl for spotting that!

@nlopezgi if that's indeed the case, then this has nothing to do with bazelbuild/bazel#8830 and --experimental_allow_tags_propagation apparently works as intended, so I want to verify the claim that repo rules do not work with tags and go from there.

Regarding #1272, what kind of test would you like me to run? It seems that tagging with no-remote-cache + the flag --experimental_allow_tags_propagation works, so do you want me to test without the flag?

For the sake of sharing, my current workaround is to use --modify_execution_info GUNZIP=+no-remote-cache,GZIP=+no-remote-cache, which is very aggressive, but it works well until a better solution is found..

Thank you guys!

[nlopezgi]

Ideally, with #1272, you should be able to set the no-remote-cache flags for GZIP/GUNZIP on the container_image target and not need to set it on the command line.

[sha1n]

@nlopezgi, sorry for getting back to the original issue again, but after talking to a colleague who has much deeper knowledge in Bazel, I think I understand what needs to be done in order to solve my issue properly and I want to try and explain it here.

According to my understanding, the container_pull repo rule generates a container_import rule, which is a normal Startlark rule and calls the gunzip/gzip actions for unzipping the layers' tar files when needed during a build. The problem is that users do not have any way of controlling the tags on the generated container_import rule.
I manually added a no-remote-cache tag to the generated container_import rule and run a build with --experimental_allow_tags_propagation and the GUNZIP actions run locally just as expected.
According to that, if we add an attribute to container_pull on which one can specify tags for the generated container_import rule, it should work perfectly.

In any case, I will try your patch and report back.

Thanks!

[nlopezgi]

thanks for the additional exploration @sha1n ,
while it is true that the container_import rule is generated and currently it is not possible to define tags that are propagated to that generated file, it should also be the case that for this generated rule the experimental_tags_propagation should work. So lets still give #1272 a try to see if that fixes the issue.

[sha1n]

Thanks @nlopezgi. I tested with #1272 and created a separate branch on my repro repository with the relevant changes and an updated trace profile- it doesn't seem to work. The container_image rule has the tag, as it did before and I still see the GUNZIP action downloading from the cache.

You can check it out here: https://github.com/sha1n/docker-no-remote-cache-issue-repro/tree/docker_rules_version_from_pr_1272

I'm not sure what you meant regarding --experimental_tags_propagation, but my tests showed that it should work if the generated container_import rule carries the no-remote-cache tag. Moreover, it doesn't work without --experimental_tags_propagation.

[nlopezgi]

Thanks for the additional exploration. It does seem then like we do need to have some way to pipe down the tags to the generated container_import target from container_pull. Happy to work with you to review a PR to enable this.

[Qinusty]

Can this now be closed?