Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support in CNS NMAgent Client to fetch secondary IPs #3017

Merged
merged 27 commits into from
Sep 19, 2024
Merged

Add support in CNS NMAgent Client to fetch secondary IPs #3017

merged 27 commits into from
Sep 19, 2024

Conversation

santhoshmprabhu
Copy link
Contributor

@santhoshmprabhu santhoshmprabhu commented Sep 13, 2024 •
edited
Loading

Reason for Change:
As part of the work to add support for NodeSubnet with Cilium, we need the CNS to be able to fetch IPs from NMAgent. This PR adds this ability to the NMAgent client in CNS. Specifically, this PR introduces:

  1. A new request object for Secondary IP requests
  2. A new method to interact with the NMAgent to get the IPs
  3. A wrapper to invoke the new method in the NMAgent client. The wrapper implements basic throttling too.
  4. Unit Tests

The fetch functionality mimics the behavior of azure-vnet-ipam, but has been re-implemented to match the NMAgent client architecture in CNS.

Issue Fixed:
NA

Requirements:

@santhoshmprabhu santhoshmprabhu requested a review from a team as a code owner September 13, 2024 22:39
@santhoshmprabhu santhoshmprabhu self-assigned this Sep 13, 2024
@santhoshmprabhu santhoshmprabhu added cns Related to CNS. go Pull requests that update Go code labels Sep 13, 2024
@santhoshmprabhu
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Microsoft"

@santhoshmprabhu
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

timraymond
timraymond requested changes Sep 16, 2024
View reviewed changes
Copy link
Member

@timraymond timraymond left a comment

Choose a reason for hiding this comment

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

Good start, I like the addition of the tests, but there's a few changes that need to be made to align it with the rest of the package.

nmagent/client.go Outdated Show resolved Hide resolved
nmagent/client.go Outdated Show resolved Hide resolved
nmagent/client.go Outdated Show resolved Hide resolved
nmagent/client.go Outdated Show resolved Hide resolved
@santhoshmprabhu santhoshmprabhu requested a review from a team as a code owner September 16, 2024 22:26
@santhoshmprabhu
Copy link
Contributor Author

@timraymond Thanks for the comments, addressed them. Please take another look.

@santhoshmprabhu
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@santhoshmprabhu
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@santhoshmprabhu
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

timraymond
timraymond previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@timraymond timraymond left a comment

Choose a reason for hiding this comment

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

Mostly non-blocking feedback about doc comments but otherwise LGTM.

nmagent/client.go Show resolved Hide resolved
cns/nodesubnet/ip_fetcher.go Outdated Show resolved Hide resolved
cns/nodesubnet/ip_fetcher.go Outdated Show resolved Hide resolved
cns/nodesubnet/ip_fetcher.go Outdated Show resolved Hide resolved
timraymond
timraymond previously approved these changes Sep 18, 2024
View reviewed changes
@santhoshmprabhu
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

rbtr
rbtr reviewed Sep 18, 2024
View reviewed changes
Copy link
Contributor

@rbtr rbtr left a comment

Choose a reason for hiding this comment

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

lgtm just one suggestion

nmagent/ipaddress.go Outdated Show resolved Hide resolved
nmagent/ipaddress.go Outdated Show resolved Hide resolved
@santhoshmprabhu
Copy link
Contributor Author

/azp run Azure Container Networking PR

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

tamilmani1989
tamilmani1989 approved these changes Sep 18, 2024
View reviewed changes
nmagent/client.go Show resolved Hide resolved
@santhoshmprabhu santhoshmprabhu added this pull request to the merge queue Sep 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2024
@santhoshmprabhu santhoshmprabhu added this pull request to the merge queue Sep 19, 2024
Merged via the queue into master with commit e6c2e2a Sep 19, 2024
14 checks passed
@santhoshmprabhu santhoshmprabhu deleted the sanprabhu/cilium-node-subnet-nma-client-changes branch September 19, 2024 05:52
@santhoshmprabhu santhoshmprabhu mentioned this pull request Sep 19, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timraymond timraymond timraymond left review comments

@rbtr rbtr rbtr approved these changes

@tamilmani1989 tamilmani1989 tamilmani1989 approved these changes

@ecigar13 ecigar13 Awaiting requested review from ecigar13 ecigar13 is a code owner automatically assigned from Azure/azure-sdn-members

@behzad-mir behzad-mir Awaiting requested review from behzad-mir

@QxBytes QxBytes Awaiting requested review from QxBytes

Assignees

@santhoshmprabhu santhoshmprabhu

Labels
cns Related to CNS. go Pull requests that update Go code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@santhoshmprabhu @timraymond @tamilmani1989 @rbtr