[network] Register the Node handler on Node, not Endpoint

[thc1006]

The network controller registers two watches on the same type:

		Watches(&invv1alpha1.Endpoint{}, &endpointEventHandler{client: mgr.GetClient()}).
		Watches(&invv1alpha1.Endpoint{}, &nodeEventHandler{client: mgr.GetClient()}).

nodeEventHandler.add opens with a type assertion and returns silently when it fails:

	cr, ok := obj.(*invv1alpha1.Node)
	if !ok {
		return
	}

Every event the second watch delivers is an Endpoint, so the assertion has never succeeded. No Node change has ever enqueued a Network.

What that costs

The reconciler reads Node state on each pass, through getProviderNodes, filtered on exactly the labels the handler matches:

	opts := []client.ListOption{
		client.MatchingLabels{
			invv1alpha1.NephioProviderKey: nokiaSRLProvider,
			invv1alpha1.NephioTopologyKey: topology,
		},
	}

So the data is consumed but never watched. A node joining or leaving a topology, or having its provider or topology label changed, is picked up only when something else — a Network write, an Endpoint event, the owned NetworkInstance/VLANIndex/Network resources — happens to trigger a reconcile. Between those, the rendered device config silently reflects a topology that no longer exists.

The fix

Register the handler against Node — and bind the pair by a type parameter, so the mistake cannot be made again:

		WatchesRawSource(source.Kind(mgr.GetCache(), &invv1alpha1.Node{},
			handler.TypedEventHandler[*invv1alpha1.Node, reconcile.Request](&nodeEventHandler{client: mgr.GetClient()}))).

source.Kind takes the watched object and the handler through one type parameter, so pointing this handler at Endpoint again stops compiling:

$ go build ./reconcilers/network/
reconcilers/network/reconciler.go:128:4: in call to source.Kind, type
handler.TypedEventHandler[*inv/v1alpha1.Node, reconcile.Request] ... does not match
inferred type handler.TypedEventHandler[*inv/v1alpha1.Endpoint, reconcile.Request]

The handler is typed on *invv1alpha1.Node throughout, so the runtime type assertion that silently swallowed every event is gone, and a failed List is logged instead of dropping the event without trace. Plus the RBAC marker, which was missing for a resource the controller both lists and now watches:

//+kubebuilder:rbac:groups=inv.nephio.org,resources=nodes,verbs=get;list;watch

On make manifests

I did not run it, because it cannot see this marker and there is nothing here for it to update. Three things say so:

1. The manifests target lives in operators/nephio-controller-manager and runs controller-gen ... paths="./...". That module contains exactly one package:

   $ cd operators/nephio-controller-manager && go list ./...
   github.com/nephio-project/nephio/operators/nephio-controller-manager

   The reconcilers are a separate module (controllers/pkg), pulled in as a blank import, so ./... never reaches them.

2. That operator has no config/ directory, so there is no generated role to refresh — running the target would create one from scratch.

3. No checked-in YAML in this repository contains the resources the existing markers declare (grep -rl networkinstances --include='*.yaml' . is empty), which confirms these markers have never been generated into anything here.

The effective ClusterRole is hand-maintained in nephio-project/catalog at nephio/core/nephio-operator/app/controller/clusterrole-network.yaml, and it already grants nodes:

- apiGroups:
  - inv.nephio.org
  resources:
  - links
  - nodes
  - endpoints

So the marker closes a gap between what the code declares and what it needs, and no deployed permission changes. Say the word if you would rather I run the target anyway and commit whatever it produces.

Proof manifests

$ go test ./reconcilers/network -count=1 -v
--- PASS: TestNodeEventHandler (0.11s)
    --- PASS: TestNodeEventHandler/a_deleted_Node_in_the_empty_topology_wakes_its_Network (0.00s)
    --- PASS: TestNodeEventHandler/a_deleted_Node_of_another_provider_wakes_nothing (0.00s)
    --- PASS: TestNodeEventHandler/a_deleted_Node_wakes_its_Network (0.00s)
    --- PASS: TestNodeEventHandler/an_explicitly_empty_topology_matches_an_empty-topology_Network (0.00s)
    --- PASS: TestNodeEventHandler/an_unchanged_update_enqueues_each_Network_once (0.00s)
    --- PASS: TestNodeEventHandler/every_matching_network,_namespace_and_name_intact (0.00s)
    --- PASS: TestNodeEventHandler/gaining_the_provider_label_wakes_the_Network_joined (0.00s)
    --- PASS: TestNodeEventHandler/losing_the_provider_label_still_wakes_the_Network_left_behind (0.00s)
    --- PASS: TestNodeEventHandler/matching_provider_and_topology (0.00s)
    --- PASS: TestNodeEventHandler/moving_between_topologies_wakes_the_Network_left_and_the_one_joined (0.00s)
    --- PASS: TestNodeEventHandler/moving_into_the_empty_topology_wakes_both_sides (0.00s)
    --- PASS: TestNodeEventHandler/moving_out_of_the_empty_topology_wakes_both_sides (0.00s)
    --- PASS: TestNodeEventHandler/no_labels (0.00s)
    --- PASS: TestNodeEventHandler/other_provider (0.00s)
    --- PASS: TestNodeEventHandler/other_topology (0.00s)
    --- PASS: TestNodeEventHandler/provider_but_the_topology_label_is_absent (0.08s)
