Merge pull request #19087 from vespa-engine/freva/remove-node-agent

Ensure unique NodeAgent ID for reallocations

5 files changed, 112 insertions, 4 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/Event.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/Event.java
new file mode 100644
index 00000000000..ca374533940
--- /dev/null
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/Event.java
@@ -0,0 +1,54 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.node.admin.configserver.noderepository;
+
+import java.time.Instant;
+import java.util.Objects;
+
+/**
+ * @author freva
+ */
+public class Event {
+    private final String agent;
+    private final String type;
+    private final Instant at;
+
+    public Event(String agent, String type, Instant at) {
+        this.agent = Objects.requireNonNull(agent);
+        this.type = Objects.requireNonNull(type);
+        this.at = Objects.requireNonNull(at);
+    }
+
+    public String agent() {
+        return agent;
+    }
+
+    public String type() {
+        return type;
+    }
+
+    public Instant at() {
+        return at;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        Event event1 = (Event) o;
+        return agent.equals(event1.agent) && type.equals(event1.type) && at.equals(event1.at);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(agent, type, at);
+    }
+
+    @Override
+    public String toString() {
+        return "Event{" +
+                "agent='" + agent + '\'' +
+                ", type='" + type + '\'' +
+                ", at=" + at +
+                '}';
+    }
+}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeSpec.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeSpec.java
index 30bc1ef5ea3..e85d51ef992 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeSpec.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeSpec.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.node.admin.task.util.file.DiskSize;
 import java.net.URI;
 import java.time.Instant;
 import java.util.EnumSet;
