fix: refactor wait API (runfinch#177)

Signed-off-by: Arjun Raja Yogidas <[email protected]>

Author: coderbirju

api/handlers/container/container.go

@@ -21,7 +21,7 @@ import (
 type Service interface {
 	GetPathToFilesInContainer(ctx context.Context, cid string, path string) (string, func(), error)
 	Remove(ctx context.Context, cid string, force, removeVolumes bool) error
-	Wait(ctx context.Context, cid string, condition string) (code int64, err error)
+	Wait(ctx context.Context, cid string, options ncTypes.ContainerWaitOptions) error
 	Start(ctx context.Context, cid string, options ncTypes.ContainerStartOptions) error
 	Stop(ctx context.Context, cid string, timeout *time.Duration) error
 	Restart(ctx context.Context, cid string, options ncTypes.ContainerRestartOptions) error

api/handlers/container/wait.go

@@ -4,9 +4,13 @@
 package container
 
 import (
+	"fmt"
 	"net/http"
+	"strconv"
+	"strings"
 
 	"github.com/containerd/containerd/v2/pkg/namespaces"
+	ncTypes "github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/gorilla/mux"
 	"github.com/sirupsen/logrus"
 
@@ -19,19 +23,45 @@ type waitError struct {
 }
 
 type waitResp struct {
-	StatusCode int64
+	StatusCode int
 	Error      *waitError `json:",omitempty"`
 }
 
+type codeCapturingWriter struct {
+	code int
+}
+
+func (w *codeCapturingWriter) Write(p []byte) (n int, err error) {
+	w.code, _ = strconv.Atoi(strings.TrimSpace(string(p)))
+	return len(p), nil
+}
+
 func (h *handler) wait(w http.ResponseWriter, r *http.Request) {
 	cid := mux.Vars(r)["id"]
 
 	// TODO: condition is not used because nerdctl doesn't support it
 	condition := r.URL.Query().Get("condition")
+	if condition != "" {
+		logrus.Debugf("Wait container API called with condition options - not supported.")
+		response.SendErrorResponse(w, http.StatusBadRequest, fmt.Errorf("wait condition is not supported"))
+		return
+	}
 	ctx := namespaces.WithNamespace(r.Context(), h.Config.Namespace)
-	code, err := h.service.Wait(ctx, cid, condition)
 
-	if code == -1 {
+	// Create the custom writer
+	codeWriter := &codeCapturingWriter{}
+
+	globalOpt := ncTypes.GlobalCommandOptions(*h.Config)
+	options := ncTypes.ContainerWaitOptions{
+		GOptions: globalOpt,
+		Stdout:   codeWriter,
+	}
+
+	err := h.service.Wait(ctx, cid, options)
+
+	code := codeWriter.code
+
+	if err != nil {
 		var errorCode int
 		switch {
 		case errdefs.IsNotFound(err):
@@ -47,13 +77,10 @@ func (h *handler) wait(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	waitResponse := waitResp{
-		StatusCode: code,
-	}
 	// if there is no err then don't need to set the error msg. e.g. when container is stopped the wait should return
 	// {"Error":null,"StatusCode":0}
-	if err != nil {
-		waitResponse.Error = &waitError{Message: err.Error()}
+	waitResponse := waitResp{
+		StatusCode: code,
 	}
 
 	response.JSON(w, http.StatusOK, waitResponse)

e2e/e2e_test.go

@@ -49,6 +49,7 @@ func TestRun(t *testing.T) {
 		tests.ContainerLogs(opt)
 		tests.ContainerKill(opt)
 		tests.ContainerInspect(opt)
+		tests.ContainerWait(opt)
 
 		// functional test for volume APIs
 		tests.VolumeList(opt)

e2e/tests/container_wait.go

@@ -0,0 +1,94 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package tests
+
+import (
+	"encoding/json"
+	"fmt"
+	"net/http"
+
+	. "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"
+)
+
+// ContainerWait tests the `POST containers/{id}/wait` API.
+func ContainerWait(opt *option.Option) {
+	Describe("wait for a container", func() {
+		var (
+			uClient *http.Client
+			version string
+			apiUrl  string
+		)
+
+		BeforeEach(func() {
+			// 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()
+			relativeUrl := fmt.Sprintf("/containers/%s/wait", testContainerName)
+			apiUrl = client.ConvertToFinchUrl(version, relativeUrl)
+		})
+
+		AfterEach(func() {
+			command.RemoveAll(opt)
+		})
+
+		It("should wait for the container to exit and return status code", func() {
+			// start a container and exit after 3s
+			command.Run(opt, "run", "-d", "--name", testContainerName, defaultImage, "sleep", "3")
+
+			// call wait on this container
+			relativeUrl := fmt.Sprintf("/containers/%s/wait", testContainerName)
+			apiUrl = client.ConvertToFinchUrl(version, relativeUrl)
+
+			res, err := uClient.Post(apiUrl, "application/json", nil)
+			Expect(err).Should(BeNil())
+			Expect(res.StatusCode).Should(Equal(http.StatusOK))
+
+			var waitResponse struct {
+				StatusCode int    `json:"StatusCode"`
+				Error      string `json:"Error"`
+			}
+			err = json.NewDecoder(res.Body).Decode(&waitResponse)
+			Expect(err).Should(BeNil())
+			Expect(waitResponse.StatusCode).Should(Equal(0))
+			Expect(waitResponse.Error).Should(BeEmpty())
+		})
+
+		It("should fail when container does not exist", func() {
+			// don't create the container and call wait api
+			res, err := uClient.Post(apiUrl, "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 reject wait condition parameter", func() {
+			// start a container
+			command.Run(opt, "run", "-d", "--name", testContainerName, defaultImage, "sleep", "5")
+
+			// try to wait with a condition
+			relativeUrl := fmt.Sprintf("/containers/%s/wait?condition=not-running", testContainerName)
+			apiUrl = client.ConvertToFinchUrl(version, relativeUrl)
+
+			res, err := uClient.Post(apiUrl, "application/json", nil)
+			Expect(err).Should(BeNil())
+			Expect(res.StatusCode).Should(Equal(http.StatusBadRequest))
+
+			var errResponse response.Error
+			err = json.NewDecoder(res.Body).Decode(&errResponse)
+			Expect(err).Should(BeNil())
+			Expect(errResponse.Message).Should(ContainSubstring("condition"))
+		})
+	})
+}

internal/backend/container.go

@@ -36,6 +36,7 @@ type NerdctlContainerSvc interface {
 	ListContainers(ctx context.Context, options types.ContainerListOptions) ([]container.ListItem, error)
 	RenameContainer(ctx context.Context, container containerd.Container, newName string, options types.ContainerRenameOptions) error
 	KillContainer(ctx context.Context, cid string, options types.ContainerKillOptions) error
+	ContainerWait(ctx context.Context, cid string, options types.ContainerWaitOptions) error
 
 	// Mocked functions for container attach
 	GetDataStore() (string, error)
@@ -125,6 +126,10 @@ func (w *NerdctlWrapper) KillContainer(ctx context.Context, cid string, options
 	return container.Kill(ctx, w.clientWrapper.client, []string{cid}, options)
 }
 
+func (w *NerdctlWrapper) ContainerWait(ctx context.Context, cid string, options types.ContainerWaitOptions) error {
+	return container.Wait(ctx, w.clientWrapper.client, []string{cid}, options)
+}
+
 func (w *NerdctlWrapper) GetNerdctlExe() (string, error) {
 	if w.nerdctlExe != "" {
 		return w.nerdctlExe, nil

internal/service/container/wait.go

@@ -6,34 +6,20 @@ package container
 import (
 	"context"
 
-	containerd "github.com/containerd/containerd/v2/client"
+	cerrdefs "github.com/containerd/errdefs"
+	ncTypes "github.com/containerd/nerdctl/v2/pkg/api/types"
+	"github.com/runfinch/finch-daemon/pkg/errdefs"
 )
 
-func (s *service) Wait(ctx context.Context, cid string, condition string) (code int64, err error) {
-	con, err := s.getContainer(ctx, cid)
-	// container wait status code is uint32, use -1 to indicate container search error
+func (s *service) Wait(ctx context.Context, cid string, options ncTypes.ContainerWaitOptions) error {
+	cont, err := s.getContainer(ctx, cid)
 	if err != nil {
-		return -1, err
+		if cerrdefs.IsNotFound(err) {
+			return errdefs.NewNotFound(err)
+		}
+		return err
 	}
-	s.logger.Debugf("wait container: %s", con.ID())
-	rawcode, err := waitContainer(ctx, con)
-	return int64(rawcode), err
-}
-
-// TODO: contribute to nerdctl to make this function public.
-func waitContainer(ctx context.Context, container containerd.Container) (code uint32, err error) {
-	task, err := container.Task(ctx, nil)
-	if err != nil {
-		return 0, err
-	}
-
-	statusC, err := task.Wait(ctx)
-	if err != nil {
-		return 0, err
-	}
-
-	status := <-statusC
-	code, _, err = status.Result()
 
-	return code, err
+	s.logger.Debugf("wait container: %s", cont.ID())
+	return s.nctlContainerSvc.ContainerWait(ctx, cont.ID(), options)
 }

internal/service/container/wait_test.go

@@ -0,0 +1,92 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package container
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	"github.com/golang/mock/gomock"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	containerd "github.com/containerd/containerd/v2/client"
+	cerrdefs "github.com/containerd/errdefs"
+	ncTypes "github.com/containerd/nerdctl/v2/pkg/api/types"
+	"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"
+)
+
+var _ = Describe("Container Wait API", func() {
+	var (
+		ctx            context.Context
+		mockCtrl       *gomock.Controller
+		logger         *mocks_logger.Logger
+		cdClient       *mocks_backend.MockContainerdClient
+		ncContainerSvc *mocks_backend.MockNerdctlContainerSvc
+		svc            *service
+		cid            string
+		waitOptions    ncTypes.ContainerWaitOptions
+		con            *mocks_container.MockContainer
+	)
+
+	BeforeEach(func() {
+		ctx = context.Background()
+		mockCtrl = gomock.NewController(GinkgoT())
+		logger = mocks_logger.NewLogger(mockCtrl)
+		cdClient = mocks_backend.NewMockContainerdClient(mockCtrl)
+		ncContainerSvc = mocks_backend.NewMockNerdctlContainerSvc(mockCtrl)
+
+		cid = "test-container-id"
+		waitOptions = ncTypes.ContainerWaitOptions{}
+		con = mocks_container.NewMockContainer(mockCtrl)
+		con.EXPECT().ID().Return(cid).AnyTimes()
+
+		svc = &service{
+			client:           cdClient,
+			nctlContainerSvc: mockNerdctlService{ncContainerSvc, nil},
+			logger:           logger,
+		}
+	})
+
+	AfterEach(func() {
+		mockCtrl.Finish()
+	})
+
+	Context("Wait API", func() {
+		It("should successfully wait for a container", func() {
+			cdClient.EXPECT().SearchContainer(gomock.Any(), cid).Return(
+				[]containerd.Container{con}, nil)
+			logger.EXPECT().Debugf("wait container: %s", cid)
+			ncContainerSvc.EXPECT().ContainerWait(ctx, cid, waitOptions).Return(nil)
+
+			err := svc.Wait(ctx, cid, waitOptions)
+			Expect(err).Should(BeNil())
+		})
+
+		It("should return NotFound error if container is not found", func() {
+			mockErr := cerrdefs.ErrNotFound.WithMessage(fmt.Sprintf("no such container: %s", cid))
+			cdClient.EXPECT().SearchContainer(gomock.Any(), cid).Return(nil, mockErr)
+			logger.EXPECT().Errorf(gomock.Any(), gomock.Any(), gomock.Any())
+
+			err := svc.Wait(ctx, cid, waitOptions)
+			Expect(err.Error()).Should(Equal(errdefs.NewNotFound(fmt.Errorf("no such container: %s", cid)).Error()))
+		})
+
+		It("should return an error if ContainerWait fails", func() {
+			cdClient.EXPECT().SearchContainer(gomock.Any(), cid).Return(
+				[]containerd.Container{con}, nil)
+			logger.EXPECT().Debugf("wait container: %s", cid)
+			mockErr := errors.New("error waiting for container")
+			ncContainerSvc.EXPECT().ContainerWait(ctx, cid, waitOptions).Return(mockErr)
+
+			err := svc.Wait(ctx, cid, waitOptions)
+			Expect(err).Should(Equal(mockErr))
+		})
+
+	})
+})