From 60d133f98161034dee5a161527168ced7b6fff9c Mon Sep 17 00:00:00 2001
From: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Date: Thu, 3 Feb 2022 12:02:05 -0500
Subject: [PATCH] PR: error messages, naming, tests

Checking return value of `shutdownInternal` for cleanup in service
tests.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
---
 cmd/containerd-shim-runhcs-v1/serve.go         |  4 ++--
 cmd/containerd-shim-runhcs-v1/service.go       | 18 +++++++++---------
 .../service_internal.go                        |  6 +++---
 .../service_internal_podshim_test.go           |  6 ++++--
 .../service_internal_taskshim_test.go          |  6 ++++--
 5 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/cmd/containerd-shim-runhcs-v1/serve.go b/cmd/containerd-shim-runhcs-v1/serve.go
index 468c47673e..6a973f1c3c 100644
--- a/cmd/containerd-shim-runhcs-v1/serve.go
+++ b/cmd/containerd-shim-runhcs-v1/serve.go
@@ -183,7 +183,7 @@ var serveCommand = cli.Command{
 			WithTID(idFlag),
 			WithIsSandbox(ctx.Bool("is-sandbox")))
 		if err != nil {
-			return fmt.Errorf("starting service: %w", err)
+			return fmt.Errorf("failed to create new service: %w", err)
 		}
 
 		s, err := ttrpc.NewServer(ttrpc.WithUnaryServerInterceptor(octtrpc.ServerInterceptor()))
@@ -240,7 +240,7 @@ var serveCommand = cli.Command{
 		case err = <-serrs:
 			// the ttrpc server shutdown without processing a shutdown request
 		case <-svc.Done():
-			if !svc.GracefulShutdown {
+			if !svc.gracefulShutdown {
 				// Return immediately, but still close ttrpc server, pipes, and spans
 				// Shouldn't need to os.Exit without clean up (ie, deferred `.Close()`s)
 				return nil
diff --git a/cmd/containerd-shim-runhcs-v1/service.go b/cmd/containerd-shim-runhcs-v1/service.go
index afdf82fc5e..c1a080eff5 100644
--- a/cmd/containerd-shim-runhcs-v1/service.go
+++ b/cmd/containerd-shim-runhcs-v1/service.go
@@ -69,13 +69,13 @@ type service struct {
 	// concurrently.
 	cl sync.Mutex
 
-	// shut is closed to signal a shut request is received
-	shut chan struct{}
-	// shutOnce is responsible for closign `shut` and any other necessary cleanup
-	shutOnce sync.Once
-	// GracefulShutdown dictates whether to shutdown gracefully and clean up resources
+	// shutdown is closed to signal a shutdown request is received
+	shutdown chan struct{}
+	// shutdownOnce is responsible for closing `shutdown` and any other necessary cleanup
+	shutdownOnce sync.Once
+	// gracefulShutdown dictates whether to shutdown gracefully and clean up resources
 	// or exit immediately
-	GracefulShutdown bool
+	gracefulShutdown bool
 }
 
 var _ = (task.TaskService)(&service{})
@@ -90,7 +90,7 @@ func NewService(o ...ServiceOption) (svc *service, err error) {
 		events:    opts.Events,
 		tid:       opts.TID,
 		isSandbox: opts.IsSandbox,
-		shut:      make(chan struct{}),
+		shutdown:  make(chan struct{}),
 	}
 	return svc, nil
 }
@@ -524,12 +524,12 @@ func (s *service) ComputeProcessorInfo(ctx context.Context, req *extendedtask.Co
 }
 
 func (s *service) Done() <-chan struct{} {
-	return s.shut
+	return s.shutdown
 }
 
 func (s *service) IsShutdown() bool {
 	select {
-	case <-s.shut:
+	case <-s.shutdown:
 		return true
 	default:
 		return false
diff --git a/cmd/containerd-shim-runhcs-v1/service_internal.go b/cmd/containerd-shim-runhcs-v1/service_internal.go
index ba98f63b61..27c1a2cfad 100644
--- a/cmd/containerd-shim-runhcs-v1/service_internal.go
+++ b/cmd/containerd-shim-runhcs-v1/service_internal.go
@@ -447,11 +447,11 @@ func (s *service) shutdownInternal(ctx context.Context, req *task.ShutdownReques
 		return empty, nil
 	}
 
-	s.shutOnce.Do(func() {
+	s.shutdownOnce.Do(func() {
 		// TODO: should taskOrPod be deleted/set to nil?
 		// TODO: is there any extra leftovers of the shimTask/Pod to clean? ie: verify all handles are closed?
-		s.GracefulShutdown = !req.Now
-		close(s.shut)
+		s.gracefulShutdown = !req.Now
+		close(s.shutdown)
 	})
 
 	return empty, nil
diff --git a/cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go b/cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go
index 5ea0955833..0d1fcc7766 100644
--- a/cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go
+++ b/cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go
@@ -26,10 +26,12 @@ func setupPodServiceWithFakes(t *testing.T) (*service, *testShimTask, *testShimT
 
 	// clean up the service
 	t.Cleanup(func() {
-		s.shutdownInternal(context.Background(), &task.ShutdownRequest{
+		if _, err := s.shutdownInternal(context.Background(), &task.ShutdownRequest{
 			ID:  s.tid,
 			Now: true,
-		})
+		}); err != nil {
+			t.Fatalf("could not shutdown service: %v", err)
+		}
 	})
 
 	pod := &testShimPod{id: tid}
diff --git a/cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go b/cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go
index e26f2e8760..43fbaa66d1 100644
--- a/cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go
+++ b/cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go
@@ -26,10 +26,12 @@ func setupTaskServiceWithFakes(t *testing.T) (*service, *testShimTask, *testShim
 
 	// clean up the service
 	t.Cleanup(func() {
-		s.shutdownInternal(context.Background(), &task.ShutdownRequest{
+		if _, err := s.shutdownInternal(context.Background(), &task.ShutdownRequest{
 			ID:  s.tid,
 			Now: true,
-		})
+		}); err != nil {
+			t.Fatalf("could not shutdown service: %v", err)
+		}
 	})
 
 	task := &testShimTask{