ok  	github.com/nephio-project/nephio/controllers/pkg/reconcilers/network	0.479s

A passing table proves little by itself. Removing the handler's provider check fails other provider; removing its topology check fails other topology and every matching network; and dropping ObjectOld from the update path fails both migration cases, which is the guarantee those cases exist for.

$ make -C controllers/pkg unit
ok  	github.com/nephio-project/nephio/controllers/pkg/reconcilers/network	0.427s	coverage: 10.3% of statements
# 15 packages ok, 0 FAIL

$ make -C controllers/pkg lint
0 issues.

$ git diff --check

The package had no test file before this, so its coverage was 0%.

What the tests do and do not pin

The registration itself is guarded by the compiler rather than by a test, which is the stronger of the two: Builder.Watches takes client.Object and an untyped handler, so it accepts any pairing, and no unit test can inspect what a built controller ended up watching without standing up a manager and a cache.

The tests pin the handler's behaviour, and the update cases exercise why both sides of an update are enqueued:

• a Node moving from topology A to topology B wakes the Network it left and the one it joined;
• losing the provider label still wakes the Network left behind;
• gaining it wakes the Network joined;
• an unchanged update enqueues each Network once;
• a Node whose topology label is absent matches nothing;
• a Node whose topology label is present and empty matches a Network whose topology is empty, and moving in or out of that topology wakes both sides;
• a deleted Node wakes its Network, and a deleted Node of another provider wakes nothing.

Dropping ObjectOld from the handler fails the first two, so the migration guarantee is pinned rather than assumed. They also count cache scans, for the reason below.

Absent and empty are two different labels

The first version of this change read the topology with a plain map lookup and skipped anything that came back "". That conflates a label which is not there with one set to the empty string, and Kubernetes allows the second: Network.spec.topology is required but carries no minLength, so topology: "" is a value the schema accepts.

The reconciler already treats it as one. getProviderNodes lists with client.MatchingLabels, and that selector matches the empty value rather than the missing key:

selector topology="" matched 1: [empty-value]      # and not [label-absent]

So a Network with an empty topology was consuming Nodes that this handler would never have woken it for — the same silent staleness this PR exists to fix, one level down. The two-value lookup distinguishes them, and dropping either half fails a different case.

Why the update path lists once

The first version of this change called the handler body twice, once per side of the update. That scans the whole Network cache twice and calls queue.Add twice for each matching Network, and I described the second Add as being folded away by the workqueue. That is only true while nothing is consuming the queue — which is exactly the situation a unit test that drains after the call creates, and not one a handler can depend on:

q.Add(req)      // first side
item, _ := q.Get()   // a worker picks it up mid-handler
q.Add(req)      // second side: item is processing, so this only sets the dirty bit
q.Done(item)    // Done sees dirty and queues it again  ->  len 1

Run against client-go's queue, the no-consumer case ends at length 1 and the interleaved case also ends at length 1 — the second one having already handed an item out, so it is two reconciles rather than one.

So Update now collects the eligible topologies of both sides, scans the cache once, and enqueues the union. One Add per Network by construction, no reliance on queue timing, and an update between two Nodes of some other provider returns without listing at all. A second scan is invisible in the resulting requests while nothing is consuming, so the tests assert the scan count directly; restoring the per-Node call fails them.

Notes

• This deliberately does not touch the shared mutable reconciliation state (r.devices) that fix: resolve concurrent state corruption in NetworkController #1082 deals with. The two overlap mechanically — same reconciler, likely conflicting diffs — but neither is a prerequisite for the other today: nothing passes MaxConcurrentReconciles to this builder, so controller-runtime's default of 1 applies (controller.go:245) and there is one worker. If concurrency is ever enabled here, fix: resolve concurrent state corruption in NetworkController #1082's fix becomes necessary for correctness.
• watch_endpoint.go still uses the untyped shape and a runtime type assertion. Converting it the same way would make both registrations compiler-checked and let the two bodies be shared, but it is a change to code this fix does not otherwise touch. Happy to follow up.
• A Node update still scans the cached Network list once per event. A typed predicate that ignores status-only changes would avoid even that if Node churn grows, but it is a separate change.
• controllers/pkg has no manifests target of its own, and neither make unit nor make lint regenerates anything, so the working tree after all four commands is clean apart from the two files in this diff.

[nephio-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign johnbelamaric for approval by writing /assign @johnbelamaric in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nephio-prow]

Hi @thc1006. Thanks for your PR.

I'm waiting for a nephio-project member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.