Fix logging response from NMAgent in syncHostNCVersion function

Co-authored-by: thatmattlong <[email protected]>

Author: Copilot

cns/restserver/internalapi.go

@@ -226,10 +226,18 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 		nmaNCs[strings.ToLower(nc.NetworkContainerID)] = nc.Version
 	}
 	hasNC.Set(float64(len(nmaNCs)))
+	
+	// Track NCs missing from NMAgent response and outdated NCs for better error reporting
+	missingNCs := make(map[string]string)       // ncID -> expected version
+	outdatedNMaNCs := make(map[string]string)   // ncID -> "expected:actual" version info
+	
 	for ncID := range outdatedNCs {
 		nmaNCVersionStr, ok := nmaNCs[ncID]
 		if !ok {
-			// NMA doesn't have this NC that we need programmed yet, bail out
+			// NMA doesn't have this NC that we need programmed yet, track as missing
+			if ncInfo, exists := service.state.ContainerStatus[ncID]; exists {
+				missingNCs[ncID] = ncInfo.CreateNetworkContainerRequest.Version
+			}
 			continue
 		}
 		nmaNCVersion, err := strconv.Atoi(nmaNCVersionStr)
@@ -258,6 +266,20 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo
 			logger.Errorf("NC version from NMA is decreasing: have %d, got %d", localNCVersion, nmaNCVersion)
 			continue
 		}
+		
+		expectedVersion := ncInfo.CreateNetworkContainerRequest.Version
+		expectedVersionInt, err := strconv.Atoi(expectedVersion)
+		if err != nil {
+			logger.Errorf("failed to parse expected NC version string %s: %s", expectedVersion, err)
+			continue
+		}
+		
+		// Check if NMAgent version is still outdated compared to expected DNC version
+		if nmaNCVersion < expectedVersionInt {
+			outdatedNMaNCs[ncID] = fmt.Sprintf("expected:%s,actual:%s", expectedVersion, nmaNCVersionStr)
+			continue
+		}
+		
 		if channelMode == cns.CRD {
 			service.MarkIpsAsAvailableUntransacted(ncInfo.ID, nmaNCVersion)
 		}
@@ -271,7 +293,18 @@ 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 len(programmedNCs), errors.Errorf("unabled to update some NCs: %v, missing or bad response from NMA", outdatedNCs)
+		var errorParts []string
+		if len(missingNCs) > 0 {
+			errorParts = append(errorParts, fmt.Sprintf("missing NCs from NMAgent response: %v", missingNCs))
+		}
+		if len(outdatedNMaNCs) > 0 {
+			errorParts = append(errorParts, fmt.Sprintf("outdated NCs in NMAgent response: %v", outdatedNMaNCs))
+		}
+		if len(errorParts) == 0 {
+			// Fallback for any unexpected cases
+			errorParts = append(errorParts, fmt.Sprintf("unable to update NCs: %v", outdatedNCs))
+		}
+		return len(programmedNCs), errors.Errorf("unable to update some NCs - %s", strings.Join(errorParts, "; "))
 	}
 
 	return len(programmedNCs), nil

cns/restserver/internalapi_test.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"reflect"
 	"strconv"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -330,6 +331,86 @@ func createNCReqeustForSyncHostNCVersion(t *testing.T) cns.CreateNetworkContaine
 	return req
 }
 
+func TestSyncHostNCVersionErrorMessages(t *testing.T) {
+	tests := []struct {
+		name              string
+		ncVersionInDNC    string
+		nmaResponse       nma.NCVersionList
+		expectedErrorSubstring string
+	}{
+		{
+			name:           "missing NC from NMAgent response",
+			ncVersionInDNC: "2",
+			nmaResponse: nma.NCVersionList{
+				Containers: []nma.NCVersion{}, // Empty response - NC is missing
+			},
+			expectedErrorSubstring: "missing NCs from NMAgent response",
+		},
+		{
+			name:           "outdated NC in NMAgent response",
+			ncVersionInDNC: "2",
+			nmaResponse: nma.NCVersionList{
+				Containers: []nma.NCVersion{
+					{
+						NetworkContainerID: ncID,
+						Version:            "1", // Outdated version compared to DNC version 2
+					},
+				},
+			},
+			expectedErrorSubstring: "outdated NCs in NMAgent response",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create NC request with specified DNC version
+			restartService()
+			setEnv(t)
+			setOrchestratorTypeInternal(cns.KubernetesCRD)
+
+			secondaryIPConfigs := make(map[string]cns.SecondaryIPConfig)
+			ipAddress := "10.0.0.16"
+			secIPConfig := newSecondaryIPConfig(ipAddress, 0)
+			ipID := uuid.New()
+			secondaryIPConfigs[ipID.String()] = secIPConfig
+			_ = createNCReqInternal(t, secondaryIPConfigs, ncID, tt.ncVersionInDNC)
+
+			// Set up mock NMAgent with the specified response
+			mnma := &fakes.NMAgentClientFake{
+				GetNCVersionListF: func(_ context.Context) (nma.NCVersionList, error) {
+					return tt.nmaResponse, nil
+				},
+			}
+			cleanup := setMockNMAgent(svc, mnma)
+			defer cleanup()
+
+			// Call syncHostNCVersion and verify it returns the expected error
+			programmedCount, err := svc.syncHostNCVersion(context.Background(), cns.KubernetesCRD)
+			
+			t.Logf("Test case: %s, programmedCount: %d, error: %v", tt.name, programmedCount, err)
+			
+			if err == nil {
+				t.Errorf("Expected error but got none")
+				return
+			}
+			
+			if !strings.Contains(err.Error(), tt.expectedErrorSubstring) {
+				t.Errorf("Expected error message to contain '%s', but got: %s", tt.expectedErrorSubstring, err.Error())
+			}
+			
+			// In the outdated case, NMAgent has a version of the NC so it counts as programmed, just not up-to-date
+			expectedProgrammedCount := 0
+			if tt.name == "outdated NC in NMAgent response" {
+				expectedProgrammedCount = 1 // NMAgent has version 1, so it's programmed to some version
+			}
+			
+			if programmedCount != expectedProgrammedCount {
+				t.Errorf("Expected programmedCount to be %d, but got %d", expectedProgrammedCount, programmedCount)
+			}
+		})
+	}
+}
+
 func TestReconcileNCWithEmptyState(t *testing.T) {
 	restartService()
 	setEnv(t)