Skip to content

Conversation

@pjohnst5
Copy link
Contributor

@pjohnst5 pjohnst5 commented Jul 31, 2025

Reason for Change:
MTPNC CRD changes for NUMA-Aware pod IB device programming

The partner team will call into CNS (still discussing how in #3825) but regardless, CNS will need to pass which IB devices to use for a pod, and DNC-RC will need to pass back to CNS the programming status of said devices

@pjohnst5 pjohnst5 changed the title MTPNC CRD Spec changes for NUMA-Aware Infiniband pods MTPNC CRD changes for NUMA-Aware Infiniband pods Jul 31, 2025
@pjohnst5 pjohnst5 force-pushed the pjohnst5/crd-changes branch from cd6d9b0 to ecfd8e8 Compare July 31, 2025 22:11
@pjohnst5 pjohnst5 force-pushed the pjohnst5/crd-changes branch from ecfd8e8 to 1ccb444 Compare July 31, 2025 22:13
@pjohnst5 pjohnst5 marked this pull request as ready for review July 31, 2025 22:16
@Copilot Copilot AI review requested due to automatic review settings July 31, 2025 22:16
@pjohnst5 pjohnst5 requested review from a team as code owners July 31, 2025 22:16
@pjohnst5 pjohnst5 requested review from rbtr and thatmattlong July 31, 2025 22:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for NUMA-aware InfiniBand pods by extending the MultitenantPodNetworkConfig CRD with InfiniBand-specific fields. The changes enable passing InfiniBand MAC addresses and tracking programming status for InfiniBand devices.

  • Added IBMACs field to store InfiniBand MAC addresses for pods
  • Added IBStatus field to track InfiniBand device programming status
  • Created new infiniband package with Status enum for device states

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
multitenancy.acn.azure.com_multitenantpodnetworkconfigs.yaml Updated CRD manifest to include IBMACs array and ibStatus integer field
zz_generated.deepcopy.go Added deep copy logic for new IBMACs slice field
multitenantpodnetworkconfig.go Added IBMACs and IBStatus fields to spec and interface structs
cns/types/infiniband/status.go New package defining InfiniBand programming status constants
Comments suppressed due to low confidence (1)

crd/multitenancy/api/v1alpha1/zz_generated.deepcopy.go:9

  • [nitpick] The import alias 'netx' is unconventional. Consider using a more standard alias like 'net' or removing the alias entirely since there's no naming conflict.
	netx "net"

@pjohnst5 pjohnst5 marked this pull request as draft August 1, 2025 18:25
@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 1, 2025

I would actually like to get #3825 ironed out before merging this

@pjohnst5 pjohnst5 marked this pull request as ready for review August 1, 2025 18:55
@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 1, 2025

Actually, this is ready for review
We are not dependent on #3825 for this, we will need to do this regardless how the CNS APIs are setup
I may later want to add an errorCode field into the interfaceInfo struct as well ( to be able to describe better what failed when programming a device ) but that is up to debate rn so this we know needs to be merged

@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 4, 2025

Hey @thatmattlong , are there any problems with associated outside package types in a CRD?
Mainly I'm talking about
multitenantpodnetworkconfig.go

IBMACAddresses []net.HardwareAddr `json:"IBMACAddresses,omitempty"`
...
IBStatus infiniband.Status `json:"ibStatus,omitempty"`

@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 4, 2025

Hey @thatmattlong , are there any problems with associated outside package types in a CRD?
Mainly I'm talking about

Yeah not best so I'll remove it

kmurudi
kmurudi previously approved these changes Aug 4, 2025
@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 4, 2025

/azp list

@azure-pipelines
Copy link

@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 4, 2025

/azp run Azure Container Networking PR

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 4, 2025

Hold on, may want to add a UID field somewhere in the MTPNC spec to differentiate between pods that have the same name
See #3825

@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 5, 2025

Doesn't seem we'll need to add pod UID, since the MTPNC will have an ownerref to the pod that it is created for (avoiding a toctou race)

@pjohnst5
Copy link
Contributor Author

pjohnst5 commented Aug 6, 2025

/azp run Azure Container Networking PR

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pjohnst5 pjohnst5 requested review from rbtr and removed request for thatmattlong August 6, 2025 20:31
@pjohnst5 pjohnst5 added this pull request to the merge queue Aug 7, 2025
Merged via the queue into master with commit 60df021 Aug 7, 2025
14 of 15 checks passed
@pjohnst5 pjohnst5 deleted the pjohnst5/crd-changes branch August 7, 2025 20:06
NihaNallappagari pushed a commit to NihaNallappagari/azure-container-networking that referenced this pull request Sep 4, 2025
* MTPNC CRD changes for NUMA-Aware pods with infiniband NICs

* More descriptive description haha

* IBMACAddresses

* Make mac string type to not break OpenAPI schema

* Status to MTPNC definition

* reference

* Rename statuses to unambiguate ambiguity

* make crd
sivakami-projects pushed a commit that referenced this pull request Oct 23, 2025
* MTPNC CRD changes for NUMA-Aware pods with infiniband NICs

* More descriptive description haha

* IBMACAddresses

* Make mac string type to not break OpenAPI schema

* Status to MTPNC definition

* reference

* Rename statuses to unambiguate ambiguity

* make crd
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.

3 participants