fix: init NNC from reconciler instead of directly #4199

rbtr · 2026-01-22T00:30:48Z

Reason for Change:
There's no clear reason that CNS initializes with an NNC that it gets out of band from the typical NNC reconciler flow.
This separate path for fetching the NNC leads to bugs such as ingesting an NNC for a previous Node of the same name and state corruption once the reconciler (which is smart enough to wait for the actual NNC for its Node) starts.
Additionally, any caching, filtering, or other selection done to NNCs for efficiency is not applied or must be duplicated.

Instead, initialization can be run from the first NNC that the reconciler is pushed. This consolidates all CNS NNC reads to a single location where list-watch optimizations and filtering can be centralized.

Copilot

Pull request overview

This pull request refactors the CNS (Container Networking Service) initialization to use the NNC (NodeNetworkConfig) reconciler flow instead of a separate out-of-band initialization path. This consolidates NNC reads to a single location, preventing bugs from ingesting stale NNCs and ensuring caching/filtering optimizations are consistently applied.

Changes:

Removed direct NNC client initialization and replaced it with an initializer callback passed to the reconciler
Modified the reconciler to invoke the initializer function on the first reconcile
Updated function signatures to accept the initializer as a parameter

Reviewed changes

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

File	Description
cns/service/main.go	Removed direct NNC client creation and retry logic; replaced with initialization closure passed to reconciler
cns/kubecontroller/nodenetworkconfig/reconciler.go	Added initializer callback field and invocation logic in Reconcile method
cns/kubecontroller/nodenetworkconfig/reconciler_test.go	Updated test calls to include no-op initializer function parameter

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T00:38:11Z

cns/service/main.go

+		if err := reconcileInitialCNSState(nnc, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2); err != nil {
+			return err
+		}
+		hasNNCInitialized.Set(2)


The metric value changed from 1 to 2, but there's no explanation for this change. The metric name and documentation suggest it's a boolean indicator (0 or 1), so setting it to 2 is unclear. Please clarify the intent or revert to the original value of 1.

Suggested change

hasNNCInitialized.Set(2)

hasNNCInitialized.Set(1)

Copilot · 2026-01-22T00:38:12Z

cns/service/main.go

-	logger.Printf("reconciled initial CNS state after %d attempts", attempt)
-	hasNNCInitialized.Set(1)
+		logger.Printf("Reconciling initial CNS state")
+		if err := reconcileInitialCNSState(nnc, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2); err != nil {


The nncInitFailure metric counter is no longer incremented when initialization fails. In the previous code, this counter was incremented in the retry loop. Consider incrementing this counter when reconcileInitialCNSState returns an error to maintain observability of initialization failures.

Suggested change

if err := reconcileInitialCNSState(nnc, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2); err != nil {

if err := reconcileInitialCNSState(nnc, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2); err != nil {

metrics.NNCInitFailure.Inc()

Copilot · 2026-01-22T00:38:12Z

cns/kubecontroller/nodenetworkconfig/reconciler.go

 // Provided nncListeners are passed the NNC after the Reconcile preprocesses it. Note: order matters! The
 // passed Listeners are notified in the order provided.
-func NewReconciler(cnscli cnsClient, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, isSwiftV2 bool) *Reconciler {
+func NewReconciler(cnscli cnsClient, initializer nodenetworkconfigSink,  ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, isSwiftV2 bool) *Reconciler {


Extra space between parameters in the function signature.

Suggested change

func NewReconciler(cnscli cnsClient, initializer nodenetworkconfigSink, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, isSwiftV2 bool) *Reconciler {

func NewReconciler(cnscli cnsClient, initializer nodenetworkconfigSink, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, isSwiftV2 bool) *Reconciler {

Copilot · 2026-01-22T00:38:12Z

cns/service/main.go

-		if err != nil {
-			logger.Errorf("failed to reconcile initial CNS state, attempt: %d err: %v", attempt, err)
-			nncInitFailure.Inc()
+


Trailing whitespace on empty line.

Copilot · 2026-01-22T00:38:12Z

cns/service/main.go

+		return nil
+	}
+
+


Trailing whitespace on empty line.

Copilot · 2026-01-22T00:38:13Z

cns/kubecontroller/nodenetworkconfig/reconciler.go

+	// call initFunc on first reconcile
+	if err := r.initializer(nnc); err != nil {
+		logger.Errorf("[cns-rc] initializer failed during reconcile: %v", err)
+		return reconcile.Result{}, errors.Wrap(err, "initializer failed during reconcile")
+	}


The new initialization logic using the initializer callback is not covered by tests. The existing tests pass a no-op function (func(*v1alpha.NodeNetworkConfig) error { return nil }). Consider adding tests that verify:

The initializer is called exactly once on the first reconcile

The initializer is not called on subsequent reconciles

Error handling when the initializer returns an error

Copilot · 2026-01-22T00:38:13Z

cns/service/main.go

+	hasInitialized := false
+	initFunc := func(nnc *v1alpha.NodeNetworkConfig) error {
+		if hasInitialized {
+			return nil
 		}
-		return errors.Wrap(err, "failed to initialize CNS state")
-	}, retry.Context(ctx), retry.Delay(initCNSInitalDelay), retry.MaxDelay(time.Minute), retry.UntilSucceeded())
-	logger.Printf("reconciled initial CNS state after %d attempts", attempt)
-	hasNNCInitialized.Set(1)
+		logger.Printf("Reconciling initial CNS state")
+		if err := reconcileInitialCNSState(nnc, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2); err != nil {
+			return err
+		}
+		hasNNCInitialized.Set(2)
+		hasInitialized = true
+		return nil


The hasInitialized flag is not protected by a mutex, which could lead to a race condition if the reconciler's Reconcile method is called concurrently. Controller-runtime can invoke reconcile handlers concurrently for different requests. Consider using a sync.Once pattern similar to how the reconciler's started field is managed, or protect the flag with a mutex.

rbtr · 2026-01-22T16:57:46Z

/azp run Azure Container Networking PR

azure-pipelines · 2026-01-22T16:57:59Z

Azure Pipelines successfully started running 1 pipeline(s).

rbtr · 2026-01-23T19:38:29Z

/azp run Azure Container Networking PR

azure-pipelines · 2026-01-23T19:38:41Z

Azure Pipelines successfully started running 1 pipeline(s).

timraymond · 2026-01-23T19:55:43Z

cns/kubecontroller/nodenetworkconfig/reconciler.go

+
 type nodeNetworkConfigListener interface {
-	Update(*v1alpha.NodeNetworkConfig) error
+	Update(*v1alpha.NodeNetworkConfig) error // phasing this out in favor of the sink


Maybe add a // Deprecated: use nodenetworkconfigSink?

timraymond · 2026-01-23T19:59:47Z

cns/service/main.go

-		err = reconcileInitialCNSState(ctx, directscopedcli, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2)
-		if err != nil {
-			logger.Errorf("failed to reconcile initial CNS state, attempt: %d err: %v", attempt, err)
-			nncInitFailure.Inc()


Do we still want to keep this counter around?

It will be implicitly counted in failed nnc reconciles

timraymond · 2026-01-23T20:00:54Z

cns/kubecontroller/nodenetworkconfig/reconciler.go

+			logger.Errorf("[cns-rc] initializer failed during reconcile: %v", err)
+			return reconcile.Result{}, errors.Wrap(err, "initializer failed during reconcile")
+		}
+		r.initializer = nil


I think it's the same behavior for this to be wedged until initialization succeeds, but do we want to eventually circuit break? It seems like that was the original intention of the existing logic as I read the comment above it... whether that was true in practice may be dubious though because of swallowing the retry error and the UntilSucceeded().

You read the prior intent correctly - we wanted to retry for a while and eventually fail out as a signal to the user that something was wrong. However, the retry was long enough that CNS wouldn't exit frequently enough to end up in CrashLoopBackoff and instead the Pod would just clock 5-6 restarts per hour.
Eventually someone might notice, but in practice this effectively was a retry-forever.

By tying this in to the Reconcile loop, we will be able to lean on the ctrlruntime machinery - it automatically does retry backoff and exposes metrics for failed reconciles which we can collect and use as a signal.

nddq

minor comments, but the changes make sense

nddq · 2026-01-23T20:33:05Z

cns/kubecontroller/nodenetworkconfig/reconciler.go

+	// call initFunc on first reconcile and never again
+	if r.initializer != nil {
+		if err := r.initializer(nnc); err != nil {
+			logger.Errorf("[cns-rc] initializer failed during reconcile: %v", err)


cns-rc? that's new 🙂

nddq · 2026-01-23T20:35:20Z

cns/kubecontroller/nodenetworkconfig/reconciler.go

 // Reconcile is called on CRD status changes
 func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
-	listenersToNotify := []nodeNetworkConfigListener{}
+	listenersToNotify := []nodenetworkconfigSink{}


were there going to be any other listener other than the ipam pool monitor?

rbtr requested a review from a team as a code owner January 22, 2026 00:30

rbtr requested review from Copilot and timraymond January 22, 2026 00:30

Copilot started reviewing on behalf of rbtr January 22, 2026 00:31 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

rbtr requested review from nddq and thatmattlong January 22, 2026 17:05

rbtr added 2 commits January 22, 2026 19:26

fix: init NNC from reconciler instead of directly

de2b3ac

add tests

14b9b41

rbtr force-pushed the fix/init-from-reconcile branch from cfab669 to 14b9b41 Compare January 23, 2026 17:48

rbtr added bug cns Related to CNS. labels Jan 23, 2026

rbtr added the fix Fixes something. label Jan 23, 2026

timraymond reviewed Jan 23, 2026

View reviewed changes

nddq reviewed Jan 23, 2026

View reviewed changes

	if err := reconcileInitialCNSState(nnc, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2); err != nil {
	if err := reconcileInitialCNSState(nnc, httpRestServiceImplementation, podInfoByIPProvider, cnsconfig.EnableSwiftV2); err != nil {
	metrics.NNCInitFailure.Inc()

	func NewReconciler(cnscli cnsClient, initializer nodenetworkconfigSink, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, isSwiftV2 bool) *Reconciler {
	func NewReconciler(cnscli cnsClient, initializer nodenetworkconfigSink, ipampoolmonitorcli nodeNetworkConfigListener, nodeIP string, isSwiftV2 bool) *Reconciler {

fix: init NNC from reconciler instead of directly #4199

Are you sure you want to change the base?

fix: init NNC from reconciler instead of directly #4199

Conversation

rbtr commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

rbtr commented Jan 22, 2026

Uh oh!

azure-pipelines bot commented Jan 22, 2026

Uh oh!

rbtr commented Jan 23, 2026

Uh oh!

azure-pipelines bot commented Jan 23, 2026

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!

nddq 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rbtr commented Jan 22, 2026 •

edited

Loading