fix: generate cni conflist if NCs already exist in state (#1940)

* fix: generate cni conflist if NCs already exist in state

* fix: lint

* fix: data race in test

* fix: lint in tests

Author: thatmattlong

cns/restserver/internalapi.go

@@ -164,7 +164,13 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo
 	service.Lock()
 	defer service.Unlock()
 	start := time.Now()
-	err := service.syncHostNCVersion(ctx, channelMode)
+	programmedNCCount, err := service.syncHostNCVersion(ctx, channelMode)
+	// even if we get an error, we want to write the CNI conflist if we have any NC programmed to any version
+	if programmedNCCount > 0 {
+		// This will only be done once per lifetime of the CNS process. This function is threadsafe and will panic
+		// if it fails, so it is safe to call in a non-preemptable goroutine.
+		go service.MustGenerateCNIConflistOnce()
+	}
 	if err != nil {
 		logger.Errorf("sync host error %v", err)
 	}
@@ -174,8 +180,13 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo
 
 var errNonExistentContainerStatus = errors.New("nonExistantContainerstatus")
 
-func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMode string) error {
+// syncHostVersion updates the CNS state with the latest programmed versions of NCs attached to the VM. If any NC in local CNS state
+// does not match the version that DNC claims to have published, this function will call NMAgent and list the latest programmed versions of
+// all NCs and update the CNS state accordingly. This function returns the the total number of NCs on this VM that have been programmed to
+// some version, NOT the number of NCs that are up-to-date.
+func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMode string) (int, error) {
 	outdatedNCs := map[string]struct{}{}
+	programmedNCs := map[string]struct{}{}
 	for idx := range service.state.ContainerStatus {
 		// Will open a separate PR to convert all the NC version related variable to int. Change from string to int is a pain.
 		localNCVersion, err := strconv.Atoi(service.state.ContainerStatus[idx].HostVersion)
@@ -194,13 +205,17 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 		} else if localNCVersion > dncNCVersion {
 			logger.Errorf("NC version from NMAgent is larger than DNC, NC version from NMAgent is %d, NC version from DNC is %d", localNCVersion, dncNCVersion)
 		}
+
+		if localNCVersion > -1 {
+			programmedNCs[service.state.ContainerStatus[idx].ID] = struct{}{}
+		}
 	}
 	if len(outdatedNCs) == 0 {
-		return nil
+		return len(programmedNCs), nil
 	}
 	ncVersionListResp, err := service.nma.GetNCVersionList(ctx)
 	if err != nil {
-		return errors.Wrap(err, "failed to get nc version list from nmagent")
+		return len(programmedNCs), errors.Wrap(err, "failed to get nc version list from nmagent")
 	}
 
 	nmaNCs := map[string]string{}
@@ -223,8 +238,13 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 		if !exist {
 			// if we marked this NC as needs update, but it no longer exists in internal state when we reach
 			// this point, our internal state has changed unexpectedly and we should bail out and try again.
-			return errors.Wrapf(errNonExistentContainerStatus, "can't find NC with ID %s in service state, stop updating this host NC version", ncID)
+			return len(programmedNCs), errors.Wrapf(errNonExistentContainerStatus, "can't find NC with ID %s in service state, stop updating this host NC version", ncID)
 		}
+		// if the NC still exists in state and is programmed to some version (doesn't have to be latest), add it to our set of NCs that have been programmed
+		if nmaNCVersion > -1 {
+			programmedNCs[ncID] = struct{}{}
+		}
+
 		localNCVersion, err := strconv.Atoi(ncInfo.HostVersion)
 		if err != nil {
 			logger.Errorf("failed to parse host nc version string %s: %s", ncInfo.HostVersion, err)
@@ -247,14 +267,10 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 	// if we didn't empty out the needs update set, NMA has not programmed all the NCs we are expecting, and we
 	// need to return an error indicating that
 	if len(outdatedNCs) > 0 {
-		return errors.Errorf("unabled to update some NCs: %v, missing or bad response from NMA", outdatedNCs)
+		return len(programmedNCs), errors.Errorf("unabled to update some NCs: %v, missing or bad response from NMA", outdatedNCs)
 	}
 
-	// if NMA has programmed all the NCs that we expect, we should write the CNI conflist. This will only be done
-	// once per lifetime of the CNS process. This function is threadsafe and will panic if it fails, so it is safe
-	// to call in a non-preemptable goroutine.
-	go service.MustGenerateCNIConflistOnce()
-	return nil
+	return len(programmedNCs), nil
 }
 
 // This API will be called by CNS RequestController on CRD update.

