Skip to content

Conversation

@lubronzhan
Copy link
Contributor

What this PR does / why we need it:
Right now it's hardcoded to whereabout's namespace, but NAD might be deployed in different namespace. If deployed in different namespace, NodeSlicePool will be garbage collected immediate after creation, and following logic in Whereabout will fail and report nil pointer.
Example in comment in this #518 (comment)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # #518

Special notes for your reviewer (optional):

@lubronzhan lubronzhan requested a review from dougbtv as a code owner May 19, 2025 18:59
@lubronzhan lubronzhan force-pushed the topic/lubron/fix-518 branch from a140fde to cfb62b3 Compare May 19, 2025 20:23
@lubronzhan
Copy link
Contributor Author

Ohk jsut found out can't set ownerRef to a resource in different namespace. So maybe this hardcoded namespace was intentional.

But NAD could be presented in any namespace in the doc https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/docs/configuration.md#namespace-isolation
Should we then allow user to create NodeSlicePool be defined in different namespace as well then

@lubronzhan
Copy link
Contributor Author

But is NodeSlicePool supposed to be 1 map to 1 cluster? Then NAD should be in the same namespace, then how does the pod in different namespace reference this NAD

@xagent003
Copy link
Contributor

Pod doesn't need to be in the same namespace as a NAD. The Pod just references the NAD via multus annotation /

We keep all NADs in one namespace, after all they generally attach to physical networks

@lubronzhan
Copy link
Contributor Author

IC, in that way, should we have a webhook to reject NAD created in other namespace. Otherwise it will just cause nil pointer exception mentioned in #518 (comment)
Or we should at least log a warning in the log?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants