Merge pull request #18304 from vespa-engine/freva/clear-on-instance-removal

Remove notifications for instances no longer referenced in deployment.xml

3 files changed, 71 insertions, 0 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index c867b97b544..ff10f3b77ca 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -445,6 +445,11 @@ public class ApplicationController {
         // Validate new deployment spec thoroughly before storing it.
         controller.jobController().deploymentStatus(application.get());
 
+        // Clear notifications for instances that are no longer declared
+        for (var name : existingInstances)
+            if ( ! declaredInstances.contains(name))
+                controller.notificationsDb().removeNotifications(NotificationSource.from(application.get().id().instance(name)));
+
         store(application);
         return application;
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDb.java
index 21df0c01f0f..beb771b1aa2 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDb.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.controller.notification;
 
 import com.yahoo.collections.Pair;
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Controller;
@@ -16,6 +17,7 @@ import java.util.Comparator;
 import java.util.List;
 import java.util.Locale;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -34,6 +36,13 @@ public class NotificationsDb {
 
     public NotificationsDb(Controller controller) {
         this(controller.clock(), controller.curator());
+
+        Set<DeploymentId> allDeployments = controller.applications().asList().stream()
+                .flatMap(application -> application.instances().values().stream())
+                .flatMap(instance -> instance.deployments().keySet().stream()
+                        .map(zone -> new DeploymentId(instance.id(), zone)))
+                .collect(Collectors.toSet());
+        removeNotificationsForRemovedInstances(allDeployments);
     }
 
     NotificationsDb(Clock clock, CuratorDb curatorDb) {
@@ -41,6 +50,26 @@ public class NotificationsDb {
         this.curatorDb = curatorDb;
     }
 
+    // TODO (freva): Remove after 7.423
+    void removeNotificationsForRemovedInstances(Set<DeploymentId> allDeployments) {
+        // Prior to 7.423, notifications created for instances that were later removed by being removed from
+        // deployment.xml were not cleared. This should only affect notifications with type 'deployment'
+        allDeployments.stream()
+                .map(deploymentId -> deploymentId.applicationId().tenant())
+                .distinct()
+                .flatMap(tenant -> curatorDb.readNotifications(tenant).stream()
+                            .filter(notification -> notification.type() == Type.deployment && notification.source().zoneId().isPresent())
+                            .map(Notification::source))
+                            .filter(source -> {
+                                ApplicationId sourceApplication = ApplicationId.from(source.tenant(),
+                                                                                     source.application().get(),
+                                                                                     source.instance().get());
+                                DeploymentId sourceDeployment = new DeploymentId(sourceApplication, source.zoneId().get());
+                                return ! allDeployments.contains(sourceDeployment);
+                            })
+                            .forEach(source -> removeNotification(source, Type.deployment));
+    }
+
     public List<Notification> listNotifications(NotificationSource source, boolean productionOnly) {
         return curatorDb.readNotifications(source.tenant()).stream()
                 .filter(notification -> source.contains(notification.source()) && (!productionOnly || notification.source().isProduction()))
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDbTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDbTest.java
index 5bd7d1db769..f6c17ffcef1 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDbTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/notification/NotificationsDbTest.java
@@ -22,7 +22,9 @@ import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -156,6 +158,41 @@ public class NotificationsDbTest {
         assertEquals(expected, curatorDb.readNotifications(tenant));
     }
 
+    @Test
+    public void removes_invalid_deployment_notifications() {
+        curatorDb.deleteNotifications(tenant); // Remove notifications set in init()
+
+        ZoneId z1 = ZoneId.from("prod", "us-west-1");
+        ZoneId z2 = ZoneId.from("prod", "eu-south-2");
+        DeploymentId d1 = new DeploymentId(ApplicationId.from("t1", "a1", "i1"), z1);
+        DeploymentId d2 = new DeploymentId(ApplicationId.from("t1", "a1", "i1"), z2);
+        DeploymentId d3 = new DeploymentId(ApplicationId.from("t1", "a1", "i2"), z1);
+        DeploymentId d4 = new DeploymentId(ApplicationId.from("t1", "a2", "i1"), z2);
+        DeploymentId d5 = new DeploymentId(ApplicationId.from("t2", "a1", "i1"), z2);
+
+        List<Notification> notifications = Stream.of(d1, d2, d3, d4, d5)
+                .flatMap(deployment -> Stream.of(Type.deployment, Type.feedBlock)
+                        .map(type -> new Notification(Instant.EPOCH, type, Level.warning, NotificationSource.from(deployment), List.of("msg"))))
+                .collect(Collectors.toUnmodifiableList());
+        notifications.stream().collect(Collectors.groupingBy(notification -> notification.source().tenant(), Collectors.toList()))
+                .forEach(curatorDb::writeNotifications);
+
+        // All except d3 plus a deployment that has no notifications
+        Set<DeploymentId> allDeployments = Set.of(d1, d2, d4, d5, new DeploymentId(ApplicationId.from("t3", "a1", "i1"), z1));
+        notificationsDb.removeNotificationsForRemovedInstances(allDeployments);
+
+        List<Notification> expectedNotifications = new ArrayList<>(notifications);
+        // Only the deployment notification for d3 should be cleared (the other types already correctly clear themselves)
+        expectedNotifications.remove(4);
+
+        List<Notification> actualNotifications = curatorDb.listNotifications().stream()
+                .flatMap(tenant -> curatorDb.readNotifications(tenant).stream())
+                .collect(Collectors.toUnmodifiableList());
+
+        assertEquals(expectedNotifications.stream().map(Notification::toString).collect(Collectors.joining("\n")),
+                actualNotifications.stream().map(Notification::toString).collect(Collectors.joining("\n")));
+    }
+
     @Before
     public void init() {
         curatorDb.writeNotifications(tenant, notifications);