cns/restserver/internalapi_test.go

@@ -9,14 +9,18 @@ import (
 	"os"
 	"reflect"
 	"strconv"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/Azure/azure-container-networking/cns"
 	"github.com/Azure/azure-container-networking/cns/fakes"
 	"github.com/Azure/azure-container-networking/cns/types"
 	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
 	nma "github.com/Azure/azure-container-networking/nmagent"
 	"github.com/google/uuid"
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
 )
 
 const (
@@ -655,3 +659,196 @@ func restartService() {
 		os.Exit(1)
 	}
 }
+
+type mockCNIConflistGenerator struct {
+	generatedCount int
+	mutex          sync.Mutex
+}
+
+func (*mockCNIConflistGenerator) Close() error { return nil }
+func (m *mockCNIConflistGenerator) Generate() error {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
+	m.generatedCount++
+	return nil
+}
+
+func (m *mockCNIConflistGenerator) getGeneratedCount() int {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
+	return m.generatedCount
+}
+
+// TestCNIConflistGenerationNewNC tests that discovering a new programmed NC in CNS state will trigger CNI conflist generation
+func TestCNIConflistGenerationNewNC(t *testing.T) {
+	ncID := "some-new-nc" //nolint:goconst // value not shared across tests, can change without issue
+	mockgen := &mockCNIConflistGenerator{}
+	service := &HTTPRestService{
+		cniConflistGenerator: mockgen,
+		state: &httpRestServiceState{
+			ContainerStatus: map[string]containerstatus{
+				ncID: {
+					ID:          ncID,
+					HostVersion: "-1",
+					CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+						Version: "0",
+					},
+				},
+			},
+		},
+		nma: &fakes.NMAgentClientFake{
+			GetNCVersionListF: func(_ context.Context) (nma.NCVersionList, error) {
+				return nma.NCVersionList{
+					Containers: []nma.NCVersion{
+						{
+							NetworkContainerID: ncID,
+							Version:            "0",
+						},
+					},
+				}, nil
+			},
+		},
+	}
+
+	service.SyncHostNCVersion(context.Background(), cns.CRD)
+	// CNI conflist gen happens in goroutine so sleep for a second to let it run
+	time.Sleep(time.Second)
+	assert.Equal(t, 1, mockgen.getGeneratedCount())
+}
+
+// TestCNIConflistGenerationExistingNC tests that if the CNS starts up with a NC already in its state, it will still generate the conflist
+func TestCNIConflistGenerationExistingNC(t *testing.T) {
+	ncID := "some-existing-nc" //nolint:goconst // value not shared across tests, can change without issue
+	mockgen := &mockCNIConflistGenerator{}
+	service := &HTTPRestService{
+		cniConflistGenerator: mockgen,
+		state: &httpRestServiceState{
+			ContainerStatus: map[string]containerstatus{
+				ncID: {
+					ID:          ncID,
+					HostVersion: "0",
+					CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+						Version: "0",
+					},
+				},
+			},
+		},
+	}
+
+	service.SyncHostNCVersion(context.Background(), cns.CRD)
+	// CNI conflist gen happens in goroutine so sleep for a second to let it run
+	time.Sleep(time.Second)
+	assert.Equal(t, 1, mockgen.getGeneratedCount())
+}
+
+// TestCNIConflistGenerationNewNCTwice tests that discovering a new programmed NC in CNS state will trigger CNI conflist generation, but syncing
+// the host NC version a second time does not regenerate the conflist (conflist should only get generated once per binary lifetime)
+func TestCNIConflistGenerationNewNCTwice(t *testing.T) {
+	ncID := "some-new-nc" //nolint:goconst // value not shared across tests, can change without issue
+	mockgen := &mockCNIConflistGenerator{}
+	service := &HTTPRestService{
+		cniConflistGenerator: mockgen,
+		state: &httpRestServiceState{
+			ContainerStatus: map[string]containerstatus{
+				ncID: {
+					ID:          ncID,
+					HostVersion: "-1",
+					CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+						Version: "0",
+					},
+				},
+			},
+		},
+		nma: &fakes.NMAgentClientFake{
+			GetNCVersionListF: func(_ context.Context) (nma.NCVersionList, error) {
+				return nma.NCVersionList{
+					Containers: []nma.NCVersion{
+						{
+							NetworkContainerID: ncID,
+							Version:            "0",
+						},
+					},
+				}, nil
+			},
+		},
+	}
+
+	service.SyncHostNCVersion(context.Background(), cns.CRD)
+	// CNI conflist gen happens in goroutine so sleep for a second to let it run
+	time.Sleep(time.Second)
+	assert.Equal(t, 1, mockgen.getGeneratedCount())
+
+	service.SyncHostNCVersion(context.Background(), cns.CRD)
+	// CNI conflist gen happens in goroutine so sleep for a second to let it run
+	time.Sleep(time.Second)
+	assert.Equal(t, 1, mockgen.getGeneratedCount()) // should still be one
+}
+
+// TestCNIConflistNotGenerated tests that the cni conflist is not generated if no NCs are programmed
+func TestCNIConflistNotGenerated(t *testing.T) {
+	newNCID := "some-new-nc" //nolint:goconst // value not shared across tests, can change without issue
+	mockgen := &mockCNIConflistGenerator{}
+	service := &HTTPRestService{
+		cniConflistGenerator: mockgen,
+		state: &httpRestServiceState{
+			ContainerStatus: map[string]containerstatus{
+				newNCID: {
+					ID:          newNCID,
+					HostVersion: "-1",
+					CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+						Version: "0",
+					},
+				},
+			},
+		},
+		nma: &fakes.NMAgentClientFake{
+			GetNCVersionListF: func(_ context.Context) (nma.NCVersionList, error) {
+				return nma.NCVersionList{}, nil
+			},
+		},
+	}
+
+	service.SyncHostNCVersion(context.Background(), cns.CRD)
+	// CNI conflist gen happens in goroutine so sleep for a second to let it run
+	time.Sleep(time.Second)
+	assert.Equal(t, 0, mockgen.getGeneratedCount())
+}
+
+// TestCNIConflistGenerationOnNMAError tests that the cni conflist is generated as long as we have at least one programmed NC even if
+// the call to NMA to list NCs fails
+func TestCNIConflistGenerationOnNMAError(t *testing.T) {
+	newNCID := "some-new-nc"           //nolint:goconst // value not shared across tests, can change without issue
+	existingNCID := "some-existing-nc" //nolint:goconst // value not shared across tests, can change without issue
+	mockgen := &mockCNIConflistGenerator{}
+	service := &HTTPRestService{
+		cniConflistGenerator: mockgen,
+		state: &httpRestServiceState{
+			ContainerStatus: map[string]containerstatus{
+				newNCID: {
+					ID:          newNCID,
+					HostVersion: "-1",
+					CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+						Version: "0",
+					},
+				},
+				existingNCID: {
+					ID:          existingNCID,
+					HostVersion: "0",
+					CreateNetworkContainerRequest: cns.CreateNetworkContainerRequest{
+						Version: "0",
+					},
+				},
+			},
+		},
+		nma: &fakes.NMAgentClientFake{
+			GetNCVersionListF: func(_ context.Context) (nma.NCVersionList, error) {
+				return nma.NCVersionList{}, errors.New("some nma error")
+			},
+		},
+	}
+
+	service.SyncHostNCVersion(context.Background(), cns.CRD)
+	// CNI conflist gen happens in goroutine so sleep for a second to let it run
+	time.Sleep(time.Second)
+	assert.Equal(t, 1, mockgen.getGeneratedCount())
+}