agent: maintenance mode api's are idempotent

command/agent/agent.go

@@ -1002,23 +1002,14 @@ func (a *Agent) unloadChecks() error {
 // EnableServiceMaintenance will register a false health check against the given
 // service ID with critical status. This will exclude the service from queries.
 func (a *Agent) EnableServiceMaintenance(serviceID string) error {
-	var service *structs.NodeService
+	service, ok := a.state.Services()[serviceID]
-	for _, svc := range a.state.Services() {
+	if !ok {
-		if svc.ID == serviceID {
-			service = svc
-		}
-	}
-
-	// Ensure the service exists
-	if service == nil {
 		return fmt.Errorf("No service registered with ID %q", serviceID)
 	}
 
 	// Ensure maintenance mode is not already enabled
-	for _, check := range a.state.Checks() {
+	if _, ok := a.state.Checks()[maintCheckID]; ok {
-		if check.CheckID == maintCheckID {
+		return nil
-			return fmt.Errorf("Maintenance mode already enabled for service %q", serviceID)
-		}
 	}
 
 	// Create and register the critical health check
@@ -1039,29 +1030,11 @@ func (a *Agent) EnableServiceMaintenance(serviceID string) error {
 // DisableServiceMaintenance will deregister the fake maintenance mode check
 // if the service has been marked as in maintenance.
 func (a *Agent) DisableServiceMaintenance(serviceID string) error {
-	var service *structs.NodeService
+	if _, ok := a.state.Services()[serviceID]; !ok {
-	for _, svc := range a.state.Services() {
-		if svc.ID == serviceID {
-			service = svc
-		}
-	}
-
-	// Ensure the service exists
-	if service == nil {
 		return fmt.Errorf("No service registered with ID %q", serviceID)
 	}
 
-	// Ensure maintenance mode is enabled
-	for _, check := range a.state.Checks() {
-		if check.CheckID == maintCheckID {
-			goto DEREGISTER
-		}
-	}
-	return fmt.Errorf("Maintenance mode not enabled for service %q", serviceID)
-
-DEREGISTER:
 	// Deregister the maintenance check
 	a.RemoveCheck(maintCheckID, true)
-
 	return nil
 }

command/agent/agent_endpoint.go

@@ -216,12 +216,12 @@ func (s *HTTPServer) AgentServiceMaintenance(resp http.ResponseWriter, req *http
 	var err error
 	if enable {
 		if err = s.agent.EnableServiceMaintenance(serviceID); err != nil {
-			resp.WriteHeader(409)
+			resp.WriteHeader(404)
 			resp.Write([]byte(err.Error()))
 		}
 	} else {
 		if err = s.agent.DisableServiceMaintenance(serviceID); err != nil {
-			resp.WriteHeader(409)
+			resp.WriteHeader(404)
 			resp.Write([]byte(err.Error()))
 		}
 	}

command/agent/agent_endpoint_test.go

@@ -507,7 +507,7 @@ func TestHTTPAgent_ServiceMaintenanceEndpoint_BadRequest(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 	if resp.Code != 405 {
-		t.Fatalf("expected 405 for non-PUT request")
+		t.Fatalf("expected 405, got %d", resp.Code)
 	}
 
 	// Fails when no enable flag provided
@@ -517,7 +517,7 @@ func TestHTTPAgent_ServiceMaintenanceEndpoint_BadRequest(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 	if resp.Code != 400 {
-		t.Fatalf("expected 400 for missing enable flag")
+		t.Fatalf("expected 400, got %d", resp.Code)
 	}
 
 	// Fails when no service ID provided
@@ -527,7 +527,17 @@ func TestHTTPAgent_ServiceMaintenanceEndpoint_BadRequest(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 	if resp.Code != 400 {
-		t.Fatalf("expected 400 for missing service ID")
+		t.Fatalf("expected 400, got %d", resp.Code)
+	}
+
+	// Fails when bad service ID provided
+	req, _ = http.NewRequest("PUT", "/v1/agent/service/maintenance/_nope_?enable=true", nil)
+	resp = httptest.NewRecorder()
+	if _, err := srv.AgentServiceMaintenance(resp, req); err == nil {
+		t.Fatalf("should have errored")
+	}
+	if resp.Code != 404 {
+		t.Fatalf("expected 404, got %d", resp.Code)
 	}
 }
 
@@ -546,29 +556,9 @@ func TestHTTPAgent_EnableServiceMaintenance(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	// Force into maintenance mode
+	// Force the service into maintenance mode
-	if err := srv.agent.EnableServiceMaintenance("test"); err != nil {
-		t.Fatalf("err: %s", err)
-	}
-
-	// Fails when service is already in maintenance mode
 	req, _ := http.NewRequest("PUT", "/v1/agent/service/maintenance/test?enable=true", nil)
 	resp := httptest.NewRecorder()
-	if _, err := srv.AgentServiceMaintenance(resp, req); err == nil {
-		t.Fatalf("should have errored")
-	}
-	if resp.Code != 409 {
-		t.Fatalf("expected 409, got %d", resp.Code)
-	}
-
-	// Remove maintenance mode
-	if err := srv.agent.DisableServiceMaintenance("test"); err != nil {
-		t.Fatalf("err: %s", err)
-	}
-
-	// Force the service into maintenance mode
-	req, _ = http.NewRequest("PUT", "/v1/agent/service/maintenance/test?enable=true", nil)
-	resp = httptest.NewRecorder()
 	if _, err := srv.AgentServiceMaintenance(resp, req); err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -597,28 +587,23 @@ func TestHTTPAgent_DisableServiceMaintenance(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	// Fails when the service is not in maintenance mode
-	req, _ := http.NewRequest("PUT", "/v1/agent/service/maintenance/test?enable=false", nil)
-	resp := httptest.NewRecorder()
-	if _, err := srv.AgentServiceMaintenance(resp, req); err == nil {
-		t.Fatalf("should have failed")
-	}
-	if resp.Code != 409 {
-		t.Fatalf("expected 409, got %d", resp.Code)
-	}
-
 	// Force the service into maintenance mode
 	if err := srv.agent.EnableServiceMaintenance("test"); err != nil {
 		t.Fatalf("err: %s", err)
 	}
 
 	// Leave maintenance mode
-	req, _ = http.NewRequest("PUT", "/v1/agent/service/maintenance/test?enable=false", nil)
+	req, _ := http.NewRequest("PUT", "/v1/agent/service/maintenance/test?enable=false", nil)
-	resp = httptest.NewRecorder()
+	resp := httptest.NewRecorder()
 	if _, err := srv.AgentServiceMaintenance(resp, req); err != nil {
 		t.Fatalf("err: %s", err)
 	}
 	if resp.Code != 200 {
 		t.Fatalf("expected 200, got %d", resp.Code)
 	}
+
+	// Ensure the maintenance check was removed
+	if _, ok := srv.agent.state.Checks()[maintCheckID]; ok {
+		t.Fatalf("should have removed maintenance check")
+	}
 }

website/source/docs/agent/http.html.markdown

@@ -553,13 +553,12 @@ The return code is 200 on success.
 
 The service maintenance endpoint allows placing a given service into
 "maintenance mode". During maintenance mode, the service will be marked as
-unavailable, and will not be present in DNS or API queries.
+unavailable, and will not be present in DNS or API queries. This API call is
+idempotent. Maintenance mode is persistent and will be automatically restored
+on agent restart.
 
 The `?enable` flag is required, and its value must be `true` (to enter
-maintenance mode), or `false` (to resume normal operation). It is an error to
+maintenance mode), or `false` (to resume normal operation).
-enable maintenance mode while it is already enabled, or disable it while it is
-already disabled. You will receive a 409 if either of these conflicts are
-observed.
 
 The return code is 200 on success.