diff options
6 files changed, 152 insertions, 31 deletions
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/wire/WireSystemFlagsDeployResult.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/wire/WireSystemFlagsDeployResult.java index 69070d86ef7..17af0027893 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/wire/WireSystemFlagsDeployResult.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/wire/WireSystemFlagsDeployResult.java @@ -18,6 +18,7 @@ import java.util.List; public class WireSystemFlagsDeployResult { @JsonProperty("changes") public List<WireFlagDataChange> changes; @JsonProperty("errors") public List<WireOperationFailure> errors; + @JsonProperty("warnings") public List<WireWarning> warnings; @JsonIgnoreProperties(ignoreUnknown = true) @JsonInclude(JsonInclude.Include.NON_NULL) @@ -39,6 +40,14 @@ public class WireSystemFlagsDeployResult { @JsonProperty("data") public WireFlagData data; } + @JsonIgnoreProperties(ignoreUnknown = true) + @JsonInclude(JsonInclude.Include.NON_NULL) + public static class WireWarning { + @JsonProperty("flag-id") public String flagId; + @JsonProperty("message") public String message; + @JsonProperty("targets") public List<String> targets; + } + public boolean hasErrors() { return errors != null && !errors.isEmpty(); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java index 2993b780dfe..161d3734aae 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/FlagsClient.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller.restapi.systemflags; import ai.vespa.util.http.retry.DelayedConnectionLevelRetryHandler; +import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.yahoo.vespa.athenz.api.AthenzIdentity; import com.yahoo.vespa.athenz.identity.ServiceIdentityProvider; @@ -35,6 +36,7 @@ import java.io.UncheckedIOException; import java.net.URI; import java.net.URISyntaxException; import java.time.Duration; +import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -65,6 +67,17 @@ class FlagsClient { }); } + List<FlagId> listDefinedFlags(FlagsTarget target) { + HttpGet request = new HttpGet(createUri(target, "/defined", List.of())); + return executeRequest(request, response -> { + verifySuccess(response, null); + JsonNode json = mapper.readTree(response.getEntity().getContent()); + List<FlagId> flagIds = new ArrayList<>(); + json.fieldNames().forEachRemaining(fieldName -> flagIds.add(new FlagId(fieldName))); + return flagIds; + }); + } + void putFlagData(FlagsTarget target, FlagData flagData) throws FlagsException, UncheckedIOException { HttpPut request = new HttpPut(createUri(target, "/data/" + flagData.id().toString(), List.of())); request.setEntity(jsonContent(flagData.serializeToJson())); @@ -82,7 +95,6 @@ class FlagsClient { }); } - private static CloseableHttpClient createClient(ServiceIdentityProvider identityProvider, Set<FlagsTarget> targets) { DelayedConnectionLevelRetryHandler retryHandler = DelayedConnectionLevelRetryHandler.Builder .withExponentialBackoff(Duration.ofSeconds(1), Duration.ofSeconds(20), 5) 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 c55d9d9a5e9..010e98c2640 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 @@ -8,6 +8,7 @@ import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult.WireFlagDataChange; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult.WireOperationFailure; +import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireSystemFlagsDeployResult.WireWarning; import java.util.ArrayList; import java.util.HashMap; @@ -17,6 +18,8 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.BiFunction; +import java.util.function.Function; import static java.util.stream.Collectors.toList; @@ -27,14 +30,16 @@ class SystemFlagsDeployResult { private final List<FlagDataChange> flagChanges; private final List<OperationError> errors; + private final List<Warning> warnings; - SystemFlagsDeployResult(List<FlagDataChange> flagChanges, List<OperationError> errors) { + SystemFlagsDeployResult(List<FlagDataChange> flagChanges, List<OperationError> errors, List<Warning> warnings) { this.flagChanges = flagChanges; this.errors = errors; + this.warnings = warnings; } SystemFlagsDeployResult(List<OperationError> errors) { - this(List.of(), errors); + this(List.of(), errors, List.of()); } List<FlagDataChange> flagChanges() { @@ -45,43 +50,50 @@ class SystemFlagsDeployResult { return errors; } + List<Warning> warnings() { return warnings; } + static SystemFlagsDeployResult merge(List<SystemFlagsDeployResult> results) { List<FlagDataChange> mergedChanges = mergeChanges(results); List<OperationError> mergedErrors = mergeErrors(results); - return new SystemFlagsDeployResult(mergedChanges, mergedErrors); + List<Warning> mergedWarnings = mergeWarnings(results); + return new SystemFlagsDeployResult(mergedChanges, mergedErrors, mergedWarnings); } private static List<OperationError> mergeErrors(List<SystemFlagsDeployResult> results) { - Map<OperationErrorWithoutTarget, Set<FlagsTarget>> targetsForError = new HashMap<>(); - for (SystemFlagsDeployResult result : results) { - for (OperationError error : result.errors()) { - var errorWithoutTarget = new OperationErrorWithoutTarget(error); - targetsForError.computeIfAbsent(errorWithoutTarget, k -> new HashSet<>()) - .addAll(error.targets()); - } - } - List<OperationError> mergedErrors = new ArrayList<>(); - targetsForError.forEach( - (error, targets) -> mergedErrors.add(error.toOperationError(targets))); - return mergedErrors; + return merge(results, SystemFlagsDeployResult::errors, OperationError::targets, + OperationErrorWithoutTarget::new, OperationErrorWithoutTarget::toOperationError); } private static List<FlagDataChange> mergeChanges(List<SystemFlagsDeployResult> results) { - Map<FlagDataChangeWithoutTarget, Set<FlagsTarget>> targetsForChange = new HashMap<>(); + return merge(results, SystemFlagsDeployResult::flagChanges, FlagDataChange::targets, + FlagDataChangeWithoutTarget::new, FlagDataChangeWithoutTarget::toFlagDataChange); + } + + private static List<Warning> mergeWarnings(List<SystemFlagsDeployResult> results) { + return merge(results, SystemFlagsDeployResult::warnings, Warning::targets, + WarningWithoutTarget::new, WarningWithoutTarget::toWarning); + } + + private static <VALUE, VALUE_WITHOUT_TARGET> List<VALUE> merge( + List<SystemFlagsDeployResult> results, + Function<SystemFlagsDeployResult, List<VALUE>> valuesGetter, + Function<VALUE, Set<FlagsTarget>> targetsGetter, + Function<VALUE, VALUE_WITHOUT_TARGET> transformer, + BiFunction<VALUE_WITHOUT_TARGET, Set<FlagsTarget>, VALUE> reverseTransformer) { + Map<VALUE_WITHOUT_TARGET, Set<FlagsTarget>> targetsForValue = new HashMap<>(); for (SystemFlagsDeployResult result : results) { - for (FlagDataChange change : result.flagChanges()) { - var changeWithoutTarget = new FlagDataChangeWithoutTarget(change); - targetsForChange.computeIfAbsent(changeWithoutTarget, k -> new HashSet<>()) - .addAll(change.targets()); + for (VALUE value : valuesGetter.apply(result)) { + VALUE_WITHOUT_TARGET valueWithoutTarget = transformer.apply(value); + targetsForValue.computeIfAbsent(valueWithoutTarget, k -> new HashSet<>()) + .addAll(targetsGetter.apply(value)); } } - List<FlagDataChange> mergedChanges = new ArrayList<>(); - targetsForChange.forEach( - (change, targets) -> mergedChanges.add(change.toFlagDataChange(targets))); - return mergedChanges; + List<VALUE> mergedValues = new ArrayList<>(); + targetsForValue.forEach( + (value, targets) -> mergedValues.add(reverseTransformer.apply(value, targets))); + return mergedValues; } - WireSystemFlagsDeployResult toWire() { var wireResult = new WireSystemFlagsDeployResult(); wireResult.changes = new ArrayList<>(); @@ -104,6 +116,14 @@ class SystemFlagsDeployResult { wireError.data = error.flagData().map(FlagData::toWire).orElse(null); wireResult.errors.add(wireError); } + wireResult.warnings = new ArrayList<>(); + for (Warning warning : warnings) { + var wireWarning = new WireWarning(); + wireWarning.message = warning.message(); + wireWarning.flagId = warning.flagId().toString(); + wireWarning.targets = warning.targets().stream().map(FlagsTarget::asString).collect(toList()); + wireResult.warnings.add(wireWarning); + } return wireResult; } @@ -264,6 +284,38 @@ class SystemFlagsDeployResult { String asString() { return stringValue; } } + static class Warning { + final String message; + final Set<FlagsTarget> targets; + final FlagId flagId; + + private Warning(String message, Set<FlagsTarget> targets, FlagId flagId) { + this.message = message; + this.targets = targets; + this.flagId = flagId; + } + + static Warning dataForUndefinedFlag(FlagsTarget target, FlagId flagId) { + return new Warning("Flag data present for undefined flag", Set.of(target), flagId); + } + + String message() { return message; } + Set<FlagsTarget> targets() { return targets; } + FlagId flagId() { return flagId; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Warning warning = (Warning) o; + return Objects.equals(message, warning.message) && + Objects.equals(targets, warning.targets) && + Objects.equals(flagId, warning.flagId); + } + + @Override public int hashCode() { return Objects.hash(message, targets, flagId); } + } + private static class FlagDataChangeWithoutTarget { final FlagId flagId; final OperationType operationType; @@ -333,4 +385,16 @@ class SystemFlagsDeployResult { @Override public int hashCode() { return Objects.hash(message, operation, flagId, flagData); } } + + private static class WarningWithoutTarget { + final String message; + final FlagId flagId; + + WarningWithoutTarget(Warning warning) { + this.message = warning.message(); + this.flagId = warning.flagId(); + } + + Warning toWarning(Set<FlagsTarget> targets) { return new Warning(message, targets, flagId); } + } } 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 c75a6d7b413..820384ce810 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 @@ -11,6 +11,7 @@ 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.SystemFlagsDataArchive; import com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.OperationError; +import com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.Warning; import java.util.ArrayList; import java.util.Collection; @@ -79,8 +80,10 @@ class SystemFlagsDeployer { private SystemFlagsDeployResult deployFlags(FlagsTarget target, Set<FlagData> flagData, boolean dryRun) { Map<FlagId, FlagData> wantedFlagData = lookupTable(flagData); Map<FlagId, FlagData> currentFlagData; + List<FlagId> definedFlags; try { currentFlagData = lookupTable(client.listFlagData(target)); + definedFlags = client.listDefinedFlags(target); } catch (Exception e) { log.log(LogLevel.WARNING, String.format("Failed to list flag data for target '%s': %s", target, e.getMessage()), e); return new SystemFlagsDeployResult(List.of(OperationError.listFailed(e.getMessage(), target))); @@ -88,12 +91,13 @@ class SystemFlagsDeployer { List<OperationError> errors = new ArrayList<>(); List<FlagDataChange> results = new ArrayList<>(); + List<Warning> warnings = new ArrayList<>(); createNewFlagData(target, dryRun, wantedFlagData, currentFlagData, results, errors); updateExistingFlagData(target, dryRun, wantedFlagData, currentFlagData, results, errors); removeOldFlagData(target, dryRun, wantedFlagData, currentFlagData, results, errors); - - return new SystemFlagsDeployResult(results, errors); + warnOnFlagDataForUndefinedFlags(target, wantedFlagData, currentFlagData, definedFlags, warnings); + return new SystemFlagsDeployResult(results, errors, warnings); } private void createNewFlagData(FlagsTarget target, @@ -171,6 +175,18 @@ class SystemFlagsDeployer { }); } + private static void warnOnFlagDataForUndefinedFlags(FlagsTarget target, + Map<FlagId, FlagData> wantedFlagData, + Map<FlagId, FlagData> currentFlagData, + List<FlagId> definedFlags, + List<Warning> warnings) { + for (FlagId flagId : currentFlagData.keySet()) { + if (wantedFlagData.containsKey(flagId) && !definedFlags.contains(flagId)) { + warnings.add(Warning.dataForUndefinedFlag(target, flagId)); + } + } + } + private static void dryRunFlagDataValidation(FlagData data) { Flags.getFlag(data.id()) .ifPresent(definition -> data.validate(definition.getUnboundFlag().serializer())); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java index 104bb91a8cb..b2a835b6b55 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java @@ -27,7 +27,8 @@ public class SystemFlagsDeployResultTest { List.of( FlagDataChange.deleted(flagOne, controllerTarget)), List.of( - OperationError.deleteFailed("delete failed", controllerTarget, flagTwo))); + OperationError.deleteFailed("delete failed", controllerTarget, flagTwo)), + List.of()); WireSystemFlagsDeployResult wire = result.toWire(); assertThat(wire.changes).hasSize(1); @@ -49,11 +50,13 @@ public class SystemFlagsDeployResultTest { SystemFlagsDeployResult resultController = new SystemFlagsDeployResult( List.of(FlagDataChange.deleted(flagOne, controllerTarget)), - List.of(OperationError.deleteFailed("message", controllerTarget, flagTwo))); + List.of(OperationError.deleteFailed("message", controllerTarget, flagTwo)), + List.of()); SystemFlagsDeployResult resultProdUsWest1 = new SystemFlagsDeployResult( List.of(FlagDataChange.deleted(flagOne, prodUsWest1Target)), - List.of(OperationError.deleteFailed("message", prodUsWest1Target, flagTwo))); + List.of(OperationError.deleteFailed("message", prodUsWest1Target, flagTwo)), + List.of()); var results = List.of(resultController, resultProdUsWest1); SystemFlagsDeployResult mergedResult = merge(results); 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 4c41b54585f..475ac12f2fd 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 @@ -132,6 +132,23 @@ public class SystemFlagsDeployerTest { .containsOnly(OperationError.archiveValidationFailed("Unknown flag file: flags/my-flag/main.prod.unknown-region.json")); } + @Test + public void creates_warning_entry_for_existing_flag_data_for_undefined_flag() throws IOException { + FlagData prodUsEast3Data = flagData("flags/my-flag/main.prod.us-east-3.json"); + FlagsClient flagsClient = mock(FlagsClient.class); + when(flagsClient.listFlagData(prodUsEast3Target)) + .thenReturn(List.of(prodUsEast3Data)); + when(flagsClient.listDefinedFlags(prodUsEast3Target)) + .thenReturn(List.of()); + SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder() + .addFile("main.prod.us-east-3.json", prodUsEast3Data) + .build(); + SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(prodUsEast3Target)); + SystemFlagsDeployResult result = deployer.deployFlags(archive, true); + assertThat(result.warnings()) + .containsOnly(SystemFlagsDeployResult.Warning.dataForUndefinedFlag(prodUsEast3Target, new FlagId("my-flag"))); + } + private static FlagData flagData(String filename) throws IOException { return FlagData.deserializeUtf8Json(Files.readAllBytes(Paths.get("src/test/resources/system-flags/" + filename))); } |