From c31fa9ba6caa401e1b2ef94cad9ce0a3b1dfe358 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Fri, 26 Feb 2021 15:50:39 +0100 Subject: Improve feedback from /system-flags/v1 responses Add flag owners to result/warning/error whenever available. Make warning/error message from undefined flags more informative. --- .../restapi/systemflags/SystemFlagsDeployResult.java | 13 ++++++++++++- .../controller/restapi/systemflags/SystemFlagsDeployer.java | 6 ++++-- .../restapi/systemflags/SystemFlagsDeployerTest.java | 4 +++- 3 files changed, 19 insertions(+), 4 deletions(-) (limited to 'controller-server/src') diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java index 57d47757c5e..d169cd97df7 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java @@ -2,7 +2,9 @@ package com.yahoo.vespa.hosted.controller.restapi.systemflags; import com.fasterxml.jackson.databind.JsonNode; +import com.yahoo.vespa.flags.FlagDefinition; import com.yahoo.vespa.flags.FlagId; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult; @@ -100,6 +102,7 @@ class SystemFlagsDeployResult { for (FlagDataChange change : flagChanges) { var wireChange = new WireFlagDataChange(); wireChange.flagId = change.flagId().toString(); + wireChange.owners = owners(change.flagId()); wireChange.operation = change.operation().asString(); wireChange.targets = change.targets().stream().map(FlagsTarget::asString).collect(toList()); wireChange.data = change.data().map(FlagData::toWire).orElse(null); @@ -113,6 +116,7 @@ class SystemFlagsDeployResult { wireError.operation = error.operation().asString(); wireError.targets = error.targets().stream().map(FlagsTarget::asString).collect(toList()); wireError.flagId = error.flagId().map(FlagId::toString).orElse(null); + wireError.owners = error.flagId().map(id -> owners(id)).orElse(List.of()); wireError.data = error.flagData().map(FlagData::toWire).orElse(null); wireResult.errors.add(wireError); } @@ -121,12 +125,17 @@ class SystemFlagsDeployResult { var wireWarning = new WireWarning(); wireWarning.message = warning.message(); wireWarning.flagId = warning.flagId().toString(); + wireWarning.owners = owners(warning.flagId()); wireWarning.targets = warning.targets().stream().map(FlagsTarget::asString).collect(toList()); wireResult.warnings.add(wireWarning); } return wireResult; } + private static List owners(FlagId id) { + return Flags.getFlag(id).map(FlagDefinition::getOwners).orElse(List.of()); + } + static class FlagDataChange { private final FlagId flagId; @@ -296,7 +305,9 @@ class SystemFlagsDeployResult { } static Warning dataForUndefinedFlag(FlagsTarget target, FlagId flagId) { - return new Warning("Flag data present for undefined flag", Set.of(target), flagId); + return new Warning( + "Flag data present for undefined flag. Remove flag data files if flag's definition is already removed from Flags/PermanentFlags. " + + "Consult ModelContext.FeatureFlags for safe removal of flag used by config-model.", Set.of(target), flagId); } String message() { return message; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java index 5ea277f5101..0331bf292a0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.controller.restapi.systemflags; import com.yahoo.concurrent.DaemonThreadFactory; import com.yahoo.config.provision.SystemName; -import java.util.logging.Level; import com.yahoo.vespa.athenz.identity.ServiceIdentityProvider; import com.yahoo.vespa.flags.FlagId; import com.yahoo.vespa.flags.Flags; @@ -25,6 +24,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.function.Function; +import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -181,9 +181,11 @@ class SystemFlagsDeployer { Map currentFlagData, List definedFlags, List errors) { + String errorMessage = "Flag not defined in target zone. If zone/configserver cluster is new, add an empty flag " + + "data file for this zone as a temporary measure until the stale flag data files are removed."; for (FlagId flagId : wantedFlagData.keySet()) { if (!currentFlagData.containsKey(flagId) && !definedFlags.contains(flagId)) { - errors.add(OperationError.createFailed("Flag not defined in target zone", target, wantedFlagData.get(flagId))); + errors.add(OperationError.createFailed(errorMessage, target, wantedFlagData.get(flagId))); } } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java index 789be26db1f..35a13cdeeec 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java @@ -147,8 +147,10 @@ public class SystemFlagsDeployerTest { .build(); SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(prodUsEast3Target)); SystemFlagsDeployResult result = deployer.deployFlags(archive, true); + String expectedErrorMessage = "Flag not defined in target zone. If zone/configserver cluster is new, " + + "add an empty flag data file for this zone as a temporary measure until the stale flag data files are removed."; assertThat(result.errors()) - .containsOnly(SystemFlagsDeployResult.OperationError.createFailed("Flag not defined in target zone", prodUsEast3Target, prodUsEast3Data)); + .containsOnly(SystemFlagsDeployResult.OperationError.createFailed(expectedErrorMessage, prodUsEast3Target, prodUsEast3Data)); } @Test -- cgit v1.2.3