+import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -60,6 +61,7 @@ public class NodeSpec {
     private final Set<String> additionalIpAddresses;
 
     private final NodeReports reports;
+    private final List<Event> events;
 
     private final Optional<String> parentHostname;
     private final Optional<URI> archiveUri;
@@ -93,6 +95,7 @@ public class NodeSpec {
             Set<String> ipAddresses,
             Set<String> additionalIpAddresses,
             NodeReports reports,
+            List<Event> events,
             Optional<String> parentHostname,
             Optional<URI> archiveUri,
             Optional<ApplicationId> exclusiveTo) {
@@ -128,9 +131,10 @@ public class NodeSpec {
         this.currentFirmwareCheck = Objects.requireNonNull(currentFirmwareCheck);
         this.resources = Objects.requireNonNull(resources);
         this.realResources = Objects.requireNonNull(realResources);
-        this.ipAddresses = Objects.requireNonNull(ipAddresses);
-        this.additionalIpAddresses = Objects.requireNonNull(additionalIpAddresses);
+        this.ipAddresses = Set.copyOf(ipAddresses);
+        this.additionalIpAddresses = Set.copyOf(additionalIpAddresses);
         this.reports = Objects.requireNonNull(reports);
+        this.events = List.copyOf(events);
         this.parentHostname = Objects.requireNonNull(parentHostname);
         this.archiveUri = Objects.requireNonNull(archiveUri);
         this.exclusiveTo = Objects.requireNonNull(exclusiveTo);
@@ -263,6 +267,10 @@ public class NodeSpec {
 
     public NodeReports reports() { return reports; }
 
+    public List<Event> events() {
+        return events;
+    }
+
     public Optional<String> parentHostname() {
         return parentHostname;
     }
@@ -308,6 +316,7 @@ public class NodeSpec {
                 Objects.equals(ipAddresses, that.ipAddresses) &&
                 Objects.equals(additionalIpAddresses, that.additionalIpAddresses) &&
                 Objects.equals(reports, that.reports) &&
+                Objects.equals(events, that.events) &&
                 Objects.equals(parentHostname, that.parentHostname) &&
                 Objects.equals(archiveUri, that.archiveUri) &&
                 Objects.equals(exclusiveTo, that.exclusiveTo);
@@ -342,6 +351,7 @@ public class NodeSpec {
                 ipAddresses,
                 additionalIpAddresses,
                 reports,
+                events,
                 parentHostname,
                 archiveUri,
                 exclusiveTo);
@@ -376,6 +386,7 @@ public class NodeSpec {
                 + " ipAddresses=" + ipAddresses
                 + " additionalIpAddresses=" + additionalIpAddresses
                 + " reports=" + reports
+                + " events=" + events
                 + " parentHostname=" + parentHostname
                 + " archiveUri=" + archiveUri
                 + " exclusiveTo=" + exclusiveTo
@@ -409,6 +420,7 @@ public class NodeSpec {
         private Set<String> ipAddresses = Set.of();
         private Set<String> additionalIpAddresses = Set.of();
         private NodeReports reports = new NodeReports();
+        private List<Event> events = List.of();
         private Optional<String> parentHostname = Optional.empty();
         private Optional<URI> archiveUri = Optional.empty();
         private Optional<ApplicationId> exclusiveTo = Optional.empty();
@@ -428,6 +440,7 @@ public class NodeSpec {
             currentRebootGeneration(node.currentRebootGeneration);
             orchestratorStatus(node.orchestratorStatus);
             reports(new NodeReports(node.reports));
+            events(node.events);
             node.wantedDockerImage.ifPresent(this::wantedDockerImage);
             node.currentDockerImage.ifPresent(this::currentDockerImage);
             node.wantedVespaVersion.ifPresent(this::wantedVespaVersion);
@@ -600,6 +613,11 @@ public class NodeSpec {
             return this;
         }
 
+        public Builder events(List<Event> events) {
+            this.events = events;
+            return this;
+        }
+
         public Builder parentHostname(String parentHostname) {
             this.parentHostname = Optional.of(parentHostname);
             return this;
@@ -714,6 +732,10 @@ public class NodeSpec {
             return reports;
         }
 
+        public List<Event> events() {
+            return events;
+        }
+
         public Optional<String> parentHostname() {
             return parentHostname;
         }
@@ -730,7 +752,7 @@ public class NodeSpec {
                     wantedRebootGeneration, currentRebootGeneration,
                     wantedFirmwareCheck, currentFirmwareCheck, modelName,
                     resources, realResources, ipAddresses, additionalIpAddresses,
-                    reports, parentHostname, archiveUri, exclusiveTo);
+                    reports, events, parentHostname, archiveUri, exclusiveTo);
         }
 
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java
index 50c39d5407c..0ea9be96fd7 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java
@@ -178,6 +178,7 @@ public class RealNodeRepository implements NodeRepository {
                 node.ipAddresses,
                 node.additionalIpAddresses,
                 reports,
+                node.history.stream().map(event -> new Event(event.agent, event.event, Instant.ofEpochMilli(event.at))).collect(Collectors.toUnmodifiableList()),
                 Optional.ofNullable(node.parentHostname),
                 Optional.ofNullable(node.archiveUri).map(URI::create),
                 Optional.ofNullable(node.exclusiveTo).map(ApplicationId::fromSerializedForm));
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/bindings/NodeRepositoryNode.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/bindings/NodeRepositoryNode.java
index 86caab9bf51..1165dc9f558 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/bindings/NodeRepositoryNode.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/bindings/NodeRepositoryNode.java
@@ -6,6 +6,7 @@ import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.JsonNode;
 
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -82,6 +83,8 @@ public class NodeRepositoryNode {
     public String archiveUri;
     @JsonProperty("exclusiveTo")
     public String exclusiveTo;
+    @JsonProperty("history")
+    public List<Event> history;
 
     @JsonProperty("reports")
     public Map<String, JsonNode> reports = null;
@@ -123,6 +126,7 @@ public class NodeRepositoryNode {
                 ", archiveUri=" + archiveUri +
                 ", reports=" + reports +
                 ", exclusiveTo=" + exclusiveTo +
+                ", history=" + history +
                 '}';
     }
 
@@ -198,4 +202,21 @@ public class NodeRepositoryNode {
         }
     }
 
+    public static class Event {
+        @JsonProperty
+        public String agent;
+        @JsonProperty
+        public String event;
+        @JsonProperty
+        public long at;
+
+        @Override
+        public String toString() {
+            return "Event{" +
+                    "agent=" + agent +
+                    ", event=" + event +
+                    ", at=" + at +
+                    '}';
+        }
+    }
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
index 8c8f3d88a71..36668158dd6 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
@@ -79,7 +79,7 @@ public class NodeAdminImpl implements NodeAdmin {
     @Override
     public void refreshContainersToRun(Set<NodeAgentContext> nodeAgentContexts) {
         Map<String, NodeAgentContext> nodeAgentContextsByHostname = nodeAgentContexts.stream()
-                .collect(Collectors.toMap(nac -> nac.hostname().value(), Function.identity()));
+                .collect(Collectors.toMap(NodeAdminImpl::nodeAgentId, Function.identity()));
 
         // Stop and remove NodeAgents that should no longer be running
         diff(nodeAgentWithSchedulerByHostname.keySet(), nodeAgentContextsByHostname.keySet())
@@ -222,4 +222,14 @@ public class NodeAdminImpl implements NodeAdmin {
         NodeAgent nodeAgent = nodeAgentFactory.create(contextManager, context);
         return new NodeAgentWithScheduler(nodeAgent, contextManager);
     }
+
+    private static String nodeAgentId(NodeAgentContext nac) {
+        // NodeAgentImpl has some internal state that should not be reused when the same hostname is re-allocated
+        // to a different application/cluster, solve this by including reservation timestamp in the key.
+        return nac.hostname().value() + "-" + nac.node().events().stream()
+                .filter(event -> "reserved".equals(event.type()))
+                .findFirst()
+                .map(event -> Long.toString(event.at().toEpochMilli()))
+                .orElse("");
+    }
 }