networks: Add Owner annotation to NetAttachDefs #897

zeeke · 2025-06-06T12:54:10Z

depends on:

Support namespaced {Sriov,SriovIB,OVS}Networks #894

NetworkattachmentDefinitions are usually created in namespaces that are different
than the {Sriov,SriovIb,OVS}Networks, so the OwnerReference object field can't be
used to endorse the ownership reference.

Add a sriovnetwork.openshift.io/owner annotation to put on the NetAttachDef
to avoid multiple {Sriov,SriovIb,OVS}Network objects does not override changes.

github-actions · 2025-06-06T12:54:23Z

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2025-06-09T09:35:08Z

Pull Request Test Coverage Report for Build 17434124856

Details

20 of 24 (83.33%) changed or added relevant lines in 2 files are covered.
6 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.2%) to 62.008%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
api/v1/helper.go	7	11	63.64%

Files with Coverage Reduction	New Missed Lines	%
controllers/drain_controller_helper.go	1	67.43%
controllers/generic_network_controller.go	5	78.35%

Totals
Change from base Build 17261676753:	0.2%
Covered Lines:	8691
Relevant Lines:	14016

💛 - Coveralls

api/v1/helper.go

pkg/consts/constants.go

controllers/generic_network_controller.go

controllers/generic_network_controller_test.go

api/v1/helper.go

adrianchiris · 2025-07-14T10:28:07Z

controllers/generic_network_controller.go

