Make transitions optional

[laurynaslubys]

Transitions, introduced in #1963 can now be disabled by calling bazel with --@io_bazel_rules_docker//transitions:enable=no.
Added tests to verify the behaviour.

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

What is the current behavior?

Transitions based on container_image attributes were always enabled.

Issue Number: N/A

What is the new behavior?

Transitions based on container_image attributes are enabled by default, but can be disabled by specifying --@io_bazel_rules_docker//transitions:enable=false

Does this PR introduce a breaking change?

• Yes
• No

Other information

[laurynaslubys]

Strange, //tests/docker/package_managers:install_pkgs_reproducibility_test fails in ci, but passes locally.

I'm not sure how that would be related to the changes in this PR. I'd appreciate any input on how to debug this.

[laurynaslubys]

I've updated the ubuntu base image, which caused the containers to rebuild and now it passes. Bazel probably had the targets cached built at different times.

[uhthomas]

Thanks for the contribution! Initially it seems sensible, I'll take a proper look tomorrow if that's okay 😄

[kriscfoster]

This is great & will help us a lot. Do we know when 0.25.0 will be released @uhthomas?

[cfife-btig]

Hey there @laurynaslubys, First of all thanks a bunch for making these changes and my team has been really interested in something to address some of the overhead issues created by transitions which we think is a nice feature, but mostly not relevant to us because are target and host are always the same . I wanted to mention 2 things:

1. I noticed this when I was trying to debug my own issue. But it looks like there is a typo here plaftorm instead of platform.
2. I think one of the main reasons my team is interested in this MR is because it could reduce the number of rebuilds. Since 0.23, we will basically have to rebuild/recompile our entire pipeline, even though our host and target are the same, because I think the transition settings get hashed into the built directory. (i.e. our bazel-out directory k8-fastbuild becomes k8-fastbuild-ST-<someHashHere.). I'm happy to open an issue, but I would really like if things could behave as before in rules_docker 0.22 where if you didn't explicitly specify something, it didn't create a new output directory for docker images.

I noticed that having the transitions:enable flag be false causing this function to return an empty JSON object works. But it sounds like your comment says maybe that's an issue for people below Bazel 5.0.

This is still a great change and my no means want to downplay it. But at least wanted to share these thoughts.

[scasagrande]

@cfife-btig Thank you for pointing this out. I spent several hours tracking down the exact same cache miss issue. Patching this to return an empty dict on 0.25.0 and turning transitions off has resolved my issue for now.

[abentley-ssimwave]

Hi @scasagrande ! I just tracked it down today, only our version of Bazel doesn't seem to be compatible with the empty dict solution, so I'm looking at 0.22.

[cfife-btig]

Just a dumb sanity check @abentley-ssimwave but are you setting io_bazel_rules_docker/transitions:enable=false on the command-line?

[scasagrande]

@abentley-ssimwave well hello!! Yes I should have included my bazel version, we were on 5.2 at time of my prior comment and we're now up to 5.3.1

[cfife-btig]

That sounds like what should happen. You need to both use the patch and set the flag transitions:enable=false. Sorry if this is redundant, but if you're not doing both of those things together then I don't think it will disable transitions.

If you only set the flag, it just sets the directory <myoutdir>-hash to <myoutdir>-somenewhash because removing those build settings are just generating a new transition hash (when it should really remove the transition has altogether), whereas in 0.22, I believe there were no transitions to begin with.

[abentley-ssimwave]

@cfife-btig Yes, I set --@io_bazel_rules_docker//transitions:enable=false you can see it in the output I posted.

[abentley-ssimwave]

@cfife-btig The result of using the flag with your patch on 3.4.1 is a whole pile of errors, and I do not agree that that "should" happen, although I can accept that there may not be a better solution for 0.25

[cfife-btig]

Hmmm. I'm not sure then. My guess would be your version of Bazel is the issue. I think I'm on 5.2

Basically the patch file returns an empty dict instead of unset, and i didn't write it but there's a comment implying it might not be supported in < 5.0 Bazel.

         # Once bazel < 5.0 is not supported we can return an empty dict here
-        return {
-            "//command_line_option:platforms": settings["//command_line_option:platforms"],
-            "@io_bazel_rules_docker//platforms:image_transition_cpu": "//plaftorms:image_transition_cpu_unset",
-            "@io_bazel_rules_docker//platforms:image_transition_os": "//plaftorms:image_transition_os_unset",
-        }
+        return {}

[abentley-ssimwave]

@cfife-btig Yes, I agree.

[cfife-btig]

Just FYI, even with this disabling transitions (which I'm not totally sure is actually happening due to the typo I commented about above). The "unset" value still gets hashed. So even with this change applied working, all this does is change K8-fastbuild-ST-someHash to K8-fastbuild-ST-someNewHash.

If you are trying to get docker rules to use the same bazel-out bins as the rest of your rules (so you have less rebuilding), your options are to either

A.) Switch back to rules_docker 0.22 (before transitions were added)
B.) Create and apply patch file to rules_docker 0.25 to return an empty dict as @scasagrande did. Note you still need to set the transitions:enable flag to false.

Here's the patch file I created from Git. I'm not guaranteeing you can copy/paste this, but it at least shows what needs to be changed.

0001-Remove-transition-settings.patch

index ceeea01..f2762d4 100644
--- a/container/image.bzl
+++ b/container/image.bzl
@@ -769,11 +769,7 @@ _outputs["build_script"] = "%{name}.executable"
 def _image_transition_impl(settings, attr):
     if not settings["@io_bazel_rules_docker//transitions:enable"]:
         # Once bazel < 5.0 is not supported we can return an empty dict here
-        return {
-            "//command_line_option:platforms": settings["//command_line_option:platforms"],
-            "@io_bazel_rules_docker//platforms:image_transition_cpu": "//plaftorms:image_transition_cpu_unset",
-            "@io_bazel_rules_docker//platforms:image_transition_os": "//plaftorms:image_transition_os_unset",
-        }
+        return {}
 
     return {
         "//command_line_option:platforms": "@io_bazel_rules_docker//platforms:image_transition",
-- 

[sushain97]

For folks who aren't particularly thrilled about patches or simply want to disable transitions in a limited capacity, rules_docker makes it relatively easy to create a custom container_image rule which excludes the transition entirely:

load("@io_bazel_rules_docker//container:container.bzl", "image")
load("@bazel_skylib//lib:dicts.bzl", "dicts")

container_image_without_transition = rule(
    attrs = dicts.omit(image.attrs, ["_allowlist_function_transition"]),
    executable = True,
    outputs = image.outputs,
    toolchains = ["@io_bazel_rules_docker//toolchains/docker:toolchain_type"],
    implementation = image.implementation,
)