feat: expose getHomeAzInfo api in cns to retrieve node home az infos from NMAgent #1642

ZetaoZhuang · 2022-10-03T18:04:14Z

Reason for Change:

To enable AZR , dnc need to call CNS to get home az infos from NMAgent. This PR is for exposing the getHomeAzInfo api in cns to achieve that.

caching homeaz infos will be covered in separated PR

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:
task link

cns/nmagent/client.go

cns/NetworkContainerContract.go

cns/client/client_test.go

cns/restserver/api.go

cns/restserver/api_test.go

cns/service/main.go

cns/types/codes.go

nmagent/client.go

nmagent/client_test.go

cns/types/codes.go

smittal22

Looks good to me.

cns/restserver/api.go

cns/restserver/api_test.go

cns/restserver/api.go

cns/restserver/restserver.go

cns/restserver/util.go

nmagent/client.go

nmagent/client_test.go

nmagent/responses.go

rbtr

do I understand correctly that nothing is currently populating these "cache" values? why not make them pull-through, so that the first attempt to read the cache does the look-up if the value has not been set? I prefer that to having an out-of-band cache put process.

cns/NetworkContainerContract.go

cns/api.go

cns/restserver/util.go

rbtr · 2022-11-02T20:38:26Z

cns/restserver/util.go

+func (service *HTTPRestService) readNMASupportedApisCache() []string {
+	service.RLock()
+	supportedApis := service.nmaSupportedApisCache
+	service.RUnlock()
+	return supportedApis
+}


I know that doing this with a map would not be thread-safe; I am not sure if returning a slice like this is thread-safe, since the slice is a sort of reference type and the underlying array can be mutated through any copy of the slice?

Ya, the slice is a sort of reference type. I don't see any scenario we would mutate the copy slice for now. But I can do a deep copy here so it has 0 chance to mutate the underlying cache value.

nmagent/responses.go

ZetaoZhuang · 2022-11-02T21:00:42Z

do I understand correctly that nothing is currently populating these "cache" values? why not make them pull-through, so that the first attempt to read the cache does the look-up if the value has not been set? I prefer that to having an out-of-band cache put process.

Ya, currently nothing is populated to these caches. Based on the design we agreed on, we will have a thread to query home az from nmagent and cache the value when cns starts. @ashvindeodhar

timraymond

I have more thoughts here, but I just want to checkpoint this review so that some feedback can be provided before I'm finished.

cns/restserver/internalapi_test.go

cns/restserver/util.go

timraymond

Some more questions on the CachedClient (it is looking better!). Also have some other questions about error handling.

nmagent/cachedclient.go

nmagent/cachedclient_test.go

cns/restserver/api.go

nmagent/cachedclient_test.go

nmagent/cachedclient.go

cns/api.go

cns/configuration/configuration.go

cns/restserver/api.go

cns/service/main.go

nmagent/cachedclient.go

nmagent/client_test.go

remove CachedClient make homeaz caching thread periodically pulling home az from nmagent indefinitely

outdated

ZetaoZhuang · 2022-12-05T23:15:13Z

/azp run

azure-pipelines · 2022-12-05T23:15:28Z

Azure Pipelines successfully started running 1 pipeline(s).

aegal · 2022-12-06T18:45:05Z

/azp run

azure-pipelines · 2022-12-06T18:45:21Z

Azure Pipelines successfully started running 1 pipeline(s).

rbtr · 2022-12-08T17:14:22Z

@ashvindeodhar please leave a note explaining why this was merged bypassing checks

ashvindeodhar · 2022-12-08T19:31:24Z

@rbtr sorry, I added a note but seems didn't hit the 'comment' button.
Merging this bypassing the windows check which has been failing for last few days. We need this change for making progress on aquarius changes and testing the critical AZR feature in timely fashion. This change is not updating any existing code / functionality so risk of any windows regressions are very low. This code adds new functionality which is currently not used. After checking with pipeline v-team ( Esteban ), I came to know that this might need few more days to fix this windows pipeline issue.