-		instance.SetFinalizers(append(instanceFinalizers, sriovnetworkv1.NETATTDEFFINALIZERNAME))
-		if err := r.Update(ctx, instance); err != nil {
-			return err
+		obj, namespace, name, err := sriovnetworkv1.StringToOwnerRef(owner)


should we limit the cleanup to specific object kind ? according to the object returned in the controller.GetObject() ?

good idea. The problem is that I didn't find a nice way to check that two pointers have the same underlying type.
The only one I found is

func isType(a, b interface{}) bool { return fmt.Sprintf("%T", a) == fmt.Sprintf("%T", b) }

https://stackoverflow.com/questions/34820469/can-i-compare-variable-types-with-type-in-golang

which is not really nice :(

adrianchiris · 2025-08-26T17:43:45Z

@zeeke once #894 is merged, lets rebase this one so we can do final round of reviews.

Copilot

Pull Request Overview

This PR implements owner annotation functionality for NetworkAttachmentDefinitions to prevent conflicts when network objects exist in different namespaces. The change replaces the previous finalizer-based ownership approach with an annotation-based system that can work across namespace boundaries.

Adds owner annotation to NetworkAttachmentDefinitions to track ownership relationships
Implements garbage collection for orphaned NetworkAttachmentDefinitions
Removes finalizer-based ownership system and migrates existing resources

Reviewed Changes

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

Show a summary per file

File	Description
pkg/consts/constants.go	Adds new owner annotation constant
controllers/suite_test.go	Updates test setup to skip name validation
controllers/sriovnetwork_controller_test.go	Adds test for finalizer removal during migration
controllers/generic_network_controller_test.go	Comprehensive tests for owner annotation functionality
controllers/generic_network_controller.go	Main controller logic changes for annotation-based ownership
bindata/manifests/cni-config/	Updates templates to include owner annotations
api/v1/testdata/	Updates test expectations for owner annotations
api/v1/helper_test.go	Improves test assertions and adds metadata
api/v1/helper.go	Implements owner reference string conversion functions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

controllers/generic_network_controller.go

api/v1/helper.go

adrianchiris · 2025-09-02T16:58:44Z

controllers/generic_network_controller.go

 			// Owned objects are automatically garbage collected. For additional cleanup logic use finalizers.
 			// Return and don't requeue
-			return reconcile.Result{}, nil
+			return reconcile.Result{}, r.garbageCollect(ctx)


for network CR instances that are in the operator namespace and point to a different namespace, are we relying on garbageCollect to clean them ?

i think the approach is problematic, since we are removing finalizers at L97, once the object is deleted, it will be removed from k8s API. if for some reason garbage collect fails and controller is restarted we will never retry.

then we rely on the next delete event to trigger garbage collect (which might take a while or never come ?)

i wonder if we should just keep finalizers on network CR instances or at least for those in the operator namespace then we explicitly delete what we need.

sounds good. The first commit is about adding the owner-ref annotation and ensure 2 NetworkObject are not clashing on the same NetAttachDef. The second commit is about using the owner-ref annotation on the netattachdef instead of networkobject's finalizers, which I was a sort of bonus change.

I'm dropping the second commit to focus this PR on having the owner-ref annotation. We can discuss about dropping the finalizers later, if needed

NetworkattachmentDefiitions are usually created in namespaces that are different than the Sriov{Ib,OVS}Networks, so the OwnerReference object field can't be used to endorse the ownership reference. Add a `sriovnetwork.openshift.io/owner` annotation to put on the NetAttachDef to avoid multiple {Sriov,SriovIb,OVS}Network objects does not override changes. Signed-off-by: Andrea Panattoni <[email protected]>

SchSeba

LGTM

just a small nit, not blocking

SchSeba · 2025-07-06T13:15:19Z

api/v1/helper.go

+		return &OVSNetwork{}, namespace, name, nil
+	}
+
+	return nil, "", "", fmt.Errorf("unknown ownerRef groupKind %s", groupKind)


just nit we can use default here instead of the return right?

this commit has been dropped:
#897 (comment)

it was about replacing finalizers delete flow with another based on garbage collecting. Now this PR only avoids multiple network objects overrides the same netAttachDef

SchSeba · 2025-09-09T07:09:16Z

controllers/generic_network_controller.go

+
+		// Note for the future: the `foundOwner != ""` condition can be removed to make the operator not touching the NetworkAttachmentDefinition created
+		// by the user.
+		if foundOwner != "" && foundOwner != expectedOwner {


just a quick question if we have !equality.Semantic.DeepEqual(found.GetAnnotations(), netAttDef.GetAnnotations() below do we still need this validation here?

yes, we do:

in case we have:

kind: NetworkAttachmentDefinition metadata: name: foo namespace: foo-ns annotations: owner-ref: SriovNetwork/sriov-operator-ns/foo spec: <sriov cni config>

which comes from an SriovNetwork in the sriov-operator-ns namespace

when reconciling another network object:

kind: SriovNetwork metadata: name: foo namespace: foo-ns spec: ...

the generated net-attach-def has owner-ref: SriovNetwork/foo-ns/foo, which would make the condition !equality.Semantic.DeepEqual(found.GetAnnotations(), netAttDef.GetAnnotations() true and the netAttachDef woudl be overridden.

The policy, as of now, is the first reconciler networkobject is the owner

SchSeba · 2025-09-10T13:03:50Z

Great work!

Merging this one

zeeke force-pushed the us/owner-references branch 6 times, most recently from e17f485 to 1a0d77d Compare June 9, 2025 09:23

zeeke mentioned this pull request Jun 13, 2025

Support namespaced {Sriov,SriovIB,OVS}Networks #894

Merged

adrianchiris reviewed Jul 3, 2025

View reviewed changes

zeeke force-pushed the us/owner-references branch from 1a0d77d to 25d4188 Compare July 9, 2025 12:53

adrianchiris reviewed Jul 14, 2025

View reviewed changes

zeeke force-pushed the us/owner-references branch 2 times, most recently from df7f95b to 525a0e4 Compare August 28, 2025 07:41

zeeke requested review from adrianchiris and e0ne August 29, 2025 14:42

SchSeba requested a review from Copilot September 2, 2025 13:07

Copilot AI reviewed Sep 2, 2025

View reviewed changes

controllers/generic_network_controller.go Outdated Show resolved Hide resolved

controllers/generic_network_controller.go Outdated Show resolved Hide resolved

api/v1/helper.go Outdated Show resolved Hide resolved

zeeke force-pushed the us/owner-references branch from 525a0e4 to 915cb47 Compare September 2, 2025 13:45

adrianchiris reviewed Sep 2, 2025

View reviewed changes

zeeke force-pushed the us/owner-references branch from 915cb47 to f4307b0 Compare September 3, 2025 12:55

SchSeba approved these changes Sep 9, 2025

View reviewed changes

adrianchiris approved these changes Sep 10, 2025

View reviewed changes

SchSeba merged commit 2f1ce14 into k8snetworkplumbingwg:master Sep 10, 2025
14 checks passed

networks: Add Owner annotation to NetAttachDefs #897

networks: Add Owner annotation to NetAttachDefs #897

Uh oh!

Conversation

zeeke commented Jun 6, 2025

Uh oh!

github-actions bot commented Jun 6, 2025

Uh oh!

coveralls commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17434124856

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrianchiris Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

zeeke Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

adrianchiris commented Aug 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrianchiris Sep 2, 2025 • 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!

zeeke Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

SchSeba left a comment

Choose a reason for hiding this comment

Uh oh!

SchSeba Jul 6, 2025

Choose a reason for hiding this comment

Uh oh!

zeeke Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

SchSeba Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

zeeke Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

SchSeba commented Sep 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jun 9, 2025 •

edited

Loading

adrianchiris Sep 2, 2025 •

edited

Loading