Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,37 @@ issues:
max-issues-per-linter: 0
new-from-rev: origin/master
linters:
presets:
- bugs
- error
- format
- performance
- unused
presets:
- bugs
- error
- format
- performance
- unused
disable:
- maligned
- scopelint
- maligned
- scopelint
- gomnd
enable:
- exportloopref
- goconst
- gocritic
- gocyclo
- gofmt
- gomnd
- goprintffuncname
- gosimple
- lll
- misspell
- nakedret
- promlinter
- revive
- exportloopref
- goconst
- gocritic
- gocyclo
- gofmt
- goprintffuncname
- gosimple
- lll
- misspell
- nakedret
- promlinter
- revive
linters-settings:
gocritic:
enabled-tags:
- "diagnostic"
- "style"
- "performance"
- "diagnostic"
- "style"
- "performance"
disabled-checks:
- "hugeParam"
- "hugeParam"
govet:
check-shadowing: true
lll:
Expand Down
2 changes: 1 addition & 1 deletion .pipelines/mdnc/azure-cns-cni-1.4.39.1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ metadata:
rules:
- apiGroups: ["acn.azure.com"]
resources: ["nodenetworkconfigs"]
verbs: ["get", "list", "watch", "patch", "update"]
verbs: ["create", "delete", "get", "list", "watch", "patch", "update"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think we'll need delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
Expand Down
2 changes: 1 addition & 1 deletion .pipelines/mdnc/azure-cns-cni-1.5.28.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ metadata:
rules:
- apiGroups: ["acn.azure.com"]
resources: ["nodenetworkconfigs"]
verbs: ["get", "list", "watch", "patch", "update"]
verbs: ["create", "delete", "get", "list", "watch", "patch", "update"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
Expand Down
2 changes: 1 addition & 1 deletion .pipelines/mdnc/azure-cns-cni-1.5.4.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ metadata:
rules:
- apiGroups: ["acn.azure.com"]
resources: ["nodenetworkconfigs"]
verbs: ["get", "list", "watch", "patch", "update"]
verbs: ["create", "delete", "get", "list", "watch", "patch", "update"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
Expand Down
2 changes: 1 addition & 1 deletion .pipelines/mdnc/azure-cns-cni.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ metadata:
rules:
- apiGroups: ["acn.azure.com"]
resources: ["nodenetworkconfigs"]
verbs: ["get", "list", "watch", "patch", "update"]
verbs: ["create", "delete", "get", "list", "watch", "patch", "update"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
Expand Down
2 changes: 1 addition & 1 deletion cns/azure-cns.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ metadata:
rules:
- apiGroups: ["acn.azure.com"]
resources: ["nodenetworkconfigs"]
verbs: ["get", "list", "watch", "patch", "update"]
verbs: ["create", "delete", "get", "list", "watch", "patch", "update"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
Expand Down
1 change: 1 addition & 0 deletions cns/configuration/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type CNSConfig struct {
EnableSubnetScarcity bool
EnableSwiftV2 bool
InitializeFromCNI bool
EnableHomeAZ bool
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

KeyVaultSettings KeyVaultSettings
MSISettings MSISettings
ManageEndpointState bool
Expand Down
2 changes: 1 addition & 1 deletion cns/restserver/homeazmonitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func (h *HomeAzMonitor) Populate(ctx context.Context) {
return

default:
returnMessage := fmt.Sprintf("[HomeAzMonitor] failed with StatusCode: %d", apiError.StatusCode())
returnMessage := fmt.Sprintf("[HomeAzMonitor] failed with StatusCode: %d and error %v", apiError.StatusCode(), err)
returnCode := types.UnexpectedError
h.update(returnCode, returnMessage, cns.HomeAzResponse{IsSupported: true})
return
Expand Down
12 changes: 12 additions & 0 deletions cns/restserver/internalapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -633,3 +633,15 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req *cns.
func (service *HTTPRestService) SetVFForAccelnetNICs() error {
return service.setVFForAccelnetNICs()
}

// 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
}
Comment on lines +637 to +647
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

80 changes: 77 additions & 3 deletions cns/service/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -668,9 +668,9 @@ func main() {
}

homeAzMonitor := restserver.NewHomeAzMonitor(nmaClient, time.Duration(cnsconfig.AZRSettings.PopulateHomeAzCacheRetryIntervalSecs)*time.Second)
// homeAz monitor is only required when there is a direct channel between DNC and CNS.
// This will prevent the monitor from unnecessarily calling NMA APIs for other scenarios such as AKS-swift, swiftv2
if cnsconfig.ChannelMode == cns.Direct {
// homeAz monitor is required when there is a direct channel between DNC and CNS OR when homeAz feature is enabled in CNS for AKS-Swift
// This will prevent the monitor from unnecessarily calling NMA APIs for other scenarios such as AKS-swift, swiftv2 when disabled.
if cnsconfig.ChannelMode == cns.Direct || cnsconfig.EnableHomeAZ {
homeAzMonitor.Start()
defer homeAzMonitor.Stop()
}
Expand Down Expand Up @@ -1303,6 +1303,68 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn
// TODO(rbtr): nodename and namespace should be in the cns config
directscopedcli := nncctrl.NewScopedClient(directnnccli, types.NamespacedName{Namespace: "kube-system", Name: nodeName})

// Create the base NNC CRD if HomeAz is enabled
if cnsconfig.EnableHomeAZ {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

i would like to see NNC creation decoupled completely from HomeAz

var homeAzResponse cns.GetHomeAzResponse
if homeAzResponse, err = httpRestServiceImplementation.GetHomeAz(ctx); err != nil {
return errors.Wrap(err, "failed to get HomeAz") // error out so that CNS restarts.
}
az := homeAzResponse.HomeAzResponse.HomeAz
logger.Printf("[Azure CNS] HomeAz: %d", az)
// Create Node Network Config CRD and update the Home Az field with the cache value from the HomeAz Monitor
var nnc *v1alpha.NodeNetworkConfig
err = retry.Do(func() error {
if nnc, err = directnnccli.Get(ctx, types.NamespacedName{Namespace: "kube-system", Name: nodeName}); err != nil {
return errors.Wrap(err, "[Azure CNS] failed to get existing NNC")
}
return nil
}, retry.Delay(initCNSInitalDelay), retry.Attempts(5))

newNNC := createBaseNNC(node)
if err != nil {
logger.Printf("[Azure CNS] Creating new base NNC with Az %d", az)
newNNC.Spec.AvailabilityZone = az
nncErr := retry.Do(func() error {
if err = directcli.Create(ctx, newNNC); err != nil {
return errors.Wrap(err, "failed to create base NNC with HomeAz")
}
return nil
}, retry.Delay(initCNSInitalDelay), retry.Attempts(5))
if nncErr != nil {
return errors.Wrap(nncErr, "[Azure CNS] failed to create base NNC with HomeAz")
}
}

if err == nil { // NNC exists, patch it with new HomeAz
logger.Printf("[Azure CNS] Patching existing NNC with new Spec with HomeAz %d", az)
newNNC.ObjectMeta.ResourceVersion = nnc.ObjectMeta.ResourceVersion
newNNC.Spec.AvailabilityZone = az
newNNC.Spec.RequestedIPCount = nnc.Spec.RequestedIPCount
newNNC.Spec.IPsNotInUse = nnc.Spec.IPsNotInUse
newNNC.Status = nnc.Status
newNNC.UID = nnc.UID
newNNC.Name = nnc.Name
newNNC.Namespace = nnc.Namespace
newNNC.Annotations = nnc.Annotations
newNNC.Labels = nnc.Labels
newNNC.Finalizers = nnc.Finalizers
newNNC.OwnerReferences = nnc.OwnerReferences
newNNC.CreationTimestamp = nnc.CreationTimestamp
newNNC.DeletionTimestamp = nnc.DeletionTimestamp
nncErr := retry.Do(func() error {
patchErr := directcli.Update(ctx, newNNC, &client.UpdateOptions{})
if patchErr != nil {
return errors.Wrap(patchErr, "failed to patch NNC")
}
return nil
}, retry.Delay(initCNSInitalDelay), retry.Attempts(5))
if nncErr != nil {
return errors.Wrap(nncErr, "[AzureCNS] failed to patch NNC with Home Az")
}
}
logger.Printf("[Azure CNS] Updated HomeAz in NNC %v", newNNC)
}

logger.Printf("Reconciling initial CNS state")
// apiserver nnc might not be registered or api server might be down and crashloop backof puts us outside of 5-10 minutes we have for
// aks addons to come up so retry a bit more aggresively here.
Expand Down Expand Up @@ -1513,6 +1575,18 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn
return nil
}

func createBaseNNC(node *corev1.Node) *v1alpha.NodeNetworkConfig {
return &v1alpha.NodeNetworkConfig{ObjectMeta: metav1.ObjectMeta{
Annotations: make(map[string]string),
Labels: map[string]string{
"managed": "true",
"owner": node.Name,
},
Name: node.Name,
Namespace: "kube-system",
}}
}

// getPodInfoByIPProvider returns a PodInfoByIPProvider that reads endpoint state from the configured source
func getPodInfoByIPProvider(
ctx context.Context,
Expand Down
3 changes: 2 additions & 1 deletion crd/nodenetworkconfig/api/v1alpha/nodenetworkconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@ type NodeNetworkConfigSpec struct {
RequestedIPCount int64 `json:"requestedIPCount"`
IPsNotInUse []string `json:"ipsNotInUse,omitempty"`
// AvailabilityZone contains the Azure availability zone for the virtual machine where network containers are placed.
// NMA returns an int value for the availability zone.
// +kubebuilder:validation:Optional
AvailabilityZone string `json:"availabilityZone,omitempty"`
AvailabilityZone uint `json:"availabilityZone,omitempty"`
}

// Status indicates the NNC reconcile status
Expand Down
2 changes: 1 addition & 1 deletion test/integration/manifests/cilium/cns-write-ovly.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ metadata:
rules:
- apiGroups: ["acn.azure.com"]
resources: ["nodenetworkconfigs"]
verbs: ["get", "list", "watch", "patch", "update"]
verbs: ["create", "delete", "get", "list", "watch", "patch", "update"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
Expand Down
Loading