k8snetworkplumbingwg / ovs-cni

Open vSwitch CNI plugin
Apache License 2.0
218 stars 70 forks source link

multus: Use thin version #281

Closed oshoval closed 11 months ago

oshoval commented 11 months ago

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
oshoval commented 11 months ago

/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 commented 11 months ago

passed

phoracek commented 11 months ago

Thanks for getting this addressed

/approve /lgtm

kubevirt-bot commented 11 months ago

[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: - ~~[OWNERS](https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/OWNERS)~~ [phoracek] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
phoracek commented 11 months ago

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

maiqueb commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

Nice, about the question - mentioned on PR desc (manifest is used at cluster/up.sh) sure https://github.com/k8snetworkplumbingwg/ovs-cni/pull/282