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

multus: Use thin version #281

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Conversation

oshoval
Copy link
Member

@oshoval oshoval commented Sep 28, 2023

What this PR does / why we need it:
Calico officially supports only multus thin,
thick fails to create pods [1]
We don't need here thick so lets use thin meanwhile (as cnao does).

[1] https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/k8snetworkplumbingwg_ovs-cni/279/pull-e2e-ovs-cni/1706970379018309632

Special notes for your reviewer:
It seems we are using https://raw.githubusercontent.com/k8snetworkplumbingwg/multus-cni/v4.0.1/deployments/multus-daemonset-thick.yml
which has image: ghcr.io/k8snetworkplumbingwg/multus-cni:snapshot-thick
same tag is used also on 4.0.2 and such, hence it might started to break suddenly when multus was changed.
Thin also have this problem (common tag between versions), so we should freeze by hash or so in a follow PR,
as CNAO does.

Release note:

None

Calico support officialy only multus thin,
thick fails to create pods.
We don't need here thick so lets use thin meanwhile.

Signed-off-by: Or Shoval <[email protected]>
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 28, 2023
@oshoval
Copy link
Member Author

oshoval commented Sep 28, 2023

/cc @phoracek
this seems to fix the problem at least locally with some more changes (i.e k8s-1.28, latest kubevirtci, calico 3.26.1)
lets see on CI, with minimal changes

Talked with Calico people, they only test with multus thin
they also say that k8s 1.25+ doesnt officially support the old calico 3.18 we are using, so worth that we update it on kubevirtci (the new calico might do problems with multus thick, but even the current calico 3.18 does it seems on some cases)

@oshoval
Copy link
Member Author

oshoval commented Sep 28, 2023

passed

@phoracek
Copy link
Member

Thanks for getting this addressed

/approve
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2023
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oshoval, phoracek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2023
@phoracek
Copy link
Member

FIY @maiqueb there seem to be compatibility issues between Calico and thick Multus

@kubevirt-bot kubevirt-bot merged commit 4247a3c into k8snetworkplumbingwg:main Sep 29, 2023
@maiqueb
Copy link

maiqueb commented Sep 29, 2023

FIY @maiqueb there seem to be compatibility issues between Calico and thick Multus

@oshoval brought this to my attention yesterday.

From what I understand, it simply can't find the plugin binary in the folder where the CNIs are supposed to be ... The question is why.

@maiqueb
Copy link

maiqueb commented Sep 29, 2023

OK found out the reason.

This issue reports this same issue. It's not only calico. Basically multus now doesn't has the path to any CNI plugins. I don't know how this could ever have passed CI ... Oh wait... I do know: in CI it uses a different manifest 0_o

This PR broke it.

This PR fixes it.

@oshoval where are we using this snapshot multus tag ?... We should stop using that and use a pinned version instead - v4.0.2-thick or stable-thick tags.

Can you take care of that ?

@oshoval
Copy link
Member Author

oshoval commented Oct 1, 2023

Nice, about the question - mentioned on PR desc (manifest is used at cluster/up.sh)
sure
#282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants