Remove exec from Watches, add Status(Un)Healthy things Watch publishes

config/config_test.go

@@ -55,7 +55,7 @@ func TestValidConfigJobs(t *testing.T) {
 		t.Fatalf("unexpected error in LoadConfig: %v", err)
 	}
 
-	if len(cfg.Jobs) != 8 {
+	if len(cfg.Jobs) != 10 {
 		t.Fatalf("expected 8 services but got %v", cfg.Jobs)
 	}
 	job0 := cfg.Jobs[0]
@@ -103,8 +103,20 @@ func TestValidConfigJobs(t *testing.T) {
 	assertEqual(t, job6.Restarts, nil, "expected '%v' for job6.Restarts but got '%v'")
 
 	job7 := cfg.Jobs[7]
-	assertEqual(t, job7.Name, "containerpilot", "expected '%v' for job7.Name but got '%v'")
-	assertEqual(t, job7.Port, 9000, "expected '%v' for job7.Port but got '%v'")
+	assertEqual(t, job7.Name, "onChange-upstreamA", "expected '%v' for job7.Name but got '%v'")
+	assertEqual(t, job7.Port, 0, "expected '%v' for job7.Port but got '%v'")
+	assertEqual(t, job7.Frequency, "", "expected '%v' for job7.Frequency but got '%v'")
+	assertEqual(t, job7.Restarts, nil, "expected '%v' for job7.Restarts but got '%v'")
+
+	job8 := cfg.Jobs[8]
+	assertEqual(t, job8.Name, "onChange-upstreamB", "expected '%v' for job8.Name but got '%v'")
+	assertEqual(t, job8.Port, 0, "expected '%v' for job8.Port but got '%v'")
+	assertEqual(t, job8.Frequency, "", "expected '%v' for job8.Frequency but got '%v'")
+	assertEqual(t, job8.Restarts, nil, "expected '%v' for job8.Restarts but got '%v'")
+
+	job9 := cfg.Jobs[9]
+	assertEqual(t, job9.Name, "containerpilot", "expected '%v' for job9.Name but got '%v'")
+	assertEqual(t, job9.Port, 9000, "expected '%v' for job9.Port but got '%v'")
 }
 
 // telemetry.Config
@@ -136,10 +148,10 @@ func TestValidConfigWatches(t *testing.T) {
 	}
 	watch0 := cfg.Watches[0]
 	watch1 := cfg.Watches[1]
-	assertEqual(t, watch0.Name, "upstreamA.watch", "expected '%v' for Name, but got '%v'")
+	assertEqual(t, watch0.Name, "watch.upstreamA", "expected '%v' for Name, but got '%v'")
 	assertEqual(t, watch0.Poll, 11, "expected '%v' for Poll, but got '%v'")
 	assertEqual(t, watch0.Tag, "dev", "expected '%v' for Tag, but got '%v'")
-	assertEqual(t, watch1.Name, "upstreamB.watch", "expected '%v' for Name, but got '%v'")
+	assertEqual(t, watch1.Name, "watch.upstreamB", "expected '%v' for Name, but got '%v'")
 	assertEqual(t, watch1.Poll, 79, "expected '%v' for Poll, but got '%v'")
 	assertEqual(t, watch1.Tag, "", "expected '%v' for Tag, but got '%v'")
 

config/template_test.go

@@ -50,10 +50,7 @@ func TestRenderConfigFileStdout(t *testing.T) {
 
 	var testJSON = `{
 	"consul": "consul:8500",
-	"backends": [{
-					"name": "upstreamA",
-					"poll": 11,
-					"onChange": "/bin/to/onChangeEvent/for/upstream/A.sh"}]}`
+	"backends": [{"name": "upstreamA", "poll": 11}]}`
 
 	// Render to file
 	defer os.Remove("testJSON.json")
@@ -86,10 +83,7 @@ func TestRenderedConfigIsParseable(t *testing.T) {
 
 	var testJSON = `{
 	"consul": "consul:8500",
-	"backends": [{
-					"name": "upstreamA{{.TESTRENDERCONFIGISPARSEABLE}}",
-					"poll": 11,
-					"onChange": "/bin/to/onChangeEvent/for/upstream/A.sh"}]}`
+	"backends": [{"name": "upstreamA{{.TESTRENDERCONFIGISPARSEABLE}}", "poll": 11}]}`
 
 	os.Setenv("TESTRENDERCONFIGISPARSEABLE", "-ok")
 	template, _ := renderConfigTemplate(testJSON)
@@ -98,7 +92,7 @@ func TestRenderedConfigIsParseable(t *testing.T) {
 		t.Fatalf("unexpected error in LoadConfig: %v", err)
 	}
 	name := config.Watches[0].Name
-	if name != "upstreamA-ok.watch" {
+	if name != "watch.upstreamA-ok" {
 		t.Fatalf("expected Watches[0] name to be upstreamA-ok but got %s", name)
 	}
 }

config/testdata/test.json5

@@ -55,19 +55,25 @@
         source: "serviceA",
         event: "stopped"
       }
+    },
+    {
+      name: "onChange-upstreamA",
+      exec: "/bin/to/onChangeEvent/for/upstream/A.sh {{.TEST}}",
+    },
+    {
+      name: "onChange-upstreamB",
+      exec: "/bin/to/onChangeEvent/for/upstream/B.sh {{.ENV_NOT_FOUND}}"
     }
   ],
   "backends": [
       {
           "name": "upstreamA",
           "poll": 11,
-          "onChange": "/bin/to/onChangeEvent/for/upstream/A.sh {{.TEST}}",
           "tag": "dev"
       },
       {
           "name": "upstreamB",
-          "poll": 79,
-          "onChange": "/bin/to/onChangeEvent/for/upstream/B.sh {{.ENV_NOT_FOUND}}"
+          "poll": 79
       }
   ],
   "telemetry": {

core/app_test.go

@@ -39,19 +39,12 @@ func TestServiceConfigRequiredFields(t *testing.T) {
 
 func TestBackendConfigRequiredFields(t *testing.T) {
 	// Missing `name`
-	var testJSON = `{"consul": "consul:8500", "backends": [
-                           {"name": "", "poll": 30, "onChange": "true"}]}`
+	var testJSON = `{"consul": "consul:8500", "backends": [{"name": "", "poll": 30}]}`
 	validateParseError(t, testJSON, []string{"`name`"})
 
 	// Missing `poll`
-	testJSON = `{"consul": "consul:8500", "backends": [
-                       {"name": "name", "onChange": "true"}]}`
+	testJSON = `{"consul": "consul:8500", "backends": [{"name": "name"}]}`
 	validateParseError(t, testJSON, []string{"`poll`"})
-
-	// Missing `onChange`
-	testJSON = `{"consul": "consul:8500", "backends": [
-                       {"name": "name", "poll": 19 }]}`
-	validateParseError(t, testJSON, []string{"`onChange`"})
 }
 
 func TestInvalidConfigNoConfigFlag(t *testing.T) {

discovery/consul/consul.go

@@ -175,19 +175,22 @@ func (c *Consul) registerCheck(service discovery.ServiceDefinition) error {
 var upstreams = make(map[string][]*consul.ServiceEntry)
 
 // CheckForUpstreamChanges runs the health check
-func (c Consul) CheckForUpstreamChanges(backendName, backendTag string) bool {
+func (c Consul) CheckForUpstreamChanges(backendName, backendTag string) (didChange, isHealthy bool) {
 	services, meta, err := c.Health().Service(backendName, backendTag, true, nil)
 	if err != nil {
 		log.Warnf("failed to query %v: %s [%v]", backendName, err, meta)
-		return false
+		return false, false
 	}
-	didChange := compareForChange(upstreams[backendName], services)
+	if len(services) > 0 {
+		isHealthy = true
+	}
+	didChange = compareForChange(upstreams[backendName], services)
 	if didChange || len(services) == 0 {
 		// We don't want to cause an onChange event the first time we read-in
 		// but we do want to make sure we've written the key for this map
 		upstreams[backendName] = services
 	}
-	return didChange
+	return didChange, isHealthy
 }
 
 // Compare the two arrays to see if the address or port has changed

discovery/consul/consul_test.go

@@ -104,19 +104,19 @@ func testConsulCheckForChanges(t *testing.T) {
 	consul, _ := NewConsulConfig(testServer.HTTPAddr)
 	service := generateServiceDefinition(backend)
 	id := service.ID
-	if consul.CheckForUpstreamChanges(backend, "") {
+	if changed, _ := consul.CheckForUpstreamChanges(backend, ""); changed {
 		t.Fatalf("First read of %s should show `false` for change", id)
 	}
 	consul.SendHeartbeat(service) // force registration and 1st heartbeat
 
-	if !consul.CheckForUpstreamChanges(backend, "") {
+	if changed, _ := consul.CheckForUpstreamChanges(backend, ""); !changed {
 		t.Errorf("%v should have changed after first health check TTL", id)
 	}
-	if consul.CheckForUpstreamChanges(backend, "") {
+	if changed, _ := consul.CheckForUpstreamChanges(backend, ""); changed {
 		t.Errorf("%v should not have changed without TTL expiring", id)
 	}
 	consul.Agent().UpdateTTL(id, "expired", "critical")
-	if !consul.CheckForUpstreamChanges(backend, "") {
+	if changed, _ := consul.CheckForUpstreamChanges(backend, ""); !changed {
 		t.Errorf("%v should have changed after TTL expired.", id)
 	}
 }
@@ -135,7 +135,7 @@ func testConsulEnableTagOverride(t *testing.T) {
 		},
 	}
 	id := service.ID
-	if consul.CheckForUpstreamChanges(backend, "") {
+	if changed, _ := consul.CheckForUpstreamChanges(backend, ""); changed {
 		t.Fatalf("First read of %s should show `false` for change", id)
 	}
 	consul.SendHeartbeat(service) // force registration

discovery/discovery.go

@@ -5,7 +5,7 @@ import log "github.com/Sirupsen/logrus"
 // Backend is an interface which all service discovery backends must implement
 type Backend interface {
 	SendHeartbeat(service *ServiceDefinition)
-	CheckForUpstreamChanges(backendName string, backendTag string) bool
+	CheckForUpstreamChanges(backendName string, backendTag string) (bool, bool)
 	MarkForMaintenance(service *ServiceDefinition)
 	Deregister(service *ServiceDefinition)
 }

integration_tests/fixtures/app/containerpilot.json5

@@ -25,32 +25,30 @@
       "exec": "/reload-app.sh"
     },
     {
-      "name": "reload-for-nginx",
-      "when": {
-        "source": "watch.nginx",
-        "event": "changed"
+      name: "reload-for-nginx",
+      when: {
+        source: "watch.nginx",
+        event: "changed"
       },
-      "exec": "/reload-app.sh"
+      exec: "/reload-app.sh"
     },
     {
-      "name": "reload-for-app",
-      "when": {
-        "source": "watch.app",
-        "event": "changed"
+      name: "reload-for-app",
+      when: {
+        source: "watch.app",
+        event: "changed"
       },
-      "exec": "/reload-app.sh"
+      exec: "/reload-app.sh"
     }
   ],
   "backends": [
     {
       "name": "nginx",
       "poll": 7,
-      "onChange": "/reload-app.sh"
     },
     {
       "name": "app",
       "poll": 5,
-      "onChange": "/reload-app.sh",
       "tag": "application"
     }
   ],

integration_tests/fixtures/nginx/etc/nginx-with-consul.json5

@@ -23,16 +23,23 @@
       "exec": ["consul-template", "-once", "-retry", "3s",
                "-consul", "consul:8500", "-template",
                "/etc/nginx-consul.ctmpl:/etc/nginx/nginx.conf"],
+    },
+    {
+      name: "onChange",
+      when: {
+        source: "watch.app",
+        event: "changed"
+      },
+      exec: [
+          "consul-template", "-once", "-consul", "consul:8500", "-template",
+          "/etc/nginx-consul.ctmpl:/etc/nginx/nginx.conf:nginx -s reload"
+      ]
     }
   ],
   "backends": [
     {
       "name": "app",
-      "poll": 1,
-      "onChange": [
-          "consul-template", "-once", "-consul", "consul:8500", "-template",
-          "/etc/nginx-consul.ctmpl:/etc/nginx/nginx.conf:nginx -s reload"
-      ]
+      "poll": 1
     }
   ]
 }

integration_tests/tests/test_discovery_consul/containerpilot.json5

@@ -23,19 +23,32 @@
     {
       name: "preStart",
       exec: "/reload-app.sh"
+    },
+    {
+      name: "reload-for-nginx",
+      when: {
+        source: "watch.nginx",
+        event: "changed"
+      },
+      exec: "/reload-app.sh"
+    },
+    {
+      name: "reload-for-app",
+      when: {
+        source: "watch.app",
+        event: "changed"
+      },
+      exec: "/reload-app.sh"
     }
   ],
   "backends": [
     {
       "name": "nginx",
       "poll": 7,
-      "onChange": "/reload-app.sh"
     },
     {
       "name": "app",
-      "poll": 5,
-      "onChange": "/reload-app.sh",
-      "tag": "application"
+      "poll": 5
     }
   ]
 }

integration_tests/tests/test_tasks/containerpilot.json5

@@ -42,19 +42,32 @@
     {
       name: "preStart",
       exec: "/reload-app.sh"
+    },
+    {
+      name: "reload-for-nginx",
+      when: {
+        source: "watch.nginx",
+        event: "changed"
+      },
+      exec: "/reload-app.sh"
+    },
+    {
+      name: "reload-for-app",
+      when: {
+        source: "watch.app",
+        event: "changed"
+      },
+      exec: "/reload-app.sh"
     }
   ],
   "backends": [
     {
       "name": "nginx",
       "poll": 7,
-      "onChange": "/reload-app.sh"
     },
     {
       "name": "app",
-      "poll": 5,
-      "onChange": "/reload-app.sh",
-      "tag": "application"
+      "poll": 5
     }
   ],
   "telemetry": {

tests/mocks/discovery.go

@@ -4,7 +4,8 @@ import "github.com/joyent/containerpilot/discovery"
 
 // NoopDiscoveryBackend is a mock discovery.Backend
 type NoopDiscoveryBackend struct {
-	Val bool
+	Val     bool
+	lastVal bool
 }
 
 // SendHeartbeat (required for mock interface)
@@ -13,9 +14,15 @@ func (noop *NoopDiscoveryBackend) SendHeartbeat(service *discovery.ServiceDefini
 }
 
 // CheckForUpstreamChanges will return the public Val field to mock
-// whether a change has occurred.
-func (noop *NoopDiscoveryBackend) CheckForUpstreamChanges(backend, tag string) bool {
-	return noop.Val
+// whether a change has occurred. Will not report a change on the second
+// check unless the Val has been updated externally by the test rig
+func (noop *NoopDiscoveryBackend) CheckForUpstreamChanges(backend, tag string) (didChange, isHealthy bool) {
+	if noop.lastVal != noop.Val {
+		didChange = true
+	}
+	noop.lastVal = noop.Val
+	isHealthy = noop.Val
+	return didChange, isHealthy
 }
 
 // MarkForMaintenance (required for mock interface)