From 633f820430fc866897c2e49e93b2bb6b6792d719 Mon Sep 17 00:00:00 2001 From: Cezar Rata Date: Thu, 5 Sep 2024 20:24:35 +0000 Subject: [PATCH] Ported changes from internal 'implement container restart API' commit Signed-off-by: Cezar Rata --- api/handlers/container/container.go | 2 + api/handlers/container/container_test.go | 9 ++ api/handlers/container/restart.go | 42 ++++++++ api/handlers/container/restart_test.go | 76 +++++++++++++ e2e/e2e_test.go | 1 + e2e/tests/container_restart.go | 106 ++++++++++++++++++ internal/service/container/restart.go | 32 ++++++ internal/service/container/restart_test.go | 119 +++++++++++++++++++++ mocks/mocks_container/containersvc.go | 14 +++ 9 files changed, 401 insertions(+) create mode 100644 api/handlers/container/restart.go create mode 100644 api/handlers/container/restart_test.go create mode 100644 e2e/tests/container_restart.go create mode 100644 internal/service/container/restart.go create mode 100644 internal/service/container/restart_test.go diff --git a/api/handlers/container/container.go b/api/handlers/container/container.go index de05d75..40a7126 100644 --- a/api/handlers/container/container.go +++ b/api/handlers/container/container.go @@ -24,6 +24,7 @@ type Service interface { Wait(ctx context.Context, cid string, condition string) (code int64, err error) Start(ctx context.Context, cid string) error Stop(ctx context.Context, cid string, timeout *time.Duration) error + Restart(ctx context.Context, cid string, timeout time.Duration) error Create(ctx context.Context, image string, cmd []string, createOpt ncTypes.ContainerCreateOptions, netOpt ncTypes.NetworkOptions) (string, error) Inspect(ctx context.Context, cid string) (*types.Container, error) WriteFilesAsTarArchive(filePath string, writer io.Writer, slashDot bool) error @@ -44,6 +45,7 @@ func RegisterHandlers(r types.VersionedRouter, service Service, conf *config.Con r.HandleFunc("/{id:.*}", h.remove, http.MethodDelete) r.HandleFunc("/{id:.*}/start", h.start, http.MethodPost) r.HandleFunc("/{id:.*}/stop", h.stop, http.MethodPost) + r.HandleFunc("/{id:.*}/restart", h.restart, http.MethodPost) r.HandleFunc("/{id:.*}/remove", h.remove, http.MethodPost) r.HandleFunc("/{id:.*}/wait", h.wait, http.MethodPost) r.HandleFunc("/create", h.create, http.MethodPost) diff --git a/api/handlers/container/container_test.go b/api/handlers/container/container_test.go index b9538cd..cb952e8 100644 --- a/api/handlers/container/container_test.go +++ b/api/handlers/container/container_test.go @@ -76,6 +76,15 @@ var _ = Describe("Container API", func() { Expect(rr).Should(HaveHTTPStatus(http.StatusInternalServerError)) Expect(rr.Body).Should(MatchJSON(`{"message": "error from stop api"}`)) }) + It("should call container restart method", func() { + // setup mocks + service.EXPECT().Restart(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("error from restart api")) + req, _ = http.NewRequest(http.MethodPost, "/containers/123/restart", nil) + // call the API to check if it returns the error generated from restart method + router.ServeHTTP(rr, req) + Expect(rr).Should(HaveHTTPStatus(http.StatusInternalServerError)) + Expect(rr.Body).Should(MatchJSON(`{"message": "error from restart api"}`)) + }) It("should call container create method", func() { // setup mocks body := []byte(`{"Image": "test-image"}`) diff --git a/api/handlers/container/restart.go b/api/handlers/container/restart.go new file mode 100644 index 0000000..1d16b2e --- /dev/null +++ b/api/handlers/container/restart.go @@ -0,0 +1,42 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package container + +import ( + "net/http" + "strconv" + "time" + + "github.com/containerd/containerd/namespaces" + "github.com/gorilla/mux" + "github.com/runfinch/finch-daemon/api/response" + "github.com/runfinch/finch-daemon/pkg/errdefs" +) + +func (h *handler) restart(w http.ResponseWriter, r *http.Request) { + cid := mux.Vars(r)["id"] + t, err := strconv.ParseInt(r.URL.Query().Get("t"), 10, 64) + if err != nil { + t = 10 // Docker/nerdctl default + } + timeout := time.Second * time.Duration(t) + + ctx := namespaces.WithNamespace(r.Context(), h.Config.Namespace) + err = h.service.Restart(ctx, cid, timeout) + // map the error into http status code and send response. + if err != nil { + var code int + switch { + case errdefs.IsNotFound(err): + code = http.StatusNotFound + default: + code = http.StatusInternalServerError + } + h.logger.Debugf("Restart container API responding with error code. Status code %d, Message: %s", code, err.Error()) + response.SendErrorResponse(w, code, err) + return + } + // successfully restarted the container. Send no content status. + response.Status(w, http.StatusNoContent) +} diff --git a/api/handlers/container/restart_test.go b/api/handlers/container/restart_test.go new file mode 100644 index 0000000..5ef0010 --- /dev/null +++ b/api/handlers/container/restart_test.go @@ -0,0 +1,76 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package container + +import ( + "fmt" + "net/http" + "net/http/httptest" + + "github.com/containerd/nerdctl/pkg/config" + "github.com/golang/mock/gomock" + "github.com/gorilla/mux" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/runfinch/finch-daemon/mocks/mocks_container" + "github.com/runfinch/finch-daemon/mocks/mocks_logger" + "github.com/runfinch/finch-daemon/pkg/errdefs" +) + +var _ = Describe("Container Restart API ", func() { + var ( + mockCtrl *gomock.Controller + logger *mocks_logger.Logger + service *mocks_container.MockService + h *handler + rr *httptest.ResponseRecorder + req *http.Request + ) + BeforeEach(func() { + mockCtrl = gomock.NewController(GinkgoT()) + defer mockCtrl.Finish() + logger = mocks_logger.NewLogger(mockCtrl) + service = mocks_container.NewMockService(mockCtrl) + c := config.Config{} + h = newHandler(service, &c, logger) + rr = httptest.NewRecorder() + req, _ = http.NewRequest(http.MethodPost, "/containers/123/restart", nil) + req = mux.SetURLVars(req, map[string]string{"id": "123"}) + }) + Context("handler", func() { + It("should return 204 as success response", func() { + // service mock returns nil to mimic handler started the container successfully. + service.EXPECT().Restart(gomock.Any(), "123", gomock.Any()).Return(nil) + + //handler should return success message with 204 status code. + h.restart(rr, req) + Expect(rr).Should(HaveHTTPStatus(http.StatusNoContent)) + }) + + It("should return 404 not found response", func() { + // service mock returns not found error to mimic user trying to start container that does not exist + service.EXPECT().Restart(gomock.Any(), "123", gomock.Any()).Return( + errdefs.NewNotFound(fmt.Errorf("container not found"))) + logger.EXPECT().Debugf("Restart container API responding with error code. Status code %d, Message: %s", 404, "container not found") + + //handler should return 404 status code with an error msg. + h.restart(rr, req) + Expect(rr).Should(HaveHTTPStatus(http.StatusNotFound)) + Expect(rr.Body).Should(MatchJSON(`{"message": "container not found"}`)) + }) + It("should return 500 internal error response", func() { + // service mock return error to mimic a user trying to start a container with an id that has + // multiple containers with same prefix. + service.EXPECT().Restart(gomock.Any(), "123", gomock.Any()).Return( + fmt.Errorf("multiple IDs found with provided prefix")) + logger.EXPECT().Debugf("Restart container API responding with error code. Status code %d, Message: %s", 500, "multiple IDs found with provided prefix") + + //handler should return 500 status code with an error msg. + h.restart(rr, req) + Expect(rr).Should(HaveHTTPStatus(http.StatusInternalServerError)) + Expect(rr.Body).Should(MatchJSON(`{"message": "multiple IDs found with provided prefix"}`)) + }) + }) +}) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index caf43ec..0abd604 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -40,6 +40,7 @@ func TestRun(t *testing.T) { tests.ContainerCreate(opt) tests.ContainerStart(opt) tests.ContainerStop(opt) + tests.ContainerRestart(opt) tests.ContainerRemove(opt) tests.ContainerList(opt) tests.ContainerRename(opt) diff --git a/e2e/tests/container_restart.go b/e2e/tests/container_restart.go new file mode 100644 index 0000000..05dc393 --- /dev/null +++ b/e2e/tests/container_restart.go @@ -0,0 +1,106 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package tests + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/runfinch/common-tests/command" + "github.com/runfinch/common-tests/option" + "github.com/runfinch/finch-daemon/api/response" + "github.com/runfinch/finch-daemon/e2e/client" +) + +// ContainerRestart tests the `POST containers/{id}/restart` API. +func ContainerRestart(opt *option.Option) { + Describe("restart a container", func() { + var ( + uClient *http.Client + version string + ) + BeforeEach(func() { + command.Run(opt, "run", "-d", "--name", testContainerName, defaultImage, + "/bin/sh", "-c", `date; sleep infinity`) + // create a custom client to use http over unix sockets + uClient = client.NewClient(GetDockerHostUrl()) + // get the docker api version that will be tested + version = GetDockerApiVersion() + }) + AfterEach(func() { + command.RemoveAll(opt) + }) + + It("should start and restart the container", func() { + containerShouldBeRunning(opt, testContainerName) + + before := time.Now().Round(0) + + restartRelativeUrl := fmt.Sprintf("/containers/%s/restart", testContainerName) + res, err := uClient.Post(client.ConvertToFinchUrl(version, restartRelativeUrl), "application/json", nil) + Expect(err).Should(BeNil()) + Expect(res.StatusCode).Should(Equal(http.StatusNoContent)) + + logsRelativeUrl := fmt.Sprintf("/containers/%s/logs", testContainerName) + opts := "?stdout=1" + + "&stderr=0" + + "&follow=0" + + "&tail=0" + res, err = uClient.Get(client.ConvertToFinchUrl(version, logsRelativeUrl+opts)) + Expect(err).Should(BeNil()) + body, err := io.ReadAll(res.Body) + Expect(err).Should(BeNil()) + + dateStr := string(body[8 : len(body)-1]) + date, _ := time.Parse(time.UnixDate, dateStr) + Expect(before.Before(date)).Should(BeTrue()) + }) + It("should fail to restart container that does not exist", func() { + // restart a container that does not exist + relativeUrl := client.ConvertToFinchUrl(version, "/containers/container-does-not-exist/restart") + res, err := uClient.Post(relativeUrl, "application/json", nil) + Expect(err).Should(BeNil()) + Expect(res.StatusCode).Should(Equal(http.StatusNotFound)) + var errResponse response.Error + err = json.NewDecoder(res.Body).Decode(&errResponse) + Expect(err).Should(BeNil()) + Expect(errResponse.Message).Should(Not(BeEmpty())) + }) + It("should restart a stopped container", func() { + containerShouldBeRunning(opt, testContainerName) + + stopRelativeUrl := fmt.Sprintf("/containers/%s/stop", testContainerName) + res, err := uClient.Post(client.ConvertToFinchUrl(version, stopRelativeUrl), "application/json", nil) + Expect(err).Should(BeNil()) + Expect(res.StatusCode).Should(Equal(http.StatusNoContent)) + containerShouldNotBeRunning(opt, testContainerName) + + restartRelativeUrl := fmt.Sprintf("/containers/%s/restart", testContainerName) + res, err = uClient.Post(client.ConvertToFinchUrl(version, restartRelativeUrl), "application/json", nil) + Expect(err).Should(BeNil()) + Expect(res.StatusCode).Should(Equal(http.StatusNoContent)) + containerShouldBeRunning(opt, testContainerName) + }) + It("should restart the container with timeout", func() { + containerShouldBeRunning(opt, testContainerName) + + // stop the container with a timeout of 5 seconds + now := time.Now() + restartRelativeUrl := fmt.Sprintf("/containers/%s/restart?t=5", testContainerName) + res, err := uClient.Post(client.ConvertToFinchUrl(version, restartRelativeUrl), "application/json", nil) + later := time.Now() + Expect(err).Should(BeNil()) + Expect(res.StatusCode).Should(Equal(http.StatusNoContent)) + elapsed := later.Sub(now) + Expect(elapsed.Seconds()).Should(BeNumerically(">", 4.0)) + Expect(elapsed.Seconds()).Should(BeNumerically("<", 10.0)) + containerShouldBeRunning(opt, testContainerName) + }) + }) +} diff --git a/internal/service/container/restart.go b/internal/service/container/restart.go new file mode 100644 index 0000000..ac9ff42 --- /dev/null +++ b/internal/service/container/restart.go @@ -0,0 +1,32 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package container + +import ( + "context" + "time" + + "github.com/runfinch/finch-daemon/pkg/errdefs" +) + +func (s *service) Restart(ctx context.Context, cid string, timeout time.Duration) error { + con, err := s.getContainer(ctx, cid) + if err != nil { + return err + } + + // restart the container and if error occurs then return error otherwise return nil + // swallow IsNotModified error on StopContainer for already stopped container, simply call StartContainer + s.logger.Debugf("restarting container: %s", cid) + if err := s.nctlContainerSvc.StopContainer(ctx, con, &timeout); err != nil && !errdefs.IsNotModified(err) { + s.logger.Errorf("Failed to stop container: %s. Error: %v", cid, err) + return err + } + if err = s.nctlContainerSvc.StartContainer(ctx, con); err != nil { + s.logger.Errorf("Failed to start container: %s. Error: %v", cid, err) + return err + } + s.logger.Debugf("successfully restarted: %s", cid) + return nil +} diff --git a/internal/service/container/restart_test.go b/internal/service/container/restart_test.go new file mode 100644 index 0000000..103f6f7 --- /dev/null +++ b/internal/service/container/restart_test.go @@ -0,0 +1,119 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package container + +import ( + "context" + "fmt" + "time" + + "github.com/containerd/containerd" + "github.com/golang/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/runfinch/finch-daemon/api/handlers/container" + "github.com/runfinch/finch-daemon/mocks/mocks_archive" + "github.com/runfinch/finch-daemon/mocks/mocks_backend" + "github.com/runfinch/finch-daemon/mocks/mocks_container" + "github.com/runfinch/finch-daemon/mocks/mocks_logger" + "github.com/runfinch/finch-daemon/pkg/errdefs" +) + +// Unit tests related to container restart API +var _ = Describe("Container Restart API ", func() { + var ( + ctx context.Context + mockCtrl *gomock.Controller + logger *mocks_logger.Logger + cdClient *mocks_backend.MockContainerdClient + ncClient *mocks_backend.MockNerdctlContainerSvc + con *mocks_container.MockContainer + cid string + tarExtractor *mocks_archive.MockTarExtractor + service container.Service + timeout time.Duration + ) + BeforeEach(func() { + ctx = context.Background() + // initialize the mocks + mockCtrl = gomock.NewController(GinkgoT()) + logger = mocks_logger.NewLogger(mockCtrl) + cdClient = mocks_backend.NewMockContainerdClient(mockCtrl) + ncClient = mocks_backend.NewMockNerdctlContainerSvc(mockCtrl) + con = mocks_container.NewMockContainer(mockCtrl) + con.EXPECT().ID().Return(cid).AnyTimes() + tarExtractor = mocks_archive.NewMockTarExtractor(mockCtrl) + service = NewService(cdClient, mockNerdctlService{ncClient, nil}, logger, nil, nil, tarExtractor) + timeout = time.Duration(10) + }) + Context("service", func() { + It("should not return any error when Running", func() { + // set up the mock to return a container that is in running state + cdClient.EXPECT().SearchContainer(gomock.Any(), cid).Return( + []containerd.Container{con}, nil).AnyTimes() + //mock the nerdctl client to mock the restart container was successful without any error. + ncClient.EXPECT().StartContainer(ctx, con).Return(nil) + ncClient.EXPECT().StopContainer(ctx, con, &timeout).Return(nil) + gomock.InOrder( + logger.EXPECT().Debugf("restarting container: %s", cid), + logger.EXPECT().Debugf("successfully restarted: %s", cid), + ) + //service should not return any error + err := service.Restart(ctx, cid, timeout) + Expect(err).Should(BeNil()) + }) + It("should not return any error when Stopped", func() { + // set up the mock to return a container that is in running state + cdClient.EXPECT().SearchContainer(gomock.Any(), cid).Return( + []containerd.Container{con}, nil).AnyTimes() + //mock the nerdctl client to mock the restart container was successful without any error. + ncClient.EXPECT().StopContainer(ctx, con, &timeout).Return(errdefs.NewNotModified(fmt.Errorf("err"))) + ncClient.EXPECT().StartContainer(ctx, con).Return(nil) + gomock.InOrder( + logger.EXPECT().Debugf("restarting container: %s", cid), + logger.EXPECT().Debugf("successfully restarted: %s", cid), + ) + //service should not return any error + err := service.Restart(ctx, cid, timeout) + Expect(err).Should(BeNil()) + }) + It("should return not found error", func() { + // set up the mock to mimic no container found for the provided container id + cdClient.EXPECT().SearchContainer(gomock.Any(), gomock.Any()).Return( + []containerd.Container{}, nil) + logger.EXPECT().Debugf("no such container: %s", gomock.Any()) + + // service should return NotFound error + err := service.Restart(ctx, cid, timeout) + Expect(errdefs.IsNotFound(err)).Should(BeTrue()) + }) + It("should return multiple containers found error", func() { + // set up the mock to mimic two containers found for the provided container id + cdClient.EXPECT().SearchContainer(gomock.Any(), gomock.Any()).Return( + []containerd.Container{con, con}, nil) + logger.EXPECT().Debugf("multiple IDs found with provided prefix: %s, total containers found: %d", cid, 2) + + // service should return error + err := service.Restart(ctx, cid, timeout) + Expect(err).Should(Not(BeNil())) + }) + It("should fail due to nerdctl client error", func() { + // set up the mock to mimic an error occurred while starting the container using nerdctl function. + cdClient.EXPECT().SearchContainer(gomock.Any(), gomock.Any()).Return( + []containerd.Container{con}, nil).AnyTimes() + + expectedErr := fmt.Errorf("nerdctl error") + ncClient.EXPECT().StopContainer(ctx, con, &timeout).Return(nil) + ncClient.EXPECT().StartContainer(ctx, con).Return(expectedErr) + gomock.InOrder( + logger.EXPECT().Debugf("restarting container: %s", cid), + ) + logger.EXPECT().Errorf("Failed to start container: %s. Error: %v", cid, expectedErr) + + // service should return not modified error. + err := service.Restart(ctx, cid, timeout) + Expect(err).Should(Equal(expectedErr)) + }) + }) +}) diff --git a/mocks/mocks_container/containersvc.go b/mocks/mocks_container/containersvc.go index 9224b4e..be39abd 100644 --- a/mocks/mocks_container/containersvc.go +++ b/mocks/mocks_container/containersvc.go @@ -184,6 +184,20 @@ func (mr *MockServiceMockRecorder) Rename(arg0, arg1, arg2, arg3 interface{}) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Rename", reflect.TypeOf((*MockService)(nil).Rename), arg0, arg1, arg2, arg3) } +// Restart mocks base method. +func (m *MockService) Restart(arg0 context.Context, arg1 string, arg2 time.Duration) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Restart", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// Restart indicates an expected call of Restart. +func (mr *MockServiceMockRecorder) Restart(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Restart", reflect.TypeOf((*MockService)(nil).Restart), arg0, arg1, arg2) +} + // Start mocks base method. func (m *MockService) Start(arg0 context.Context, arg1 string) error { m.ctrl.T.Helper()