Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invert CNI result order for UDN #4770

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Conversation

trozet
Copy link
Contributor

@trozet trozet commented Oct 10, 2024

Makes crio happy

@trozet trozet requested a review from a team as a code owner October 10, 2024 21:00
@trozet trozet requested a review from ricky-rav October 10, 2024 21:00
@github-actions github-actions bot added the area/unit-testing Issues related to adding/updating unit tests label Oct 11, 2024
@trozet
Copy link
Contributor Author

trozet commented Oct 12, 2024

network segmentation e2es wont pass until we have the multus fix

@tssurya
Copy link
Contributor

tssurya commented Oct 14, 2024

@dougbtv @maiqueb do either of you know which Multus fix @trozet is talking about here? Trying to keep things moving...

@maiqueb
Copy link
Contributor

maiqueb commented Oct 14, 2024

@dougbtv @maiqueb do either of you know which Multus fix @trozet is talking about here? Trying to keep things moving...

I'll add these unit tests tomorrow.

Yes; this is the PR: k8snetworkplumbingwg/network-attachment-definition-client#73

We'll need afterwards to update the vendored library in multus.

Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

There's a typo in the variable name, plus, we got to keep the comments updated - or delete them.

Code-wise, looks good.

go-controller/pkg/cni/cni.go Outdated Show resolved Hide resolved
go-controller/pkg/cni/cni.go Outdated Show resolved Hide resolved
go-controller/pkg/cni/cni_test.go Show resolved Hide resolved
@tssurya
Copy link
Contributor

tssurya commented Oct 15, 2024

I can push for the reviewed comments do you have the unit tests ready? if so I can combine that commit in here as well

@maiqueb
Copy link
Contributor

maiqueb commented Oct 15, 2024

I can push for the reviewed comments do you have the unit tests ready? if so I can combine that commit in here as well

I spoke too soon; the unit tests were already adapted in this PR, Tim had done them.

There's no need to add more.

One extra thing @tssurya: you need to adjust the version of multus we are using: https://github.com/ovn-org/ovn-kubernetes/blob/93dbdf49f755472a62f64e6b5111d1801078de05/contrib/kind-common#L405

For that, Doug needs to merge the multus vendored status generation library, and create a new tag.

@dougbtv
Copy link

dougbtv commented Oct 15, 2024

New Multus tag @ https://github.com/k8snetworkplumbingwg/multus-cni/releases/tag/v4.1.3 thanks!

@tssurya
Copy link
Contributor

tssurya commented Oct 21, 2024

@dougbtv shall we also prepare the bump for downstream multus? not sure where we do that today...

@qinqon
Copy link
Contributor

qinqon commented Oct 21, 2024

I see that all the comments from @maiqueb are addressed so this lookgs good to me.

@tssurya
Copy link
Contributor

tssurya commented Oct 21, 2024

https://github.com/ovn-org/ovn-kubernetes/actions/runs/11438299020/job/31820962325?pr=4770 is likely due to #4783 but don't have time to cross check, let's see if CI turns green

@tssurya tssurya merged commit 75d2e6d into ovn-kubernetes:master Oct 21, 2024
38 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/e2e-testing area/unit-testing Issues related to adding/updating unit tests feature/user-defined-network-segmentation All PRs related to User defined network segmentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants