Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Make transitions optional #2068

Merged
merged 2 commits into from
May 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,12 @@ like:
Unable to load package for //:WORKSPACE: BUILD file not found in any of the following directories.
```

* rules_docker uses transitions to build your containers using toolchains the correct
architecture and operating system. If you run into issues with toolchain resolutions,
you can disable this behaviour, by adding this to your .bazelrc:
```
build --@io_bazel_rules_docker//transitions:enable=false
```
## Using with Docker locally.

Suppose you have a `container_image` target `//my/image:helloworld`:
Expand Down Expand Up @@ -635,7 +641,7 @@ nodejs_image(
name = "nodejs_image",
entry_point = "@your_workspace//path/to:file.js",
# npm deps will be put into their own layer
data = [":file.js", "@npm//some-npm-dep"],
data = [":file.js", "@npm//some-npm-dep"],
...
)
```
Expand Down Expand Up @@ -1150,7 +1156,7 @@ load("@io_bazel_rules_docker//toolchains/docker:toolchain.bzl",

docker_toolchain_configure(
name = "docker_config",
# Replace this with a Bazel label to the config.json file. Note absolute or relative
# Replace this with a Bazel label to the config.json file. Note absolute or relative
# paths are not supported. Docker allows you to specify custom authentication credentials
# in the client configuration JSON file.
# See https://docs.docker.com/engine/reference/commandline/cli/#configuration-files
Expand Down
6 changes: 3 additions & 3 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,9 @@ http_archive(

http_archive(
name = "ubuntu1604",
sha256 = "aa772738311761ca9bbd94a507d15e78a81755c885641a5a3523a9b3eecb5516",
strip_prefix = "base-images-docker-01267e68a505f32188553a2706bd7096bd9ea6f5/ubuntu1604",
urls = ["https://github.com/GoogleContainerTools/base-images-docker/archive/01267e68a505f32188553a2706bd7096bd9ea6f5.tar.gz"],
sha256 = "e7e4c84f99df99f2a616d5328c41b5237c57c2550b0bd95a3de3888ad595e724",
strip_prefix = "base-images-docker-49320166744d93e4ca7754bc192484f9ce8c1686/ubuntu1604",
urls = ["https://github.com/GoogleContainerTools/base-images-docker/archive/49320166744d93e4ca7754bc192484f9ce8c1686.tar.gz"],
)

http_archive(
Expand Down
17 changes: 14 additions & 3 deletions container/image.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,15 @@ _outputs["config_digest"] = "%{name}.json.sha256"
_outputs["build_script"] = "%{name}.executable"

def _image_transition_impl(settings, attr):
return dicts.add(settings, {
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",
Copy link

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

@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.

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@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

Copy link

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@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

Choose a reason for hiding this comment

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

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 {}

Choose a reason for hiding this comment

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

@cfife-btig Yes, I agree.

}

return {
"//command_line_option:platforms": "@io_bazel_rules_docker//platforms:image_transition",
"@io_bazel_rules_docker//platforms:image_transition_cpu": "@platforms//cpu:" + {
# Architecture aliases.
Expand All @@ -779,11 +787,14 @@ def _image_transition_impl(settings, attr):
"ppc64le": "ppc",
}.get(attr.architecture, attr.architecture),
"@io_bazel_rules_docker//platforms:image_transition_os": "@platforms//os:" + attr.operating_system,
})
}

_image_transition = transition(
implementation = _image_transition_impl,
inputs = [],
inputs = [
"@io_bazel_rules_docker//transitions:enable",
"//command_line_option:platforms",
],
outputs = [
"//command_line_option:platforms",
"@io_bazel_rules_docker//platforms:image_transition_cpu",
Expand Down
78 changes: 78 additions & 0 deletions tests/container/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ load("//contrib:test.bzl", "container_test")
load(":apple.bzl", "create_banana_directory")
load(":empty_layers.bzl", "empty_layers")
load(":pull_info_validation_test.bzl", "pull_info_validation_test")
load(":transitions.bzl", "templated_file", "transition_test")

package(default_visibility = ["//visibility:public"])

Expand Down Expand Up @@ -999,3 +1000,80 @@ container_bundle(
"localhost:5000/image4:latest": "//testdata:with_double_label",
},
)

genrule(
name = "got_os",
outs = ["got_os.txt"],
cmd = select({
"@platforms//os:windows": "echo windows > \"$@\"",
"@platforms//os:linux": "echo linux > \"$@\"",
"@platforms//os:macos": "echo macos > \"$@\"",
}),
)

genrule(
name = "got_arch",
outs = ["got_arch.txt"],
cmd = select({
"@platforms//cpu:arm64": "echo arm64 > \"$@\"",
"@platforms//cpu:aarch64": "echo arm64 > \"$@\"",
"@platforms//cpu:x86_64": "echo x86_64 > \"$@\"",
"@platforms//cpu:arm": "echo arm > \"$@\"",
}),
)

container_image(
name = "transitioned_image",
architecture = "arm64",
files = [
":got_arch",
":got_os",
],
operating_system = "windows",
)

templated_file(
name = "transitions_off",
out = "transitions_off.yaml",
substitutions =
select({
"@platforms//os:linux": ["%WANT_OS%=linux"],
"@platforms//os:macos": ["%WANT_OS%=macos"],
}) + select({
"@platforms//cpu:arm64": ["%WANT_ARCH%=arm64"],
"@platforms//cpu:aarch64": ["%WANT_ARCH%=arm64"],
"@platforms//cpu:x86_64": ["%WANT_ARCH%=x86_64"],
}),
template = "//tests/container/configs:transitions_off.yaml.tpl",
)

container_test(
name = "_transitions_on_test_base",
configs = ["//tests/container/configs:transitions_on.yaml"],
driver = "tar",
image = ":transitioned_image",
# marked manual, because it will only pass if transitions are enabled
# enabled or disabled
tags = ["manual"],
)

transition_test(
name = "transitions_on_test",
actual = ":_transitions_on_test_base",
transitions_enabled = True,
)

container_test(
name = "_transitions_off_test_base",
configs = ["transitions_off"],
driver = "tar",
image = ":transitioned_image",
# marked manual, because it will only pass if transitions are disabled
tags = ["manual"],
)

transition_test(
name = "transitions_off_test",
actual = ":_transitions_off_test_base",
transitions_enabled = False,
)
2 changes: 1 addition & 1 deletion tests/container/configs/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@
# See the License for the specific language governing permissions and
# limitations under the License.

exports_files(glob(["*.yaml"]))
exports_files(glob(["*.yaml"]) + glob(["*.yaml.tpl"]))
9 changes: 9 additions & 0 deletions tests/container/configs/transitions_off.yaml.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
schemaVersion: 2.0.0

fileContentTests:
- name: "validate architecture"
path: "/Files/got_arch.txt"
expectedContents: ["%WANT_ARCH%"]
- name: "validate os"
path: "/Files/got_os.txt"
expectedContents: ["%WANT_OS%"]
9 changes: 9 additions & 0 deletions tests/container/configs/transitions_on.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
schemaVersion: 2.0.0

fileContentTests:
- name: "validate architecture"
path: "/Files/got_arch.txt"
expectedContents: ["arm64"]
- name: "validate os"
path: "/Files/got_os.txt"
expectedContents: ["windows"]
77 changes: 77 additions & 0 deletions tests/container/transitions.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
"""
Rules for running tests with a specific value of the //transitions:enabled flag
"""

def _templated_file_impl(ctx):
out = ctx.outputs.out
ctx.actions.expand_template(
template = ctx.file.template,
output = out,
substitutions = dict([s.split("=", 1) for s in ctx.attr.substitutions]),
)
return DefaultInfo(
files = depset([out]),
)

# Replaces substitutions split on `=` in the template. Substitution is a list
# so that is usable with multiple selects,
templated_file = rule(
attrs = {
"template": attr.label(
mandatory = True,
allow_single_file = True,
),
"substitutions": attr.string_list(),
"out": attr.output(),
},
implementation = _templated_file_impl,
)

def _enable_transition_impl(settings, attr):
_ = settings
return {"@io_bazel_rules_docker//transitions:enable": attr.transitions_enabled}

_enable_transition = transition(
implementation = _enable_transition_impl,
inputs = [],
outputs = [
"@io_bazel_rules_docker//transitions:enable",
],
)

def _transitioned_test(ctx):
source_info = ctx.attr.actual[DefaultInfo]

# Bazel wants the executable to be generated by this rule, let's oblige by
# just copying the actual runner.
executable = None
if source_info.files_to_run and source_info.files_to_run.executable:
executable = ctx.actions.declare_file("{}_{}".format(ctx.file.actual.basename, "on" if ctx.attr.transitions_enabled else "off"))
ctx.actions.run_shell(
command = "cp {} {}".format(source_info.files_to_run.executable.path, executable.path),
inputs = [source_info.files_to_run.executable],
outputs = [executable],
)
return [DefaultInfo(
files = depset(ctx.files.actual),
runfiles = source_info.default_runfiles.merge(source_info.data_runfiles),
executable = executable,
)]

# Defines a test that runs with a specific value of the
# @io_bazel_rules_docker//transitions:enable flag.
transition_test = rule(
attrs = {
"actual": attr.label(
mandatory = True,
allow_single_file = True,
),
"transitions_enabled": attr.bool(),
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
},
cfg = _enable_transition,
test = True,
implementation = _transitioned_test,
)
34 changes: 34 additions & 0 deletions transitions/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Copyright 2022 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")

package(
default_visibility = ["//visibility:public"],
)

bool_flag(
name = "enable",
build_setting_default = True,
)

config_setting(
name = "enabled",
flag_values = {"@io_bazel_rules_docker//transitions:enable": "true"},
)

config_setting(
name = "disabled",
flag_values = {"@io_bazel_rules_docker//transitions:enable": "false"},
)