feat: Creating NNC with HomeAz info in AKS-Swift Workflows when CNS starts up behind a configuration flag #3157

nairashu · 2024-11-19T21:41:47Z

Reason for Change:
feat: Creating NNC with HomeAz info in AKS-Swift Workflows when CNS starts up behind a configuration flag

Issue Fixed:
N/A

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

timraymond · 2024-11-20T15:25:00Z

@nairashu If this is still WIP, can you mark this as draft?

ramiro-gamarra

A few questions/thoughts

ramiro-gamarra · 2024-12-18T23:43:12Z

cns/configuration/configuration.go

 	EnableSubnetScarcity        bool
 	EnableSwiftV2               bool
 	InitializeFromCNI           bool
+	EnableHomeAz                bool


Maybe this should be EnableNNCCreation. It will be clearer when AKS reviews toggles to CNS config.

We typically control the component configurations closer to our service feature naming so keeping it related to HomeAz makes more sense as the flag is to use the HomeAzMonitor overall to retrieve the Az and then create the NNC from that.

I know that "Az" is everywhere, so it's probably impractical to change at this point, but "AZ" is an abbreviation, so this should be EnableHomeAZ by typical Go naming conventions.

Changed this to EnableHomeAZ

cns/service/main.go

ramiro-gamarra · 2024-12-18T23:51:34Z

cns/service/main.go

