Refactored deployment issues >_<

1 files changed, 57 insertions, 132 deletions

diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java
index f8d09ac8b27..aa93fb1cfe2 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java
@@ -1,13 +1,11 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.maintenance;
 
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.vespa.hosted.controller.Application;
-import com.yahoo.vespa.hosted.controller.api.integration.Contacts.UserContact;
-import com.yahoo.vespa.hosted.controller.api.integration.Issues;
-import com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo;
-import com.yahoo.vespa.hosted.controller.api.integration.stubs.ContactsMock;
-import com.yahoo.vespa.hosted.controller.api.integration.stubs.PropertiesMock;
+import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId;
+import com.yahoo.vespa.hosted.controller.api.integration.stubs.LoggingDeploymentIssues;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
@@ -15,28 +13,18 @@ import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb;
 import org.junit.Before;
 import org.junit.Test;
 
-import java.time.Clock;
 import java.time.Duration;
-import java.util.Arrays;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
-import java.util.Optional;
-import java.util.stream.Collectors;
 
-import static com.yahoo.vespa.hosted.controller.api.integration.Contacts.Category.admin;
-import static com.yahoo.vespa.hosted.controller.api.integration.Contacts.Category.engineeringOwner;
-import static com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo.Status.done;
-import static com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo.Status.toDo;
 import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.component;
 import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionCorpUsEast1;
 import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.stagingTest;
 import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.systemTest;
 import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.maxFailureAge;
-import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.maxInactivityAge;
-import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.terminalUser;
-import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.vespaOps;
+import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.maxInactivity;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -51,27 +39,18 @@ public class DeploymentIssueReporterTest {
 
     private DeploymentTester tester;
     private DeploymentIssueReporter reporter;
-    private ContactsMock contacts;
-    private PropertiesMock properties;
-    private MockIssues issues;
+    private MockDeploymentIssues issues;
 
     @Before
     public void setup() {
         tester = new DeploymentTester();
-        contacts = new ContactsMock();
-        properties = new PropertiesMock();
-        issues = new MockIssues(tester.clock());
-        reporter = new DeploymentIssueReporter(tester.controller(), contacts, properties, issues, Duration.ofMinutes(5), 
-                                               new JobControl(new MockCuratorDb()));
-    }
-
-    private List<IssueInfo> openIssuesFor(Application application) {
-        return issues.fetchSimilarTo(reporter.deploymentIssueFrom(tester.controller().applications().require(application.id())));
+        issues = new MockDeploymentIssues();
+        reporter = new DeploymentIssueReporter(tester.controller(), issues, Duration.ofMinutes(5), new JobControl(new MockCuratorDb()));
     }
 
     @Test
     public void testDeploymentFailureReporting() {
-        // All applications deploy from unique SD projects.
+        // All applications deploy from unique build projects.
         Long projectId1 = 10L;
         Long projectId2 = 20L;
         Long projectId3 = 30L;
@@ -79,11 +58,12 @@ public class DeploymentIssueReporterTest {
         // Only the first two have propertyIds set now.
         Long propertyId1 = 1L;
         Long propertyId2 = 2L;
+        Long propertyId3 = 3L;
 
         // Create and deploy one application for each of three tenants.
         Application app1 = tester.createApplication("application1", "tenant1", projectId1, propertyId1);
         Application app2 = tester.createApplication("application2", "tenant2", projectId2, propertyId2);
-        Application app3 = tester.createApplication("application3", "tenant3", projectId3, null);
+        Application app3 = tester.createApplication("application3", "tenant3", projectId3, propertyId3);
 
         // And then we need lots of successful applications, so we won't assume we just have a faulty Vespa out.
         for (long i = 4; i <= 10; i++) {
@@ -93,18 +73,6 @@ public class DeploymentIssueReporterTest {
             tester.deployAndNotify(app, applicationPackage, true, stagingTest);
             tester.deployAndNotify(app, applicationPackage, true, productionCorpUsEast1);
         }
-
-        // Both the first tenants belong to the same JIRA queue. (Not sure if this is possible, but let's test it anyway.
-        String jiraQueue = "PROJECT";
-        properties.addClassification(propertyId1, jiraQueue);
-        properties.addClassification(propertyId1, jiraQueue);
-
-        // Only tenant1 has contacts listed in opsDb.
-        UserContact
-                alice = new UserContact("alice", "Alice", admin),
-                bob = new UserContact("bob", "Robert", engineeringOwner);
-        contacts.addContact(propertyId1, Arrays.asList(alice, bob));
-
         // end of setup.
 
         // NOTE: All maintenance should be idempotent within a small enough time interval, so maintain is called twice in succession throughout.
@@ -125,7 +93,7 @@ public class DeploymentIssueReporterTest {
 
         reporter.maintain();
         reporter.maintain();
-        assertEquals("No deployments are detected as failing for a long time initially.", 0, issues.issues.size());
+        assertEquals("No deployments are detected as failing for a long time initially.", 0, issues.size());
 
 
         // Advance to where deployment issues should be detected.
@@ -133,146 +101,103 @@ public class DeploymentIssueReporterTest {
 
         reporter.maintain();
         reporter.maintain();
-        assertEquals("One issue is produced for app1.", 1, openIssuesFor(app1).size());
-        assertEquals("No issues are produced for app2.", 0, openIssuesFor(app2).size());
-        assertEquals("One issue is produced for app3.", 1, openIssuesFor(app3).size());
-        assertTrue("The issue for app1 is stored in their JIRA queue.", openIssuesFor(app1).get(0).key().startsWith(jiraQueue));
-        assertTrue("The issue for an application without propertyId is addressed to vespaOps.", openIssuesFor(app3).get(0).key().startsWith(vespaOps.queue()));
+        assertTrue("One issue is produced for app1.", issues.isOpenFor(app1.id()));
+        assertFalse("No issues are produced for app2.", issues.isOpenFor(app2.id()));
+        assertTrue("One issue is produced for app3.", issues.isOpenFor(app3.id()));
 
 
-        // Verify idempotency of filing.
-        reporter.maintain();
-        reporter.maintain();
-        assertEquals("No issues are re-filed when still open.", 2, issues.issues.size());
-
-
-        // tenant3 closes their issue prematurely; see that we get a new filing.
-        issues.complete(openIssuesFor(app3).get(0).id());
-        assertEquals("The issue is removed (test of the tester, really...).", 0, openIssuesFor(app3).size());
+        // app3 closes their issue prematurely; see that it is refiled.
+        issues.closeFor(app3.id());
+        assertFalse("No issue is open for app3.", issues.isOpenFor(app3.id()));
 
         reporter.maintain();
         reporter.maintain();
-        assertTrue("Issue is re-produced for app3, addressed correctly.", openIssuesFor(app3).get(0).key().startsWith(vespaOps.queue()));
+        assertTrue("Issue is re-filed for app3.", issues.isOpenFor(app3.id()));
 
 
         // Some time passes; tenant1 leaves her issue unattended, while tenant3 starts work and updates the issue.
         // app2 also has an intermittent failure; see that we detect this as a Vespa problem, and file an issue to ourselves.
         tester.deployAndNotify(app2, applicationPackage, false, productionCorpUsEast1);
-        tester.clock().advance(maxInactivityAge.plus(maxFailureAge));
-        issues.comment(openIssuesFor(app3).get(0).id(), "We are trying to fix it!");
-
-        reporter.maintain();
-        reporter.maintain();
-        assertEquals("The issue for app1 is escalated once.", alice.username(), openIssuesFor(app1).get(0).assignee().get());
-
+        tester.clock().advance(maxInactivity.plus(maxFailureAge));
+        issues.touchFor(app3.id());
 
+        assertFalse("We have no platform issues initially.", issues.platformIssue());
         reporter.maintain();
         reporter.maintain();
-        assertEquals("We get an issue to vespaOps when more than 20% of applications have old failures.", 1,
-                issues.fetchSimilarTo(reporter.manyFailingDeploymentsIssueFrom(Arrays.asList(
-                        tester.controller().applications().get(app1.id()).get(),
-                        tester.controller().applications().get(app2.id()).get(),
-                        tester.controller().applications().get(app3.id()).get()))).size());
-        assertEquals("No issue is filed for app2 while Vespa is considered broken.", 0, openIssuesFor(app2).size());
+        assertEquals("The issue for app1 is escalated once.", 1, issues.escalationLevelFor(app1.id()));
+        assertTrue("We get a platform issue when more than 20% of applications are failing.", issues.platformIssue());
+        assertFalse("No issue is filed for app2 while Vespa is considered broken.", issues.isOpenFor(app2.id()));
 
 
         // app3 fixes its problem, but the ticket is left open; see the resolved ticket is not escalated when another escalation period has passed.
         tester.deployAndNotify(app2, applicationPackage, true, productionCorpUsEast1);
         tester.deployAndNotify(app3, applicationPackage, true, productionCorpUsEast1);
-        tester.clock().advance(maxInactivityAge.plus(Duration.ofDays(1)));
+        tester.clock().advance(maxInactivity.plus(Duration.ofDays(1)));
 
         reporter.maintain();
         reporter.maintain();
-        assertEquals("The issue for app1 is escalated once more.", bob.username(), openIssuesFor(app1).get(0).assignee().get());
-        assertEquals("The issue for app3 is still unassigned.", Optional.empty(), openIssuesFor(app3).get(0).assignee());
+        assertFalse("We no longer have a platform issue.", issues.platformIssue());
+        assertEquals("The issue for app1 is escalated once more.", 2, issues.escalationLevelFor(app1.id()));
+        assertEquals("The issue for app3 is not escalated.", 0, issues.escalationLevelFor(app3.id()));
 
 
-        // app1 still does nothing with their issue; see the terminal user gets it in the end.
         // app3 now has a new failure past max failure age; see that a new issue is filed.
         tester.notifyJobCompletion(component, app3, true);
         tester.deployAndNotify(app3, applicationPackage, true, systemTest);
         tester.deployAndNotify(app3, applicationPackage, true, stagingTest);
         tester.deployAndNotify(app3, applicationPackage, false, productionCorpUsEast1);
-        tester.clock().advance(maxInactivityAge.plus(maxFailureAge));
+        tester.clock().advance(maxInactivity.plus(maxFailureAge));
 
         reporter.maintain();
         reporter.maintain();
-        assertEquals("The issue for app1 is escalated to the terminal user.", terminalUser.username(), openIssuesFor(app1).get(0).assignee().get());
-        assertEquals("A new issue is filed for app3.", 2, openIssuesFor(app3).size());
+        assertTrue("A new issue is filed for app3.", issues.isOpenFor(app3.id()));
     }
 
-    class MockIssues implements Issues {
 
-        final Map<String, Issue> issues = new HashMap<>();
-        final Map<String, IssueInfo> metas = new HashMap<>();
-        final Map<String, Long> counters = new HashMap<>();
-        Clock clock;
+    class MockDeploymentIssues extends LoggingDeploymentIssues {
 
-        MockIssues(Clock clock) { this.clock = clock; }
+        Map<ApplicationId, IssueId> applicationIssues = new HashMap<>();
+        Map<IssueId, Integer> issueLevels = new HashMap<>();
 
-        public void addWatcher(String jiraIssueId, String watcher) {
-            touch(jiraIssueId);
+        MockDeploymentIssues() {
+            super(tester.clock());
         }
 
-        public void reassign(String jiraIssueId, String assignee) {
-            metas.compute(jiraIssueId, (__, jiraIssueMeta) ->
-                    new IssueInfo(
-                            jiraIssueId,
-                            jiraIssueMeta.key(),
-                            clock.instant(),
-                            Optional.of(assignee),
-                            jiraIssueMeta.status()));
+        @Override
+        protected void escalateIssue(IssueId issueId) {
+            super.escalateIssue(issueId);
+            issueLevels.merge(issueId, 1, Integer::sum);
         }
 
-        public void comment(String jiraIssueId, String comment) {
-            touch(jiraIssueId);
+        @Override
+        protected IssueId fileIssue(ApplicationId applicationId) {
+            IssueId issueId = super.fileIssue(applicationId);
+            applicationIssues.put(applicationId, issueId);
+            return issueId;
         }
 
-        public void update(String jiraIssueId, String description) {
-            issues.compute(jiraIssueId, (__, issue) ->
-                    new Issue(issue.summary(), description, issue.classification().orElse(null)));
+        void closeFor(ApplicationId applicationId) {
+            issueUpdates.remove(applicationIssues.remove(applicationId));
         }
 
-        public String file(Issue issue) {
-            String jiraIssueId = (issues.size() + 1L) + "";
-            Long counter = counters.merge(issue.classification().get().queue(), 0L, (old, __) -> old + 1);
-            String jiraIssueKey = issue.classification().get().queue() + '-' + counter;
-            issues.put(jiraIssueId, issue);
-            metas.put(jiraIssueId, new IssueInfo(jiraIssueId, jiraIssueKey, clock.instant(), null, toDo));
-            return jiraIssueId;
+        void touchFor(ApplicationId applicationId) {
+            issueUpdates.put(applicationIssues.get(applicationId), tester.clock().instant());
         }
 
-        public IssueInfo fetch(String jiraIssueId) {
-            return metas.get(jiraIssueId);
+        boolean isOpenFor(ApplicationId applicationId) {
+            return applicationIssues.containsKey(applicationId);
         }
 
-        public List<IssueInfo> fetchSimilarTo(Issue issue) {
-            return issues.entrySet().stream()
-                    .filter(entry -> entry.getValue().summary().equals(issue.summary()))
-                    .map(Map.Entry::getKey)
-                    .map(metas::get)
-                    .filter(meta -> meta.status() != done)
-                    .collect(Collectors.toList());
+        int escalationLevelFor(ApplicationId applicationId) {
+            return issueLevels.getOrDefault(applicationIssues.get(applicationId), 0);
         }
 
-        private void complete(String jiraIssueId) {
-            metas.compute(jiraIssueId, (__, jiraIssueMeta) ->
-                    new IssueInfo(
-                            jiraIssueId,
-                            jiraIssueMeta.key(),
-                            clock.instant(),
-                            jiraIssueMeta.assignee(),
-                            done));
+        int size() {
+            return issueUpdates.size();
         }
 
-        private void touch(String jiraIssueId) {
-            metas.compute(jiraIssueId, (__, jiraIssueMeta) ->
-                    new IssueInfo(
-                            jiraIssueId,
-                            jiraIssueMeta.key(),
-                            clock.instant(),
-                            jiraIssueMeta.assignee(),
-                            jiraIssueMeta.status()));
+        boolean platformIssue() {
+            return platformIssue.get();
         }
 
     }