Skip to content

Commit

Permalink
Make it possible to toggle cache key scrubbing by rule kind
Browse files Browse the repository at this point in the history
Add the target kind (e.g. java_library) to the scrubbing matcher configuration. This is useful because different kinds may use the same mnemonic, and specifying an explicit set of labels is labor-intensive and error-prone.

Closes #21151.

PiperOrigin-RevId: 605624773
Change-Id: Ib5748ca2dbb3fd6d30b27fc1b292d7cd826ced62
  • Loading branch information
bozaro authored and bazel-io committed Feb 9, 2024
1 parent c5f9c86 commit b471046
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 9 deletions.
12 changes: 9 additions & 3 deletions src/main/java/com/google/devtools/build/lib/remote/Scrubber.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.remote.RemoteScrubbing.Config;
Expand Down Expand Up @@ -104,6 +105,7 @@ public static class SpawnScrubber {

private final Pattern mnemonicPattern;
private final Pattern labelPattern;
private final Pattern kindPattern;
private final boolean matchTools;

private final ImmutableList<Pattern> omittedInputPatterns;
Expand All @@ -114,6 +116,7 @@ private SpawnScrubber(Config.Rule ruleProto) {
Config.Matcher matcherProto = ruleProto.getMatcher();
this.mnemonicPattern = Pattern.compile(emptyToAll(matcherProto.getMnemonic()));
this.labelPattern = Pattern.compile(emptyToAll(matcherProto.getLabel()));
this.kindPattern = Pattern.compile(emptyToAll(matcherProto.getKind()));
this.matchTools = matcherProto.getMatchTools();

Config.Transform transformProto = ruleProto.getTransform();
Expand All @@ -134,12 +137,15 @@ private String emptyToAll(String s) {
/** Whether this scrubber applies to the given {@link Spawn}. */
private boolean matches(Spawn spawn) {
String mnemonic = spawn.getMnemonic();
String label = spawn.getResourceOwner().getOwner().getLabel().getCanonicalForm();
boolean isForTool = spawn.getResourceOwner().getOwner().isBuildConfigurationForTool();
ActionOwner actionOwner = spawn.getResourceOwner().getOwner();
String label = actionOwner.getLabel().getCanonicalForm();
String kind = actionOwner.getTargetKind();
boolean isForTool = actionOwner.isBuildConfigurationForTool();

return (!isForTool || matchTools)
&& mnemonicPattern.matcher(mnemonic).matches()
&& labelPattern.matcher(label).matches();
&& labelPattern.matcher(label).matches()
&& kindPattern.matcher(kind).matches();
}

/** Whether the given input should be omitted from the cache key. */
Expand Down
4 changes: 4 additions & 0 deletions src/main/protobuf/remote_scrubbing.proto
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ option java_package = "com.google.devtools.build.lib.remote";
message Config {
// Describes a set of actions. An action must pass all criteria to match.
message Matcher {
// A regex matching the rule kind of the action owner.
// Use .* if a partial match is desired.
// If empty, matches every kind.
string kind = 4;
// A regex matching the canonical label of the action owner.
// Use .* if a partial match is desired.
// If empty, matches every label.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
public class FakeOwner implements ActionExecutionMetadata {
private final String mnemonic;
private final String progressMessage;
@Nullable private final String ownerLabel;
private final String ownerLabel;
private final String ownerRuleKind;
@Nullable private final Artifact primaryOutput;
@Nullable private final PlatformInfo platform;
private final ImmutableMap<String, String> execProperties;
Expand All @@ -53,13 +54,15 @@ public class FakeOwner implements ActionExecutionMetadata {
String mnemonic,
String progressMessage,
String ownerLabel,
String ownerRuleKind,
@Nullable Artifact primaryOutput,
@Nullable PlatformInfo platform,
ImmutableMap<String, String> execProperties,
boolean isBuiltForToolConfiguration) {
this.mnemonic = mnemonic;
this.progressMessage = progressMessage;
this.ownerLabel = checkNotNull(ownerLabel);
this.ownerRuleKind = checkNotNull(ownerRuleKind);
this.primaryOutput = primaryOutput;
this.platform = platform;
this.execProperties = execProperties;
Expand All @@ -72,6 +75,7 @@ private FakeOwner(
mnemonic,
progressMessage,
ownerLabel,
/* ownerRuleKind= */ "dummy-target-kind",
/* primaryOutput= */ null,
platform,
ImmutableMap.of(),
Expand All @@ -87,7 +91,7 @@ public ActionOwner getOwner() {
return ActionOwner.createDummy(
Label.parseCanonicalUnchecked(ownerLabel),
new Location("dummy-file", 0, 0),
/* targetKind= */ "dummy-target-kind",
ownerRuleKind,
mnemonic,
/* configurationChecksum= */ "configurationChecksum",
new BuildConfigurationEvent(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public final class SpawnBuilder {
private String mnemonic = "Mnemonic";
private String progressMessage = "progress message";
private String ownerLabel = "//dummy:label";
private String ownerRuleKind = "dummy-target-kind";
@Nullable private Artifact ownerPrimaryOutput;
@Nullable private PlatformInfo platform;
private final List<String> args;
Expand Down Expand Up @@ -96,6 +97,7 @@ public Spawn build() {
mnemonic,
progressMessage,
ownerLabel,
ownerRuleKind,
ownerPrimaryOutput,
platform,
execProperties,
Expand Down Expand Up @@ -140,6 +142,12 @@ public SpawnBuilder withOwnerLabel(String ownerLabel) {
return this;
}

@CanIgnoreReturnValue
public SpawnBuilder withOwnerRuleKind(String ownerRuleKind) {
this.ownerRuleKind = checkNotNull(ownerRuleKind);
return this;
}

@CanIgnoreReturnValue
public SpawnBuilder withOwnerPrimaryOutput(Artifact output) {
ownerPrimaryOutput = checkNotNull(output);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,54 @@ public void matchWildcardLabel() {
assertThat(scrubber.forSpawn(createSpawn("//spam:eggs", "Foo"))).isNull();
}

@Test
public void matchExactKind() {
var scrubber =
new Scrubber(
Config.newBuilder()
.addRules(
Config.Rule.newBuilder()
.setMatcher(Config.Matcher.newBuilder().setKind("java_library")))
.build());

assertThat(scrubber.forSpawn(createSpawn("//foo:bar", "Foo", "java_library", false)))
.isNotNull();
assertThat(scrubber.forSpawn(createSpawn("//foo:barbaz", "Foo", "java_test", false))).isNull();
}

@Test
public void matchUnionKind() {
var scrubber =
new Scrubber(
Config.newBuilder()
.addRules(
Config.Rule.newBuilder()
.setMatcher(Config.Matcher.newBuilder().setKind("java_library|java_test")))
.build());

assertThat(scrubber.forSpawn(createSpawn("//foo:bar", "Foo", "java_library", false)))
.isNotNull();
assertThat(scrubber.forSpawn(createSpawn("//spam:eggs", "Foo", "java_test", false)))
.isNotNull();
assertThat(scrubber.forSpawn(createSpawn("//quux:xyzzy", "Foo", "go_library", false))).isNull();
}

@Test
public void matchWildcardKind() {
var scrubber =
new Scrubber(
Config.newBuilder()
.addRules(
Config.Rule.newBuilder()
.setMatcher(Config.Matcher.newBuilder().setKind("java_.*")))
.build());

assertThat(scrubber.forSpawn(createSpawn("//foo:bar", "Foo", "java_library", false)))
.isNotNull();
assertThat(scrubber.forSpawn(createSpawn("//foo:baz", "Foo", "java_test", false))).isNotNull();
assertThat(scrubber.forSpawn(createSpawn("//spam:eggs", "Foo", "go_library", false))).isNull();
}

@Test
public void rejectToolAction() {
var scrubber =
Expand All @@ -137,7 +185,9 @@ public void rejectToolAction() {
.build());

assertThat(scrubber.forSpawn(createSpawn("//foo:bar", "Foo"))).isNotNull();
assertThat(scrubber.forSpawn(createSpawn("//foo:bar", "Foo", /* forTool= */ true))).isNull();
assertThat(
scrubber.forSpawn(createSpawn("//foo:bar", "Foo", "java_library", /* forTool= */ true)))
.isNull();
}

@Test
Expand All @@ -155,7 +205,9 @@ public void acceptToolAction() {
.build());

assertThat(scrubber.forSpawn(createSpawn("//foo:bar", "Foo"))).isNotNull();
assertThat(scrubber.forSpawn(createSpawn("//foo:bar", "Foo", /* forTool= */ true))).isNotNull();
assertThat(
scrubber.forSpawn(createSpawn("//foo:bar", "Foo", "java_library", /* forTool= */ true)))
.isNotNull();
}

@Test
Expand Down Expand Up @@ -420,13 +472,15 @@ private static Spawn createSpawn() {
}

private static Spawn createSpawn(String label, String mnemonic) {
return createSpawn(label, mnemonic, /* forTool= */ false);
return createSpawn(label, mnemonic, /* ruleKind= */ "dummy-target-kind", /* forTool= */ false);
}

private static Spawn createSpawn(String label, String mnemonic, boolean forTool) {
private static Spawn createSpawn(
String label, String mnemonic, String ruleKind, boolean forTool) {
return new SpawnBuilder("cmd")
.withOwnerLabel(label)
.withMnemonic(mnemonic)
.withOwnerRuleKind(ruleKind)
.setBuiltForToolConfiguration(forTool)
.build();
}
Expand Down

0 comments on commit b471046

Please sign in to comment.