+		var nnc *v1alpha.NodeNetworkConfig
+		if nnc, err = directnnccli.Get(ctx, types.NamespacedName{Namespace: "kube-system", Name: nodeName}); err != nil {
+			logger.Errorf("[Azure CNS] failed to get existing NNC: %v", err)
+		}
+
+		newNNC := createBaseNNC(node)
+		if nnc == nil {
+			logger.Printf("[Azure CNS] Creating new base NNC")


Error conditions are not clear here. If there is a transient network error, the get should be retried no? Otherwise, is there a way to detect that we reached the apiserver and it returned "not found"? That should be the only time when we attempt to create the nnc.

Adding a retrier in here for transient errors.

cns/service/main.go

timraymond · 2024-12-19T17:05:07Z

cns/restserver/internalapi.go

+func (service *HTTPRestService) GetHomeAz(ctx context.Context) (homeAzResponse cns.GetHomeAzResponse) {
+	service.RLock()
+	homeAzResponse = service.homeAzMonitor.GetHomeAz(ctx)
+	service.RUnlock()


This should be defer-ed to insulate it from panics

Added the defer but also noticed this file is missing defer in multiple other places

"Leave the campsite cleaner than you found it" applies -- but also I only care about this instance for sake of this PR.

Totally on the same page with that philosophy so did add in the defer above.

cns/restserver/internalapi.go

.golangci.yml

timraymond · 2024-12-19T18:03:03Z

cns/configuration/configuration.go

 	EnableSubnetScarcity        bool
 	EnableSwiftV2               bool
 	InitializeFromCNI           bool
+	EnableHomeAz                bool


I know that "Az" is everywhere, so it's probably impractical to change at this point, but "AZ" is an abbreviation, so this should be EnableHomeAZ by typical Go naming conventions.

cns/service/main.go

… Go version

… Cache

…Az accordingly

…z value

…4 type

… type

ramiro-gamarra · 2024-12-31T19:12:13Z

cns/configuration/configuration.go

 	EnableSubnetScarcity        bool
 	EnableSwiftV2               bool
 	InitializeFromCNI           bool
+	EnableHomeAZ                bool


I think the previous comment got lost but I think we need further discussion on the name of this field. I don't think EnableHomeAZ is the correct name because:

Home AZ is already enabled when mode is direct, which makes this flag confusing.

The major feature is the NNC creation, which should be independent of home az being available or not.
I still think EnableNNCCreation is a better name. Would like to get feedback from @thatmattlong and @rbtr on this.

agree, flag should tightly describe what function it does, not the abstract name of the scenario.
CreateNNC would be even better

ramiro-gamarra · 2024-12-31T19:12:26Z

cns/service/main.go

 	directscopedcli := nncctrl.NewScopedClient(directnnccli, types.NamespacedName{Namespace: "kube-system", Name: nodeName})

+	// Create the base NNC CRD if HomeAz is enabled
+	if cnsconfig.EnableHomeAZ {


better to extract the scope opened by this flag to a different function. it will help clean up some code paths.

i would like to see NNC creation decoupled completely from HomeAz

rbtr · 2025-01-03T22:33:32Z

.pipelines/mdnc/azure-cns-cni-1.4.39.1.yaml

 - apiGroups: ["acn.azure.com"]
  resources: ["nodenetworkconfigs"]
-  verbs: ["get", "list", "watch", "patch", "update"]
+  verbs: ["create", "delete", "get", "list", "watch", "patch", "update"]


i don't think we'll need delete

The design is to move the node reconcile logic over to CNS.
It makes more sense to create and cleanup the NNC based on when the node is created and deleted. As a result, I added the operational capability to delete the NNC as well to CNS as eventually we can clean up the nodes faster in this case. DNC, DNC-RC and DNCCleanup can work async as it does today to cleanup the NC and other associations.

Why do we want to avoid CNS from doing this?

In what scenario could CNS know it should delete an NNC?
I think it's impossible. The NNC should only be deleted once the Node no longer exists. If the Node doesn't exist, CNS cannot be running and can't delete the NNC.
Even DNC-RC does not delete NNCs - when the NNC is created it gets its OwnerRef set to the Node object. Then when the Node object is deleted from Kubernetes, the NNC is automatically garbage collected. These refs use object UUIDs; this is the only safe way to delete an NNC.

rbtr · 2025-01-03T22:40:29Z

cns/restserver/internalapi.go

+// GetHomeAz - Get the Home Az for the Node where CNS is running.
+func (service *HTTPRestService) GetHomeAz(ctx context.Context) (cns.GetHomeAzResponse, error) {
+	service.RLock()
+	defer service.RUnlock()
+	homeAzResponse := service.homeAzMonitor.GetHomeAz(ctx)
+	if homeAzResponse.Response.ReturnCode == types.NotFound {
+		return homeAzResponse, errors.New(homeAzResponse.Response.Message)
+	}
+
+	return homeAzResponse, nil
+}


what value is this adding over calling service.homeAzMonitor.GetHomeAz directly?

github-actions · 2025-01-22T00:01:16Z

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

github-actions · 2025-01-29T00:01:22Z

Pull request closed due to inactivity.

nairashu requested review from a team as code owners November 19, 2024 21:41

nairashu requested a review from paulyufan2 November 19, 2024 21:41

nairashu force-pushed the asn/HomeAz branch from 0e4d116 to c53c237 Compare November 25, 2024 22:49

nairashu force-pushed the asn/HomeAz branch from 8a12c9b to 85eb1ad Compare December 3, 2024 23:51

nairashu changed the title ~~[WIP]: feat: Creating NNC with HomeAz info in AKS-Swift Workflows when CNS starts up behind a configuration flag~~ feat: Creating NNC with HomeAz info in AKS-Swift Workflows when CNS starts up behind a configuration flag Dec 17, 2024

nairashu force-pushed the asn/HomeAz branch from a6974e0 to 0ded5a3 Compare December 18, 2024 22:58

ramiro-gamarra requested changes Dec 18, 2024

View reviewed changes

timraymond reviewed Dec 19, 2024

View reviewed changes

nairashu and others added 16 commits December 23, 2024 14:35

feat: Creating NNC with HomeAz info in AKS-Swift Workflows

cc81929

feat: Adding placeholders for NNC creation logic

95d78f7

Adding a base create NNC method

48e0e1d

feat: Adding placeholders for NNC creation logic

ff0bb65

fix: Removed the ctx from the baseNNC creation

0b1ca25

fix: Added error check while writing the base NNC to the cluster

69cb096

fix: Added error check while writing the base NNC to the cluster

e5f2637

fix: Disabling the gomnd linter as it is no loger active with the new…

c7129e1

… Go version

feat: Updating the NNC with the HomeAz details from the homeAzMonitor…

978fe6c

… Cache

fix: Updated the context that needs to be passed

600dd98

fix:Updating the NNC to Patch or create a new NNC and update the Home…

df7c25a

…Az accordingly

fix: Updated the code to delete and the create the NNC with the HomeA…

e0b0a89

…z value

upadte: Fixing the type for the Availability zone for the NNC to int6…

8e9ce08

…4 type

upadte: Fixing the type for the Availability zone for the NNC to uint…

e11ded8

… type

fix: Updated the config name to EnableHomeAZ

aa40cac

fix: Addressed all the PR comments

32c562e

nairashu force-pushed the asn/HomeAz branch from 8c76ca6 to 32c562e Compare December 23, 2024 22:35

ramiro-gamarra reviewed Dec 31, 2024

View reviewed changes

rbtr reviewed Jan 3, 2025

View reviewed changes

ramiro-gamarra self-requested a review January 3, 2025 23:29

github-actions bot added the stale Stale due to inactivity. label Jan 22, 2025

github-actions bot closed this Jan 29, 2025

github-actions bot deleted the asn/HomeAz branch January 29, 2025 00:01

feat: Creating NNC with HomeAz info in AKS-Swift Workflows when CNS starts up behind a configuration flag #3157

feat: Creating NNC with HomeAz info in AKS-Swift Workflows when CNS starts up behind a configuration flag #3157

Uh oh!

Conversation

nairashu commented Nov 19, 2024

Uh oh!

timraymond commented Nov 20, 2024

Uh oh!

ramiro-gamarra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nairashu Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timraymond Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 22, 2025

Uh oh!

github-actions bot commented Jan 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nairashu Dec 23, 2024 •

edited

Loading

timraymond Dec 23, 2024 •

edited

Loading