From bfc7be9690ff1bdbd00ee3f8352ad06d7203eba9 Mon Sep 17 00:00:00 2001 From: qibobo Date: Sun, 22 Dec 2019 20:55:03 -0600 Subject: [PATCH] bug fix: Schedules can not be removed in synchronization when an application is deleted directly #553 (#554) * bug fix: Schedules can not be removed in synchronization when an application is deleted directly #553 * modify log --- .../scheduler/service/ScheduleManager.java | 14 ++++------- .../service/ScheduleManagerTest.java | 7 +----- .../cmd/scalingengine/scalingengine_test.go | 2 +- src/autoscaler/scalingengine/scalingengine.go | 12 +++++++--- .../scalingengine/scalingengine_test.go | 8 +++---- .../scalingengine/server/scaling_handler.go | 17 ++++---------- .../server/scaling_handler_test.go | 23 ++----------------- .../scalingengine/server/server_test.go | 4 ++-- 8 files changed, 28 insertions(+), 59 deletions(-) diff --git a/scheduler/src/main/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManager.java b/scheduler/src/main/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManager.java index a222585d2..e41235c3b 100644 --- a/scheduler/src/main/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManager.java +++ b/scheduler/src/main/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManager.java @@ -479,15 +479,11 @@ private void notifyScalingEngineForDelete(String appId, long scheduleId) { try { restOperations.delete(scalingEnginePathActiveSchedule); } catch (HttpStatusCodeException hce) { - if (hce.getStatusCode() == HttpStatus.NOT_FOUND) { - message = messageBundleResourceHelper - .lookupMessage("scalingengine.notification.activeschedule.notFound", appId, scheduleId); - logger.info(message, hce); - } else { - String errorMessage = messageBundleResourceHelper.lookupMessage("scalingengine.notification.error", - hce.getResponseBodyAsString(), appId, scheduleId, "delete"); - throw new SchedulerInternalException(errorMessage, hce); - } + + String errorMessage = messageBundleResourceHelper.lookupMessage("scalingengine.notification.error", + hce.getResponseBodyAsString(), appId, scheduleId, "delete"); + throw new SchedulerInternalException(errorMessage, hce); + } catch (ResourceAccessException rae) { String errorMessage = messageBundleResourceHelper.lookupMessage("scalingengine.notification.error", rae.getMessage(), appId, scheduleId, "delete"); diff --git a/scheduler/src/test/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManagerTest.java b/scheduler/src/test/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManagerTest.java index 6723ec014..382dfc700 100644 --- a/scheduler/src/test/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManagerTest.java +++ b/scheduler/src/test/java/org/cloudfoundry/autoscaler/scheduler/service/ScheduleManagerTest.java @@ -535,7 +535,7 @@ public void testDeleteSchedules_when_activeSchedule_not_found_in_scalingEngine() String scalingEnginePathActiveSchedule = scalingEngineUrl + "/v1/apps/" + appId + "/active_schedules/" + scheduleId; mockServer.expect(ExpectedCount.times(1), requestTo(scalingEnginePathActiveSchedule)) - .andExpect(method(HttpMethod.DELETE)).andRespond(withStatus(HttpStatus.NOT_FOUND)); + .andExpect(method(HttpMethod.DELETE)).andRespond(withStatus(HttpStatus.OK)); scheduleManager.deleteSchedules(appId); @@ -554,11 +554,6 @@ public void testDeleteSchedules_when_activeSchedule_not_found_in_scalingEngine() Mockito.verify(activeScheduleDao, Mockito.times(1)).deleteActiveSchedulesByAppId(appId); mockServer.verify(); - - String expectedMessage = messageBundleResourceHelper - .lookupMessage("scalingengine.notification.activeschedule.notFound", appId, scheduleId); - assertThat(logCaptor.getValue().getMessage().getFormattedMessage(), Is.is(expectedMessage)); - assertThat("Log level should be INFO", logCaptor.getValue().getLevel(), Is.is(Level.INFO)); } @Test diff --git a/src/autoscaler/scalingengine/cmd/scalingengine/scalingengine_test.go b/src/autoscaler/scalingengine/cmd/scalingengine/scalingengine_test.go index 9ebf38a01..b228d5037 100644 --- a/src/autoscaler/scalingengine/cmd/scalingengine/scalingengine_test.go +++ b/src/autoscaler/scalingengine/cmd/scalingengine/scalingengine_test.go @@ -190,7 +190,7 @@ var _ = Describe("Main", func() { rsp, err = httpClient.Do(req) Expect(err).NotTo(HaveOccurred()) - Expect(rsp.StatusCode).To(Equal(http.StatusNoContent)) + Expect(rsp.StatusCode).To(Equal(http.StatusOK)) rsp.Body.Close() }) }) diff --git a/src/autoscaler/scalingengine/scalingengine.go b/src/autoscaler/scalingengine/scalingengine.go index 4979081e9..9a211fda3 100644 --- a/src/autoscaler/scalingengine/scalingengine.go +++ b/src/autoscaler/scalingengine/scalingengine.go @@ -33,6 +33,8 @@ type scalingEngine struct { type ActiveScheduleNotFoundError struct { } +type AppNotFoundError struct { +} func (ase *ActiveScheduleNotFoundError) Error() string { return fmt.Sprintf("active schedule not found") @@ -306,9 +308,8 @@ func (s *scalingEngine) RemoveActiveSchedule(appId string, scheduleId string) er } if (currentSchedule == nil) || (currentSchedule.ScheduleId != scheduleId) { - err = &ActiveScheduleNotFoundError{} - logger.Error("failed-to-remove-active-schedule", err) - return err + logger.Info("active-schedule-not-found", lager.Data{"appId": appId, "scheduleId": scheduleId}) + return nil } err = s.scalingEngineDB.RemoveActiveSchedule(appId) @@ -330,6 +331,11 @@ func (s *scalingEngine) RemoveActiveSchedule(appId string, scheduleId string) er appEntity, err := s.cfClient.GetApp(appId) if err != nil { + if _, ok := err.(*models.AppNotFoundErr); ok { + logger.Info("app-not-found", lager.Data{"appId": appId}) + history.Status = models.ScalingStatusIgnored + return nil + } logger.Error("failed-to-get-app-info", err) history.Status = models.ScalingStatusFailed history.Error = "failed to get app info: " + err.Error() diff --git a/src/autoscaler/scalingengine/scalingengine_test.go b/src/autoscaler/scalingengine/scalingengine_test.go index 32f518651..02fa166b2 100644 --- a/src/autoscaler/scalingengine/scalingengine_test.go +++ b/src/autoscaler/scalingengine/scalingengine_test.go @@ -967,8 +967,8 @@ var _ = Describe("ScalingEngine", func() { BeforeEach(func() { scalingEngineDB.GetActiveScheduleReturns(nil, nil) }) - It("should error", func() { - Expect(err).To(BeAssignableToTypeOf(&ActiveScheduleNotFoundError{})) + It("should not have any error", func() { + Expect(err).NotTo(HaveOccurred()) Expect(scalingEngineDB.RemoveActiveScheduleCallCount()).To(BeZero()) }) }) @@ -977,8 +977,8 @@ var _ = Describe("ScalingEngine", func() { BeforeEach(func() { scalingEngineDB.GetActiveScheduleReturns(&models.ActiveSchedule{ScheduleId: "a-different-schedule-id"}, nil) }) - It("should error", func() { - Expect(err).To(BeAssignableToTypeOf(&ActiveScheduleNotFoundError{})) + It("should not have any error", func() { + Expect(err).NotTo(HaveOccurred()) Expect(scalingEngineDB.RemoveActiveScheduleCallCount()).To(BeZero()) }) }) diff --git a/src/autoscaler/scalingengine/server/scaling_handler.go b/src/autoscaler/scalingengine/server/scaling_handler.go index e8a03274f..6cba52007 100644 --- a/src/autoscaler/scalingengine/server/scaling_handler.go +++ b/src/autoscaler/scalingengine/server/scaling_handler.go @@ -220,22 +220,13 @@ func (h *ScalingHandler) RemoveActiveSchedule(w http.ResponseWriter, r *http.Req if err != nil { logger.Error("failed-to-remove-active-schedule", err) - switch err.(type) { - case *scalingengine.ActiveScheduleNotFoundError: - handlers.WriteJSONResponse(w, http.StatusNotFound, models.ErrorResponse{ - Code: "Not-Found", - Message: "Active schedule not found", - }) - default: - handlers.WriteJSONResponse(w, http.StatusInternalServerError, models.ErrorResponse{ - Code: "Interal-Server-Error", - Message: "Error removing active schedule"}) - - } + handlers.WriteJSONResponse(w, http.StatusInternalServerError, models.ErrorResponse{ + Code: "Interal-Server-Error", + Message: "Error removing active schedule"}) return } - w.WriteHeader(http.StatusNoContent) + w.WriteHeader(http.StatusOK) } func (h *ScalingHandler) GetActiveSchedule(w http.ResponseWriter, r *http.Request, vars map[string]string) { diff --git a/src/autoscaler/scalingengine/server/scaling_handler_test.go b/src/autoscaler/scalingengine/server/scaling_handler_test.go index 2ed9822be..4dd516343 100644 --- a/src/autoscaler/scalingengine/server/scaling_handler_test.go +++ b/src/autoscaler/scalingengine/server/scaling_handler_test.go @@ -4,7 +4,6 @@ import ( "autoscaler/db" "autoscaler/fakes" "autoscaler/models" - "autoscaler/scalingengine" . "autoscaler/scalingengine/server" "code.cloudfoundry.org/lager/lagertest" @@ -514,26 +513,8 @@ var _ = Describe("ScalingHandler", func() { }) Context("when removing active schedule succeeds", func() { - It("returns 204", func() { - Expect(resp.Code).To(Equal(http.StatusNoContent)) - }) - }) - - Context("when active schedule is not found", func() { - BeforeEach(func() { - scalingEngine.RemoveActiveScheduleReturns(&scalingengine.ActiveScheduleNotFoundError{}) - }) - - It("returns 404", func() { - Expect(resp.Code).To(Equal(http.StatusNotFound)) - - errJson := &models.ErrorResponse{} - err = json.Unmarshal(resp.Body.Bytes(), errJson) - Expect(err).ToNot(HaveOccurred()) - Expect(errJson).To(Equal(&models.ErrorResponse{ - Code: "Not-Found", - Message: "Active schedule not found", - })) + It("returns 200", func() { + Expect(resp.Code).To(Equal(http.StatusOK)) }) }) diff --git a/src/autoscaler/scalingengine/server/server_test.go b/src/autoscaler/scalingengine/server/server_test.go index a513a3edf..1ae67aa32 100644 --- a/src/autoscaler/scalingengine/server/server_test.go +++ b/src/autoscaler/scalingengine/server/server_test.go @@ -190,9 +190,9 @@ var _ = Describe("Server", func() { method = http.MethodDelete }) Context("when requesting correctly", func() { - It("should return 204", func() { + It("should return 200", func() { Expect(err).ToNot(HaveOccurred()) - Expect(rsp.StatusCode).To(Equal(http.StatusNoContent)) + Expect(rsp.StatusCode).To(Equal(http.StatusOK)) rsp.Body.Close() }) })