Merge pull request #11321 from vespa-engine/bjorncs/system-flags-handler-improvements

Bjorncs/system flags handler improvements

5 files changed, 277 insertions, 73 deletions

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 d11e17ce634..045b8809041 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
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.controller.api.systemflags.v1.wire.WireErrorRespon
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpResponse;
 import org.apache.http.HttpStatus;
+import org.apache.http.NameValuePair;
 import org.apache.http.client.ResponseHandler;
 import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.HttpDelete;
@@ -24,6 +25,7 @@ import org.apache.http.entity.StringEntity;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.DefaultHttpRequestRetryHandler;
 import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.message.BasicNameValuePair;
 import org.apache.http.util.EntityUtils;
 
 import javax.net.ssl.HostnameVerifier;
@@ -56,7 +58,7 @@ class FlagsClient {
     }
 
     List<FlagData> listFlagData(FlagsTarget target) throws FlagsException, UncheckedIOException {
-        HttpGet request = new HttpGet(createUri(target, "/data"));
+        HttpGet request = new HttpGet(createUri(target, "/data", List.of(new BasicNameValuePair("recursive", "true"))));
         return executeRequest(request, response -> {
             verifySuccess(response, target, null);
             return FlagData.deserializeList(EntityUtils.toByteArray(response.getEntity()));
@@ -64,7 +66,7 @@ class FlagsClient {
     }
 
     void putFlagData(FlagsTarget target, FlagData flagData) throws FlagsException, UncheckedIOException {
-        HttpPut request = new HttpPut(createUri(target, "/data/" + flagData.id().toString()));
+        HttpPut request = new HttpPut(createUri(target, "/data/" + flagData.id().toString(), List.of()));
         request.setEntity(jsonContent(flagData.serializeToJson()));
         executeRequest(request, response -> {
             verifySuccess(response, target, flagData.id());
@@ -73,7 +75,7 @@ class FlagsClient {
     }
 
     void deleteFlagData(FlagsTarget target, FlagId flagId) throws FlagsException, UncheckedIOException {
-        HttpDelete request = new HttpDelete(createUri(target, "/data/" + flagId.toString()));
+        HttpDelete request = new HttpDelete(createUri(target, "/data/" + flagId.toString(), List.of()));
         executeRequest(request, response -> {
             verifySuccess(response, target, flagId);
             return null;
@@ -105,9 +107,12 @@ class FlagsClient {
         }
     }
 
-    private static URI createUri(FlagsTarget target, String subPath) {
+    private static URI createUri(FlagsTarget target, String subPath, List<NameValuePair> queryParams) {
         try {
-            return new URIBuilder(target.endpoint()).setPath(FLAGS_V1_PATH + subPath).build();
+            return new URIBuilder(target.endpoint())
+                    .setPath(FLAGS_V1_PATH + subPath)
+                    .setParameters(queryParams)
+                    .build();
         } catch (URISyntaxException e) {
             throw new RuntimeException(e); // should never happen
         }
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 ae1cb6321bd..3809bf6b400 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
@@ -7,9 +7,9 @@ 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;
 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 java.util.ArrayList;
-import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -26,17 +26,31 @@ import static java.util.stream.Collectors.toList;
 class SystemFlagsDeployResult {
 
     private final List<FlagDataChange> flagChanges;
+    private final List<OperationError> errors;
 
-    SystemFlagsDeployResult(List<FlagDataChange> flagChanges) { this.flagChanges = flagChanges; }
+    SystemFlagsDeployResult(List<FlagDataChange> flagChanges, List<OperationError> errors) {
+        this.flagChanges = flagChanges;
+        this.errors = errors;
+    }
+
+    SystemFlagsDeployResult(List<OperationError> errors) {
+        this(List.of(), errors);
+    }
 
     List<FlagDataChange> flagChanges() {
         return flagChanges;
     }
 
+    List<OperationError> errors() {
+        return errors;
+    }
+
     static SystemFlagsDeployResult merge(List<SystemFlagsDeployResult> results) {
         Map<FlagDataOperation, Set<FlagsTarget>> targetsForOperation = new HashMap<>();
 
+        List<OperationError> errors = new ArrayList<>();
         for (SystemFlagsDeployResult result : results) {
+            errors.addAll(result.errors);
             for (FlagDataChange change : result.flagChanges()) {
                 FlagDataOperation operation = new FlagDataOperation(change);
                 targetsForOperation.computeIfAbsent(operation, k -> new HashSet<>())
@@ -47,7 +61,7 @@ class SystemFlagsDeployResult {
         List<FlagDataChange> mergedResult = new ArrayList<>();
         targetsForOperation.forEach(
                 (operation, targets) -> mergedResult.add(operation.toFlagDataChange(targets)));
-        return new SystemFlagsDeployResult(mergedResult);
+        return new SystemFlagsDeployResult(mergedResult, errors);
     }
 
     WireSystemFlagsDeployResult toWire() {
@@ -61,6 +75,15 @@ class SystemFlagsDeployResult {
             wireChange.data = change.data().map(FlagData::toWire).orElse(null);
             wireChange.previousData = change.previousData().map(FlagData::toWire).orElse(null);
         }
+        wireResult.errors = new ArrayList<>();
+        for (OperationError error : errors) {
+            var wireError = new WireOperationFailure();
+            wireError.message = error.message();
+            wireError.operation = error.operation().asString();
+            wireError.target = error.target().asString();
+            wireError.flagId = error.flagId().map(FlagId::toString).orElse(null);
+            wireError.data = error.flagData().map(FlagData::toWire).orElse(null);
+        }
         return wireResult;
     }
 
@@ -81,16 +104,16 @@ class SystemFlagsDeployResult {
             this.previousData = previousData;
         }
 
-        static FlagDataChange created(FlagId flagId, Set<FlagsTarget> targets, FlagData data) {
-            return new FlagDataChange(flagId, targets, OperationType.CREATE, data, null);
+        static FlagDataChange created(FlagId flagId, FlagsTarget target, FlagData data) {
+            return new FlagDataChange(flagId, Set.of(target), OperationType.CREATE, data, null);
         }
 
-        static FlagDataChange deleted(FlagId flagId, Set<FlagsTarget> targets) {
-            return new FlagDataChange(flagId, targets, OperationType.DELETE, null, null);
+        static FlagDataChange deleted(FlagId flagId, FlagsTarget target) {
+            return new FlagDataChange(flagId, Set.of(target), OperationType.DELETE, null, null);
         }
 
-        static FlagDataChange updated(FlagId flagId, Set<FlagsTarget> targets, FlagData data, FlagData previousData) {
-            return new FlagDataChange(flagId, targets, OperationType.UPDATE, data, previousData);
+        static FlagDataChange updated(FlagId flagId, FlagsTarget target, FlagData data, FlagData previousData) {
+            return new FlagDataChange(flagId, Set.of(target), OperationType.UPDATE, data, previousData);
         }
 
         FlagId flagId() {
@@ -142,21 +165,79 @@ class SystemFlagsDeployResult {
         }
     }
 
+    static class OperationError {
+
+        final String message;
+        final FlagsTarget target;
+        final OperationType operation;
+        final FlagId flagId;
+        final FlagData flagData;
+
+        private OperationError(
+                String message, FlagsTarget target, OperationType operation, FlagId flagId, FlagData flagData) {
+            this.message = message;
+            this.target = target;
+            this.operation = operation;
+            this.flagId = flagId;
+            this.flagData = flagData;
+        }
+
+        static OperationError listFailed(String message, FlagsTarget target) {
+            return new OperationError(message, target, OperationType.LIST, null, null);
+        }
+
+        static OperationError createFailed(String message, FlagsTarget target, FlagData flagData) {
+            return new OperationError(message, target, OperationType.CREATE, flagData.id(), flagData);
+        }
+
+        static OperationError updateFailed(String message, FlagsTarget target, FlagData flagData) {
+            return new OperationError(message, target, OperationType.UPDATE, flagData.id(), flagData);
+        }
+
+        static OperationError deleteFailed(String message, FlagsTarget target, FlagId id) {
+            return new OperationError(message, target, OperationType.UPDATE, id, null);
+        }
+
+        String message() { return message; }
+        FlagsTarget target() { return target; }
+        OperationType operation() { return operation; }
+        Optional<FlagId> flagId() { return Optional.ofNullable(flagId); }
+        Optional<FlagData> flagData() { return Optional.ofNullable(flagData); }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (o == null || getClass() != o.getClass()) return false;
+            OperationError that = (OperationError) o;
+            return Objects.equals(message, that.message) &&
+                    Objects.equals(target, that.target) &&
+                    operation == that.operation &&
+                    Objects.equals(flagId, that.flagId) &&
+                    Objects.equals(flagData, that.flagData);
+        }
+
+        @Override public int hashCode() { return Objects.hash(message, target, operation, flagId, flagData); }
+
+        @Override
+        public String toString() {
+            return "OperationFailure{" +
+                    "message='" + message + '\'' +
+                    ", target=" + target +
+                    ", operation=" + operation +
+                    ", flagId=" + flagId +
+                    ", flagData=" + flagData +
+                    '}';
+        }
+    }
+
     enum OperationType {
-        CREATE("create"), DELETE("delete"), UPDATE("update");
+        CREATE("create"), DELETE("delete"), UPDATE("update"), LIST("list");
 
         private final String stringValue;
 
         OperationType(String stringValue) { this.stringValue = stringValue; }
 
         String asString() { return stringValue; }
-
-        static OperationType fromString(String stringValue) {
-            return Arrays.stream(values())
-                    .filter(v -> v.stringValue.equals(stringValue))
-                    .findAny()
-                    .orElseThrow(() -> new IllegalArgumentException("Unknown string value: " + stringValue));
-        }
     }
 
     private static class FlagDataOperation {
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 2e783d5fcb3..6b6a93934b3 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
@@ -2,22 +2,27 @@
 package com.yahoo.vespa.hosted.controller.restapi.systemflags;
 
 import com.yahoo.concurrent.DaemonThreadFactory;
+import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.athenz.identity.ServiceIdentityProvider;
 import com.yahoo.vespa.flags.FlagId;
 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 java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ExecutorCompletionService;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.function.Function;
+import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.FlagDataChange;
@@ -29,10 +34,11 @@ import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsD
  */
 class SystemFlagsDeployer  {
 
+    private static final Logger log = Logger.getLogger(SystemFlagsDeployer.class.getName());
+
     private final FlagsClient client;
     private final Set<FlagsTarget> targets;
-    private final ExecutorCompletionService<SystemFlagsDeployResult> completionService =
-            new ExecutorCompletionService<>(Executors.newCachedThreadPool(new DaemonThreadFactory("system-flags-deployer-")));
+    private final ExecutorService executor = Executors.newCachedThreadPool(new DaemonThreadFactory("system-flags-deployer-"));
 
 
     SystemFlagsDeployer(ServiceIdentityProvider identityProvider, Set<FlagsTarget> targets) {
@@ -45,62 +51,119 @@ class SystemFlagsDeployer  {
     }
 
     SystemFlagsDeployResult deployFlags(SystemFlagsDataArchive archive, boolean dryRun) {
+        Map<FlagsTarget, Future<SystemFlagsDeployResult>> futures = new HashMap<>();
         for (FlagsTarget target : targets) {
-            completionService.submit(() -> deployFlags(target, archive.flagData(target), dryRun));
+            futures.put(target, executor.submit(() -> deployFlags(target, archive.flagData(target), dryRun)));
         }
         List<SystemFlagsDeployResult> results = new ArrayList<>();
-        Future<SystemFlagsDeployResult> future;
-        try {
-            while (results.size() < targets.size() && (future = completionService.take()) != null) {
-                try {
-                    results.add(future.get());
-                } catch (ExecutionException e) {
-                    // TODO Handle errors
-                    throw new RuntimeException(e);
-                }
+        futures.forEach((target, future) -> {
+            try {
+                results.add(future.get());
+            } catch (InterruptedException | ExecutionException e) {
+                log.log(LogLevel.ERROR, String.format("Failed to deploy flags for target '%s': %s", target, e.getMessage()), e);
+                throw new RuntimeException(e);
             }
-            return SystemFlagsDeployResult.merge(results);
-        } catch (InterruptedException e) {
-            throw new RuntimeException(e);
-        }
+        });
+        return SystemFlagsDeployResult.merge(results);
     }
 
-    // TODO Handle http status code 4xx/5xx (e.g for unknown flag id)
     private SystemFlagsDeployResult deployFlags(FlagsTarget target, Set<FlagData> flagData, boolean dryRun) {
         Map<FlagId, FlagData> wantedFlagData = lookupTable(flagData);
-        Map<FlagId, FlagData> currentFlagData = lookupTable(client.listFlagData(target));
+        Map<FlagId, FlagData> currentFlagData;
+        try {
+            currentFlagData = lookupTable(client.listFlagData(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)));
+        }
+
+        List<OperationError> errors = new ArrayList<>();
+        List<FlagDataChange> results = new ArrayList<>();
 
-        List<FlagDataChange> result = 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);
+    }
 
+    private void createNewFlagData(FlagsTarget target,
+                                   boolean dryRun,
+                                   Map<FlagId, FlagData> wantedFlagData,
+                                   Map<FlagId, FlagData> currentFlagData,
+                                   List<FlagDataChange> results,
+                                   List<OperationError> errors) {
         wantedFlagData.forEach((id, data) -> {
-            if (currentFlagData.containsKey(id)) {
-                FlagData currentData = currentFlagData.get(id);
-                if (currentData.toJsonNode().equals(data.toJsonNode())) {
-                    return; // noop
+            FlagData currentData = currentFlagData.get(id);
+            if (currentData != null) {
+                return; // not a new flag
+            }
+            if (!dryRun) {
+                try {
+                    client.putFlagData(target, data);
+                } catch (Exception e) {
+                    log.log(LogLevel.WARNING, String.format("Failed to put flag '%s' for target '%s': %s", data.id(), target, e.getMessage()), e);
+                    errors.add(OperationError.createFailed(e.getMessage(), target, data));
+                    return;
                 }
-                result.add(FlagDataChange.updated(id, Set.of(target), data, currentData));
-            } else {
-                result.add(FlagDataChange.created(id, Set.of(target), data));
+            }
+            results.add(FlagDataChange.created(id, target, data));
+        });
+    }
+
+    private void updateExistingFlagData(FlagsTarget target,
+                                        boolean dryRun,
+                                        Map<FlagId, FlagData> wantedFlagData,
+                                        Map<FlagId, FlagData> currentFlagData,
+                                        List<FlagDataChange> results,
+                                        List<OperationError> errors) {
+        wantedFlagData.forEach((id, wantedData) -> {
+            FlagData currentData = currentFlagData.get(id);
+            if (currentData == null || isEqual(currentData, wantedData)) {
+                return; // not an flag data update
             }
             if (!dryRun) {
-                client.putFlagData(target, data);
+                try {
+                    client.putFlagData(target, wantedData);
+                } catch (Exception e) {
+                    log.log(LogLevel.WARNING, String.format("Failed to update flag '%s' for target '%s': %s", wantedData.id(), target, e.getMessage()), e);
+                    errors.add(OperationError.updateFailed(e.getMessage(), target, wantedData));
+                    return;
+                }
             }
+            results.add(FlagDataChange.updated(id, target, wantedData, currentData));
         });
+    }
 
+    private void removeOldFlagData(FlagsTarget target,
+                                   boolean dryRun,
+                                   Map<FlagId, FlagData> wantedFlagData,
+                                   Map<FlagId, FlagData> currentFlagData,
+                                   List<FlagDataChange> results,
+                                   List<OperationError> errors) {
         currentFlagData.forEach((id, data) -> {
-            if (!wantedFlagData.containsKey(id)) {
-                if (!dryRun) {
+            if (wantedFlagData.containsKey(id)) {
+                return; // not a removed flag
+            }
+            if (!dryRun) {
+                try {
                     client.deleteFlagData(target, id);
+                } catch (Exception e) {
+                    log.log(LogLevel.WARNING, String.format("Failed to delete flag '%s' for target '%s': %s", id, target, e.getMessage()), e);
+                    errors.add(OperationError.deleteFailed(e.getMessage(), target, id));
+                    return;
                 }
-                result.add(FlagDataChange.deleted(id, Set.of(target)));
             }
+            results.add(FlagDataChange.deleted(id, target));
         });
-
-        return new SystemFlagsDeployResult(result);
     }
 
     private static Map<FlagId, FlagData> lookupTable(Collection<FlagData> data) {
         return data.stream().collect(Collectors.toMap(FlagData::id, Function.identity()));
     }
 
+    private static boolean isEqual(FlagData l, FlagData r) {
+        return Objects.equals(l.toJsonNode(), r.toJsonNode());
+    }
+
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java
index 32cb79963c1..fd594cd57bc 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java
@@ -6,6 +6,7 @@ import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.container.jdisc.LoggingRequestHandler;
 import com.yahoo.container.logging.AccessLog;
+import com.yahoo.log.LogLevel;
 import com.yahoo.restapi.ErrorResponse;
 import com.yahoo.restapi.JacksonJsonResponse;
 import com.yahoo.restapi.Path;
@@ -55,14 +56,19 @@ public class SystemFlagsHandler extends LoggingRequestHandler {
     }
 
     private HttpResponse deploy(HttpRequest request, boolean dryRun) {
-        // TODO Error handling
-        String contentType = request.getHeader("Content-Type");
-        if (!contentType.equalsIgnoreCase("application/zip")) {
-            return ErrorResponse.badRequest("Invalid content type: " + contentType);
+        try {
+            String contentType = request.getHeader("Content-Type");
+            if (!contentType.equalsIgnoreCase("application/zip")) {
+                return ErrorResponse.badRequest("Invalid content type: " + contentType);
+            }
+            SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromZip(request.getData());
+            SystemFlagsDeployResult result = deployer.deployFlags(archive, dryRun);
+            return new JacksonJsonResponse<>(200, result.toWire());
+        } catch (Exception e) {
+            String errorMessage = "System flags deploy failed: " + e.getMessage();
+            log.log(LogLevel.ERROR, errorMessage, e);
+            return ErrorResponse.internalServerError(errorMessage);
         }
-        SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromZip(request.getData());
-        SystemFlagsDeployResult result = deployer.deployFlags(archive, dryRun);
-        return new JacksonJsonResponse<>(200, result.toWire());
     }
 
 }
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 42e33bc2f8f..596f056758d 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
@@ -11,13 +11,16 @@ import com.yahoo.vespa.hosted.controller.integration.ZoneRegistryMock;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.util.List;
 import java.util.Set;
 
 import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.FlagDataChange;
+import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.OperationError;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -27,17 +30,18 @@ import static org.mockito.Mockito.when;
 public class SystemFlagsDeployerTest {
 
     private static final SystemName SYSTEM = SystemName.main;
+    private static final FlagId FLAG_ID = new FlagId("my-flag");
 
-    @Test
-    public void deploys_flag_data_to_targets() throws IOException {
-        ZoneApiMock prodUsWest1Zone = ZoneApiMock.fromId("prod.us-west-1");
-        ZoneApiMock prodUsEast3Zone = ZoneApiMock.fromId("prod.us-east-3");
-        ZoneRegistryMock registry = new ZoneRegistryMock(SYSTEM).setZones(prodUsWest1Zone, prodUsEast3Zone);
+    private final ZoneApiMock prodUsWest1Zone = ZoneApiMock.fromId("prod.us-west-1");
+    private final ZoneApiMock prodUsEast3Zone = ZoneApiMock.fromId("prod.us-east-3");
+    private final ZoneRegistryMock registry = new ZoneRegistryMock(SYSTEM).setZones(prodUsWest1Zone, prodUsEast3Zone);
 
-        FlagsTarget controllerTarget = FlagsTarget.forController(SYSTEM);
-        FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone.getId());
-        FlagsTarget prodUsEast3Target = FlagsTarget.forConfigServer(registry, prodUsEast3Zone.getId());
+    private final FlagsTarget controllerTarget = FlagsTarget.forController(SYSTEM);
+    private final FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone.getId());
+    private final FlagsTarget prodUsEast3Target = FlagsTarget.forConfigServer(registry, prodUsEast3Zone.getId());
 
+    @Test
+    public void deploys_flag_data_to_targets() throws IOException {
         FlagsClient flagsClient = mock(FlagsClient.class);
         when(flagsClient.listFlagData(controllerTarget)).thenReturn(List.of());
         when(flagsClient.listFlagData(prodUsWest1Target)).thenReturn(List.of(flagData("existing-prod.us-west-1.json")));
@@ -61,11 +65,56 @@ public class SystemFlagsDeployerTest {
         verify(flagsClient).putFlagData(prodUsEast3Target, prodUsEast3Data);
         verify(flagsClient, never()).putFlagData(prodUsWest1Target, defaultData);
         List<FlagDataChange> changes = result.flagChanges();
-        FlagId flagId = new FlagId("my-flag");
+
         assertThat(changes).containsOnly(
-                FlagDataChange.created(flagId, Set.of(controllerTarget), defaultData),
-                FlagDataChange.updated(flagId, Set.of(prodUsEast3Target), prodUsEast3Data, existingProdUsEast3Data));
+                FlagDataChange.created(FLAG_ID, controllerTarget, defaultData),
+                FlagDataChange.updated(FLAG_ID, prodUsEast3Target, prodUsEast3Data, existingProdUsEast3Data));
+    }
+
+    @Test
+    public void dryrun_should_not_change_flags() throws IOException {
+        FlagsClient flagsClient = mock(FlagsClient.class);
+        when(flagsClient.listFlagData(controllerTarget)).thenReturn(List.of());
+
+        FlagData defaultData = flagData("flags/my-flag/main.json");
+        SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder()
+                .addFile("main.json", defaultData)
+                .build();
+
+        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, Set.of(controllerTarget));
+        SystemFlagsDeployResult result = deployer.deployFlags(archive, true);
+
+        verify(flagsClient, times(1)).listFlagData(controllerTarget);
+        verify(flagsClient, never()).putFlagData(controllerTarget, defaultData);
+        verify(flagsClient, never()).deleteFlagData(controllerTarget, FLAG_ID);
+
+        assertThat(result.flagChanges()).containsOnly(
+                FlagDataChange.created(FLAG_ID, controllerTarget, defaultData));
+        assertThat(result.errors()).isEmpty();
+    }
+
+    @Test
+    public void creates_error_entries_in_result_if_flag_data_operations_fail() throws IOException {
+        FlagsClient flagsClient = mock(FlagsClient.class);
+        UncheckedIOException exception = new UncheckedIOException(new IOException("I/O error message"));
+        when(flagsClient.listFlagData(prodUsWest1Target)).thenThrow(exception);
+        when(flagsClient.listFlagData(prodUsEast3Target)).thenReturn(List.of());
+
+        FlagData defaultData = flagData("flags/my-flag/main.json");
+        SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder()
+                .addFile("main.json", defaultData)
+                .build();
+
+        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, Set.of(prodUsWest1Target, prodUsEast3Target));
+
+        SystemFlagsDeployResult result = deployer.deployFlags(archive, false);
+
+        System.out.println(result);
 
+        assertThat(result.errors()).containsOnly(
+                OperationError.listFailed(exception.getMessage(), prodUsWest1Target));
+        assertThat(result.flagChanges()).containsOnly(
+                FlagDataChange.created(FLAG_ID, prodUsEast3Target, defaultData));
     }
 
     private static FlagData flagData(String filename) throws IOException {