…from NMAgent (Azure#1642)

ZetaoZhuang requested a review from a team as a code owner October 3, 2022 18:04

ZetaoZhuang requested review from thatmattlong and removed request for a team October 3, 2022 18:04

timraymond requested changes Oct 3, 2022

View reviewed changes

cns/nmagent/client.go Outdated Show resolved Hide resolved

timraymond previously approved these changes Oct 10, 2022

View reviewed changes

ZetaoZhuang dismissed timraymond’s stale review via 8703ca3 October 11, 2022 17:33

timraymond previously approved these changes Oct 11, 2022

View reviewed changes

ZetaoZhuang dismissed timraymond’s stale review via 33fd55d October 13, 2022 17:55

ZetaoZhuang force-pushed the GetHomeAzInfo branch from 33fd55d to 676cbba Compare October 13, 2022 18:00

ZetaoZhuang requested a review from timraymond October 13, 2022 18:01

ashvindeodhar requested changes Oct 19, 2022

View reviewed changes

ZetaoZhuang force-pushed the GetHomeAzInfo branch from d68a5d4 to 4d52c08 Compare October 21, 2022 00:04

ZetaoZhuang requested a review from ashvindeodhar October 21, 2022 00:06

smittal22 reviewed Oct 25, 2022

View reviewed changes

cns/types/codes.go Outdated Show resolved Hide resolved

smittal22 reviewed Oct 25, 2022

View reviewed changes

cns/types/codes.go Outdated Show resolved Hide resolved

smittal22 previously approved these changes Oct 25, 2022

View reviewed changes

ashvindeodhar requested changes Nov 1, 2022

View reviewed changes

ZetaoZhuang dismissed smittal22’s stale review via 46f96ea November 2, 2022 17:40

rbtr previously requested changes Nov 2, 2022

View reviewed changes

ZetaoZhuang requested review from ashvindeodhar, rbtr and smittal22 November 2, 2022 23:41

timraymond requested changes Nov 3, 2022

View reviewed changes

cns/restserver/internalapi_test.go Outdated Show resolved Hide resolved

cns/restserver/util.go Outdated Show resolved Hide resolved

ZetaoZhuang force-pushed the GetHomeAzInfo branch from 7720a87 to 6989483 Compare November 4, 2022 18:05

timraymond requested changes Nov 4, 2022

View reviewed changes

ZetaoZhuang force-pushed the GetHomeAzInfo branch 2 times, most recently from 41ee6ec to acafba7 Compare November 8, 2022 18:23

timraymond requested changes Nov 8, 2022

View reviewed changes

nmagent/cachedclient_test.go Outdated Show resolved Hide resolved

nmagent/cachedclient_test.go Outdated Show resolved Hide resolved

nmagent/cachedclient.go Outdated Show resolved Hide resolved

nmagent/cachedclient.go Outdated Show resolved Hide resolved

ashvindeodhar requested changes Nov 10, 2022

View reviewed changes

ZetaoZhuang added 6 commits November 30, 2022 11:13

address comment

9835284

refactoring code to have homeaz caching as a separated entity

755652c

remove CachedClient make homeaz caching thread periodically pulling home az from nmagent indefinitely

rename func

b000607

refactor code

671d353

address comment

dbc924c

rebase master and address comments

4cc049b

ZetaoZhuang force-pushed the GetHomeAzInfo branch from 9a8d022 to 4cc049b Compare November 30, 2022 19:55

ZetaoZhuang requested a review from ashvindeodhar November 30, 2022 20:01

address comments

53bdf68

timraymond approved these changes Dec 1, 2022

View reviewed changes

ashvindeodhar approved these changes Dec 2, 2022

View reviewed changes

Merge branch 'master' into GetHomeAzInfo

6131c5c

ZetaoZhuang enabled auto-merge (squash) December 2, 2022 23:21

Merge branch 'master' into GetHomeAzInfo

5a3f99c

ashvindeodhar disabled auto-merge December 8, 2022 16:22

ashvindeodhar merged commit e6eadd3 into master Dec 8, 2022

ashvindeodhar deleted the GetHomeAzInfo branch December 8, 2022 16:23

rjdenney pushed a commit to rjdenney/azure-container-networking that referenced this pull request Jan 19, 2023

feat: expose getHomeAzInfo api in cns to retrieve node home az infos …

7b1a3ea

…from NMAgent (Azure#1642)

smittal22 pushed a commit to smittal22/azure-container-networking that referenced this pull request Jan 26, 2023

feat: expose getHomeAzInfo api in cns to retrieve node home az infos …

6aece64

…from NMAgent (Azure#1642)

smittal22 pushed a commit to smittal22/azure-container-networking that referenced this pull request Jan 30, 2023

feat: expose getHomeAzInfo api in cns to retrieve node home az infos …

9ac5146

…from NMAgent (Azure#1642)

smittal22 pushed a commit to smittal22/azure-container-networking that referenced this pull request Feb 3, 2023

feat: expose getHomeAzInfo api in cns to retrieve node home az infos …

f358e9a

…from NMAgent (Azure#1642)

tamilmani1989 added the cns Related to CNS. label Mar 16, 2023

smittal22 added the AZR AZR related PR label Aug 26, 2024

feat: expose getHomeAzInfo api in cns to retrieve node home az infos from NMAgent #1642

feat: expose getHomeAzInfo api in cns to retrieve node home az infos from NMAgent #1642

Uh oh!

Conversation

ZetaoZhuang commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smittal22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rbtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rbtr Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

ZetaoZhuang Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZetaoZhuang commented Nov 2, 2022

Uh oh!

timraymond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

timraymond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZetaoZhuang commented Dec 5, 2022

Uh oh!

azure-pipelines bot commented Dec 5, 2022

Uh oh!

aegal commented Dec 6, 2022

Uh oh!

ZetaoZhuang commented Oct 3, 2022 •

edited

Loading