fix: carefully retry restarting HNS if it hangs

Signed-off-by: Evan Baker <[email protected]>

Author: rbtr

platform/os_windows.go

@@ -17,6 +17,7 @@ import (
 	"github.com/Azure/azure-container-networking/log"
 	"github.com/Azure/azure-container-networking/platform/windows/adapter"
 	"github.com/Azure/azure-container-networking/platform/windows/adapter/mellanox"
+	"github.com/avast/retry-go/v4"
 	"github.com/pkg/errors"
 	"go.uber.org/zap"
 	"golang.org/x/sys/windows"
@@ -302,32 +303,61 @@ func restartHNS(ctx context.Context) error {
 	}
 	defer service.Close()
 	// Stop the service
-	_, err = service.Control(svc.Stop)
-	if err != nil {
-		return errors.Wrap(err, "could not stop service")
+	log.Printf("Stopping HNS service")
+	_ = retry.Do(
+		tryStopServiceFn(ctx, service),
+		retry.UntilSucceeded(),
+		retry.Context(ctx),
+	)
+	// Start the service again
+	log.Printf("Starting HNS service")
+	if err := service.Start(); err != nil {
+		return errors.Wrap(err, "could not start service")
 	}
-	// Wait for the service to stop
-	ticker := time.NewTicker(500 * time.Millisecond) //nolint:gomnd // 500ms
-	defer ticker.Stop()
-	for { // hacky cancellable do-while
+	log.Printf("HNS service started")
+	return nil
+}
+
+type managedService interface {
+	Control(control svc.Cmd) (svc.Status, error)
+	Query() (svc.Status, error)
+}
+
+func tryStopServiceFn(ctx context.Context, service managedService) func() error {
+	return func() error {
 		status, err := service.Query()
 		if err != nil {
 			return errors.Wrap(err, "could not query service status")
 		}
+		// If the service is already stopped, no need to stop it again
 		if status.State == svc.Stopped {
-			break
+			return nil
 		}
-		select {
-		case <-ctx.Done():
-			return errors.New("context cancelled")
-		case <-ticker.C:
+		_, err = service.Control(svc.Stop)
+		if err != nil {
+			return errors.Wrap(err, "could not stop service")
 		}
+		// Wait for the service to stop
+		ticker := time.NewTicker(500 * time.Millisecond) //nolint:gomnd // 500ms
+		defer ticker.Stop()
+		for {
+			log.Printf("Waiting for HNS service to stop")
+			status, err := service.Query()
+			if err != nil {
+				return errors.Wrap(err, "could not query service status")
+			}
+			if status.State == svc.Stopped {
+				log.Printf("HNS service stopped")
+				break
+			}
+			select {
+			case <-ctx.Done():
+				return errors.New("context cancelled")
+			case <-ticker.C:
+			}
+		}
+		return nil
 	}
-	// Start the service again
-	if err := service.Start(); err != nil {
-		return errors.Wrap(err, "could not start service")
-	}
-	return nil
 }
 
 func HasMellanoxAdapter() bool {

platform/os_windows_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows/svc"
 )
 
 var errTestFailure = errors.New("test failure")
@@ -146,3 +147,110 @@ func TestExecuteCommandTimeout(t *testing.T) {
 	_, err := client.ExecuteCommand(context.Background(), "ping", "-t", "localhost")
 	require.Error(t, err)
 }
+
+type mockManagedService struct {
+	queryFuncs  []func() (svc.Status, error)
+	controlFunc func(svc.Cmd) (svc.Status, error)
+}
+
+func (m *mockManagedService) Query() (svc.Status, error) {
+	queryFunc := m.queryFuncs[0]
+	m.queryFuncs = m.queryFuncs[1:]
+	return queryFunc()
+}
+
+func (m *mockManagedService) Control(cmd svc.Cmd) (svc.Status, error) {
+	return m.controlFunc(cmd)
+}
+
+func TestTryStopServiceFn(t *testing.T) {
+	tests := []struct {
+		name        string
+		queryFuncs  []func() (svc.Status, error)
+		controlFunc func(svc.Cmd) (svc.Status, error)
+		expectError bool
+	}{
+		{
+			name: "Service already stopped",
+			queryFuncs: []func() (svc.Status, error){
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Stopped}, nil
+				},
+			},
+			controlFunc: nil,
+			expectError: false,
+		},
+		{
+			name: "Service running and stops successfully",
+			queryFuncs: []func() (svc.Status, error){
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Running}, nil
+				},
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Stopped}, nil
+				},
+			},
+			controlFunc: func(svc.Cmd) (svc.Status, error) {
+				return svc.Status{State: svc.Stopped}, nil
+			},
+			expectError: false,
+		},
+		{
+			name: "Service running and stops after multiple attempts",
+			queryFuncs: []func() (svc.Status, error){
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Running}, nil
+				},
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Running}, nil
+				},
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Running}, nil
+				},
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Stopped}, nil
+				},
+			},
+			controlFunc: func(svc.Cmd) (svc.Status, error) {
+				return svc.Status{State: svc.Stopped}, nil
+			},
+			expectError: false,
+		},
+		{
+			name: "Service running and fails to stop",
+			queryFuncs: []func() (svc.Status, error){
+				func() (svc.Status, error) {
+					return svc.Status{State: svc.Running}, nil
+				},
+			},
+			controlFunc: func(svc.Cmd) (svc.Status, error) {
+				return svc.Status{State: svc.Running}, errors.New("failed to stop service") //nolint:err113 // test error
+			},
+			expectError: true,
+		},
+		{
+			name: "Service query fails",
+			queryFuncs: []func() (svc.Status, error){
+				func() (svc.Status, error) {
+					return svc.Status{}, errors.New("failed to query service status") //nolint:err113 // test error
+				},
+			},
+			controlFunc: nil,
+			expectError: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			service := &mockManagedService{
+				queryFuncs:  tt.queryFuncs,
+				controlFunc: tt.controlFunc,
+			}
+			err := tryStopServiceFn(context.Background(), service)()
+			if tt.expectError {
+				assert.Error(t, err)
+				return
+			}
+			assert.NoError(t, err)
+		})
+	}
+}