Skip to content

Conversation

@NihaNallappagari
Copy link
Contributor

@NihaNallappagari NihaNallappagari commented Jul 10, 2025

Reason for Change:
Swiftv2 network containers (NCs) are published to a different path in pubsub compared to Swiftv1 NCs, and the NMA getNCDetails API only returns programmed Swiftv1 NC details. For Swiftv2, the programmed NC goal state is exposed through IMDS, so we need to query IMDS to retrieve those NC details. In this code review, I am adding a call to NMA SupportedAPIs to verify that the build includes the required features. If the expected API is present, I proceed to call the IMDS NC details API and combine that information with the Swiftv1 NMA API results to ensure any outdated NCs are updated with the latest details from IMDS or NMA. If the latest build is not detected, the IMDS NCVersions API call is skipped, and only the NMA NCVersions API is used for the query swiftv1 NC goal state

Issue Fixed:

Requirements:

Notes:

update values add client test scenarios . . . . .
Copilot AI review requested due to automatic review settings July 10, 2025 15:27
@NihaNallappagari NihaNallappagari requested a review from a team as a code owner July 10, 2025 15:27
@NihaNallappagari NihaNallappagari requested a review from camrynl July 10, 2025 15:27
@NihaNallappagari NihaNallappagari changed the title Delegated nc update [Draft] Sync delegated nic associated nc with NMA published goal state Jul 10, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Implements delegated network container version updates by combining NMAgent and IMDS version sources and expands related unit tests.

  • Removes obsolete log calls in CNS service initializers.
  • Enhances syncHostNCVersion to fetch NC versions from IMDS, merge with NMAgent data, and update host state.
  • Adds new GetNCVersionsFromIMDS support in IMDS client and covers success and error scenarios with unit tests.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
cns/service/main.go Removed redundant logger.Printf calls in controller initializers
cns/restserver/restserver.go Added GetNCVersionsFromIMDS to imdsClient interface
cns/restserver/internalapi.go Merged IMDS NC versions into syncHostNCVersion and added helper methods
cns/restserver/internalapi_test.go Expanded SyncHostNCVersion tests to cover IMDS fallback and error cases
cns/imds/client.go Introduced GetNCVersionsFromIMDS and associated types for network metadata
cns/imds/client_test.go Added tests for GetNCVersionsFromIMDS, including success and failure cases
cns/fakes/imdsclientfake.go Mock implementation for GetNCVersionsFromIMDS
Comments suppressed due to low confidence (1)

cns/restserver/internalapi.go:25

  • The code uses strings (e.g., strings.ToLower, strings.Contains) and fmt.Errorf but the import block does not include "strings" or "fmt". Add these imports to avoid compile errors.
"github.com/pkg/errors" 
@NihaNallappagari NihaNallappagari changed the title [Draft] Sync delegated nic associated nc with NMA published goal state [Draft] Sync swiftv2 nc with NMA published goal state Jul 10, 2025
@NihaNallappagari NihaNallappagari changed the title [Draft] Sync swiftv2 nc with NMA published goal state [Draft][feat] Sync swiftv2 nc with NMA published goal state Jul 10, 2025
@NihaNallappagari NihaNallappagari marked this pull request as draft July 10, 2025 15:58
@Azure Azure deleted a comment from Copilot AI Jul 28, 2025
@Azure Azure deleted a comment from Copilot AI Jul 28, 2025
@Azure Azure deleted a comment from Copilot AI Jul 28, 2025
@NihaNallappagari NihaNallappagari marked this pull request as ready for review July 29, 2025 07:53
@NihaNallappagari NihaNallappagari changed the title [Draft][feat] Sync swiftv2 nc with NMA published goal state [feat] Sync swiftv2 nc with NMA published goal state Jul 29, 2025
@NihaNallappagari NihaNallappagari changed the title [feat] Sync swiftv2 nc with NMA published goal state [feat] synchronize NC version with NMA programmed goal state Jul 29, 2025
miguelgoms
miguelgoms previously approved these changes Aug 27, 2025
@NihaNallappagari
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).
@NihaNallappagari NihaNallappagari added this pull request to the merge queue Sep 2, 2025
@NihaNallappagari NihaNallappagari removed this pull request from the merge queue due to a manual request Sep 2, 2025
@NihaNallappagari NihaNallappagari added this pull request to the merge queue Sep 2, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 3, 2025
@NihaNallappagari NihaNallappagari added this pull request to the merge queue Sep 3, 2025
Merged via the queue into Azure:master with commit 2f0311d Sep 3, 2025
16 checks passed
@NihaNallappagari NihaNallappagari deleted the delegatedNCUpdate branch September 3, 2025 22:05
NihaNallappagari added a commit to NihaNallappagari/azure-container-networking that referenced this pull request Sep 4, 2025
) * Make nma api calls and imds call to update delegated NC goal state update values add client test scenarios . . . . . * Address comments * fix lint error * address code review comments * update imds version and * Check imds version to confirm if imds changes are available * fix formatting errors * . * Update naming convenction * remove nmagent api check
sivakami-projects pushed a commit that referenced this pull request Oct 23, 2025
* Make nma api calls and imds call to update delegated NC goal state update values add client test scenarios . . . . . * Address comments * fix lint error * address code review comments * update imds version and * Check imds version to confirm if imds changes are available * fix formatting errors * . * Update naming convenction * remove nmagent api check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants