Skip to content

Conversation

@aggarwal0009
Copy link
Contributor

Reason for Change:

New CRD PodNetworkInstance added to enable VNET multitenancy

PNIs represent optional requirements, or behavior configurations for how we setup the pod networking. They should map 1:1 and follow the lifetime of a customer workload.

Issue Fixed:

Requirements:

Notes:

@aggarwal0009 aggarwal0009 force-pushed the ankaggar/ankaggar/PNI-CRD branch from 1324f1c to 9f00eaf Compare August 10, 2023 22:10
@aggarwal0009 aggarwal0009 self-assigned this Aug 10, 2023
miguelgoms
miguelgoms previously approved these changes Aug 10, 2023
Signed-off-by: aggarwal0009 <127549148+aggarwal0009@users.noreply.github.com>
Signed-off-by: aggarwal0009 <127549148+aggarwal0009@users.noreply.github.com>
@aggarwal0009 aggarwal0009 requested a review from a team as a code owner August 14, 2023 16:20
@aggarwal0009 aggarwal0009 requested a review from rbtr August 14, 2023 16:20
@aggarwal0009 aggarwal0009 force-pushed the ankaggar/ankaggar/PNI-CRD branch from 7a4cf67 to 4cfe869 Compare August 14, 2023 16:22
// PodNetworkInstanceSpec defines the desired state of PodNetworkInstance
type PodNetworkInstanceSpec struct {
// +kubebuilder:default=0
// +kubebuilder:validation:Optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

not exactly sure what this annotation means but a PodNetwork is not an optional field of a PodNetworkInstance

Copy link
Contributor Author

@aggarwal0009 aggarwal0009 Aug 14, 2023

Choose a reason for hiding this comment

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

With omitempty in place, this annotation has no effect.. I would also need to drop omit empty from the field for it to be marked required in CRD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I dropped omitempty from podnetwork and it is now marked req in generated CRD.


// PodNetworkInstanceStatus defines the observed state of PodNetworkInstance
type PodNetworkInstanceStatus struct {
// +kubebuilder:validation:Optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, I wouldn't mark this optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed but it has no effect. Do I need to drop omitempty from PodIPAddresses field here? They can be empty initially right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm yeah let's keep it like you have it for now

miguelgoms
miguelgoms previously approved these changes Aug 14, 2023
thatmattlong
thatmattlong previously approved these changes Aug 14, 2023
Signed-off-by: aggarwal0009 <127549148+aggarwal0009@users.noreply.github.com>
@aggarwal0009 aggarwal0009 dismissed stale reviews from thatmattlong and miguelgoms via 351ea4d August 15, 2023 15:32
@aggarwal0009 aggarwal0009 requested a review from rbtr August 15, 2023 15:33
@neaggarwMS
Copy link
Member

Merging as Dual stack E2E is flaky. Pinging Ramiro to get that fixed.

@neaggarwMS neaggarwMS merged commit 906313d into master Aug 16, 2023
@neaggarwMS neaggarwMS deleted the ankaggar/ankaggar/PNI-CRD branch August 16, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants