k8s, cluster-up: Copy istio CNI net conf to where Multus expects #932

ormergi · 2022-12-13T07:24:35Z

Since Multus v4 it expects CNI net conf file to exist at /etc/cni/net.d but Isitio generates its net conf file at /etc/cni/multus/net.d.

Copy istio CNI net conf to where multus expects on cluster-up in order to enable deploying both Isitio and Multus until proper solution is available, tracking issue [1] [2].

[1] #906 [2] k8snetworkplumbingwg/multus-cni#982

Signed-off-by: Or Mergi [email protected]

ormergi · 2022-12-13T07:24:56Z

/cc @maiqueb @brianmcarey @dhiller

fetch kubevirt/kubevirtci#932 changes Signed-off-by: Or Mergi <[email protected]>

phoracek · 2022-12-13T08:43:31Z

cluster-up/cluster/k8s-provider-common.sh

@@ -155,4 +163,5 @@ function up() {
        sleep 5
    done

+    copy_istio_cni_conf_files


Please add a FIXME comment linking to a tracker on Multus and also open an Issue under this repository, so we don't forget to clean up.

phoracek · 2022-12-13T08:44:41Z

cluster-up/cluster/k8s-provider-common.sh

+function copy_istio_cni_conf_files() {
+  if [ "$KUBEVIRT_DEPLOY_ISTIO" == "true" ] && [ "$KUBEVIRT_WITH_CNAO" == "true" ]; then
+    for nodeNum in $(seq -f "%02g" 1 $KUBEVIRT_NUM_NODES); do
+      $ssh node${nodeNum} -- sudo cp -uv /etc/cni/multus/net.d/*istio*.conf /etc/cni/net.d/


Would creating a symlink be an option too? It would allow us to keep it generic and we would not need to wait for Istio to finish deployment

@phoracek it requires knowing the name of isito conf file that will be generated in order to create the link (though it won't likely to change), then it would be possible to create the link as part of deploy_istio() (before Isitio deployment ready), for example:

ISTIO_CONF_FILE_NAME="YYY-istio-cni.conf" ... function deploy_istio() { if [ "$KUBEVIRT_DEPLOY_ISTIO" == "true" ]; then if [ "$KUBEVIRT_WITH_CNAO" == "true" ]; then ... $ssh node${nodeNum} -- sudo ln -sr /etc/cni/multus/net.d/${ISTIO_CONF_FILE_NAME} /etc/cni/net.d/ ...

WDYT?

What I meant is linking the whole directory. So no matter from which one you read/write-to, we will always have the same set of configs

It wont work because Multus dosent look for net conf files recursively nor look into links, the conf file must be copied.

i see, interesting, thanks for checking that

dhiller · 2022-12-13T10:50:17Z

cluster-up/cluster/k8s-provider-common.sh

@@ -88,6 +88,14 @@ function wait_for_istio_ready() {
    fi
 }

+function copy_istio_cni_conf_files() {
+  if [ "$KUBEVIRT_DEPLOY_ISTIO" == "true" ] && [ "$KUBEVIRT_WITH_CNAO" == "true" ]; then


nit: please adjust indentation to the remainder of the script

Thanks for the heads up, done

ormergi · 2022-12-13T14:47:20Z

@phoracek @dhiller I have resolved your review comments please take another look 🙂

phoracek

Thanks

Since Multus v4 it expects CNI net conf file to exist at '/etc/cni/net.d' but isitio generates then at '/etc/cni/multus/net.d'. Copy istio CNI net conf to where multus expects on cluster-up in order to enable deploying both Isitio and Multus v4 until proper solution is available, tracking issue [1] [2]. [1] kubevirt#906 [2] k8snetworkplumbingwg/multus-cni#982 Signed-off-by: Or Mergi <[email protected]>

ormergi · 2022-12-13T14:56:18Z

Changes: fixed missing if closure closing, missing indentation and removed the fixme comment leftover.

maiqueb

Thanks.

maiqueb · 2022-12-13T15:34:33Z

Weird, the 1.26 provisioning lane is failing w/

+ dnf install -y kernel-modules-5.14.0-210.el9.x86_64
CentOS Stream 9 - BaseOS                        8.8 MB/s | 6.0 MB     00:00    
CentOS Stream 9 - AppStream                      10 MB/s |  16 MB     00:01    
CentOS Stream 9 - Extras packages                11 kB/s | 9.2 kB     00:00    
No match for argument: kernel-modules-5.14.0-210.el9.x86_64
Error: Unable to find a match: kernel-modules-5.14.0-210.el9.x86_64

But I see the pkg available online:
http://mirror.stream.centos.org/9-stream/BaseOS/x86_64/os/Packages/kernel-modules-5.14.0-210.el9.x86_64.rpm

... let's blindly retest once.
/test check-provision-k8s-1.26

maiqueb · 2022-12-13T15:47:22Z

Can we be missing some packages on the 1.26 lanes ?
@dhiller / @brianmcarey

podman run --rm --entrypoint bash -it centos:stream9 
Resolved "centos" as an alias (/etc/containers/registries.conf.d/000-shortnames.conf)
Trying to pull quay.io/centos/centos:stream9...
Getting image source signatures
Copying blob 5af5d4bc82b8 done  
Copying config bc5c131925 done  
Writing manifest to image destination
Storing signatures
[root@95b437f4fede /]# dnf info --showduplicates kernel-modules | grep Version -A1
Version      : 5.14.0
Release      : 196.el9
--
Version      : 5.14.0
Release      : 197.el9
--
Version      : 5.14.0
Release      : 200.el9
--
Version      : 5.14.0
Release      : 202.el9
--
Version      : 5.14.0
Release      : 205.el9

dhiller · 2022-12-13T17:00:13Z

I am trying https://cloud.centos.org/centos/9-stream/x86_64/images/CentOS-Stream-Vagrant-9-20221206.0.x86_64.vagrant-libvirt.box

aaaand, it ofcourse has the same issue 😕

ormergi · 2022-12-13T17:46:45Z

Please note that this PR change has nothing to do with check-provision-k8s-1.26 and check-up-kind-1.23-vgpu lane failures 🙂

dhiller · 2022-12-13T17:52:12Z

Please note that this PR change has nothing to do with check-provision-k8s-1.26 and check-up-kind-1.23-vgpu lane failures slightly_smiling_face

Yes but it will break the publishing, since the 1.26 will fail to build

Signed-off-by: Or Mergi <[email protected]>

ormergi · 2022-12-13T18:07:14Z

Yes but it will break the publishing, since the 1.26 will fail to build

Oh, I didn't realize that, I though that it wont be included in the publish process since the lane is not mandatory :/

Barakmor1 · 2022-12-14T07:22:56Z

During provisioning the provision.sh is being run inside the Centos Stream 9 box, which is a vm that is started inside a container image. This image should have the same kernel version for every run, but seemingly it changed. Asking @Barakmor1 to keep me honest here.

Yes that is how it should:
https://github.com/kubevirt/kubevirtci/blob/main/cluster-up/cluster/K8S_DEV_GUIDE.md
We changed the Centos Stream 9 version is because the lane started failing few days ago:
#924

Signed-off-by: Or Mergi <[email protected]>

brianmcarey · 2022-12-14T08:46:48Z

/test check-provision-k8s-1.26

dhiller · 2022-12-14T09:33:21Z

I am suspecting that the change originates from a change in vmlinuz and/or initrd.img which are both pulled during image creation.

I am seeing a change on the initrd.img from the 12th of Dec and for vmlinuz on the 9th of Dec here: http://mirror.stream.centos.org/9-stream/BaseOS/x86_64/os/images/pxeboot/

ormergi · 2022-12-14T11:05:48Z

I am suspecting that the change originates from a change in vmlinuz and/or initrd.img which are both pulled during image creation.

I am seeing a change on the initrd.img from the 12th of Dec and for vmlinuz on the 9th of Dec here: http://mirror.stream.centos.org/9-stream/BaseOS/x86_64/os/images/pxeboot/

Note that it is the same for centos8, initd.img and vmlinuz have different release dates
http://mirror.centos.org/centos/8-stream/BaseOS/x86_64/os/images/pxeboot/

brianmcarey · 2022-12-14T12:20:17Z

/test check-provision-k8s-1.26

dhiller · 2022-12-14T12:21:58Z

/test check-provision-k8s-1.26
/override check-up-kind-1.23-vgpu

dhiller

/approv

dhiller · 2022-12-14T12:23:40Z

/approve

kubevirt-bot · 2022-12-14T12:34:33Z

@dhiller: Overrode contexts on behalf of dhiller: check-up-kind-1.23-vgpu

In response to this:

/test check-provision-k8s-1.26
/override check-up-kind-1.23-vgpu

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

brianmcarey · 2022-12-14T12:51:05Z

/test check-provision-k8s-1.26

oshoval · 2022-12-14T12:51:06Z

cluster-up/cluster/k8s-provider-common.sh

+# copy_istio_cni_conf_files copy the generated Istio CNI net conf file
+# (at '/etc/cni/multus/net.d/') to where Multus expect CNI net conf files ('/etc/cni/net.d/')
+function copy_istio_cni_conf_files() {
+    if [ "$KUBEVIRT_DEPLOY_ISTIO" == "true" ] && [ "$KUBEVIRT_WITH_CNAO" == "true" ]; then


what about KUBVIRT_WITH_CNAO_SKIP_CONFIG ?
The CR might be installed bit later and include the multus (but not sure if those folders do exists in this case at this stage ?)

can be ignored, if there is code that returns error upon this configuration

what about KUBVIRT_WITH_CNAO_SKIP_CONFIG ?
The CR might be installed bit later and include the multus (but not sure if those folders do exists in this case at this stage ?)

If by CR you mean CNAO CR, in the scenrtio where Multus deployment hangs no harm will do to the cluster if Istio confs will be copied to /etc/cni/net.d(it is already created by kubeadm IIRC)

Note that copy_istio_cni_conf_files is called only after both CNAO and Istio deployments are ready, so it is not likely to happen.

This function is called only after both CNAO and Istio deployments are ready, so it is not likely to happen.

I am not sure what you mean, the error flag is set for script.

I mean that the condition checks KUBEVIRT_WITH_CNAO but doesnt check the possible
KUBVIRT_WITH_CNAO_SKIP_CONFIG
That allows you to install CNAO but wihtout the CNAO CR (where you say which components to install)

So if CNAO will be installed without CR (i.e using KUBVIRT_WITH_CNAO_SKIP_CONFIG)
and Istio will be installed
and later the CNAO CR will install multus, this WA code wont run and we will have the bug

Thanks for explaining.
I think it's better to have it the way it is (w/o handling KUBVIRT_WITH_CNAO_SKIP_CONFIG) because it's we want it run when both Istio and Multus (through CNAO) are deployed.

BTW, I couldn't find usage of KUBVIRT_WITH_CNAO_SKIP_CONFIG on kubevirt CI jobs, perhaps it should be removed.

I am fine to not handle it (we just need to know we don't support this config)
but please don't remove it .
We are using KUBVIRT_WITH_CNAO_SKIP_CONFIG on KubeSecondaryDNS.
It is helpful once you want to deploy the cached CNAO, but with your CR.
This way you don't need to care about CNAO bumps, as it is part of the kubevirtci TAG that you need to bump anyhow

maiqueb · 2022-12-14T13:18:23Z

OK seems it managed to install the kernel modules.

@dhiller your /approve was not handled by the bot. Is something wrong with it ?

brianmcarey

/approve

kubevirt-bot · 2022-12-14T15:10:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianmcarey, dhiller

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~~ [brianmcarey,dhiller]

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

ormergi · 2022-12-14T18:02:31Z

/test check-provision-k8s-1.25

kubevirt-commenter-bot · 2022-12-14T20:13:02Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[6b053d7 k8s, cluster-up: Copy istio CNI net conf to where Multus expects](kubevirt/kubevirtci#932) [f930087 cluster-up: add environment variable to enable FIPS](kubevirt/kubevirtci#931) [c1a7fde Upgrade cri-o to version 1.25 for 1.25/6 providers](kubevirt/kubevirtci#930) [16f44a7 Delete k8s-1.24-ipv6](kubevirt/kubevirtci#929) ```release-note NONE ``` Signed-off-by: kubevirt-bot <[email protected]>

[5660e89 Enable PSA on any provider](kubevirt/kubevirtci#915) [6304ac8 bump, cnao: k8s-1.2[4,5] to cnao v0.82.0](https://github.com/kubevirt/kubevirtci/pull/910)](https://github.com/kubevirt/kubevirtci/pull/933) [6b053d7 k8s, cluster-up: Copy istio CNI net conf to where Multus expects](kubevirt/kubevirtci#932) [f930087 cluster-up: add environment variable to enable FIPS](kubevirt/kubevirtci#931) [c1a7fde Upgrade cri-o to version 1.25 for 1.25/6 providers](kubevirt/kubevirtci#930) [16f44a7 Delete k8s-1.24-ipv6](kubevirt/kubevirtci#929) ```release-note NONE ``` Signed-off-by: kubevirt-bot <[email protected]>

Signed-off-by: Or Mergi <[email protected]>

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Dec 13, 2022

kubevirt-bot requested review from davidvossel and dhiller December 13, 2022 07:24

kubevirt-bot added the size/XS label Dec 13, 2022

kubevirt-bot requested review from brianmcarey and maiqueb December 13, 2022 07:24

ormergi added a commit to ormergi/kubevirt that referenced this pull request Dec 13, 2022

debug: fetch kubevirt/kubevirtci#932 changes

8841c8a

fetch kubevirt/kubevirtci#932 changes Signed-off-by: Or Mergi <[email protected]>

ormergi mentioned this pull request Dec 13, 2022

Test kubevirt/kubevirtci#932 with #6852 kubevirt/kubevirt#8934

Closed

phoracek reviewed Dec 13, 2022

View reviewed changes

dhiller reviewed Dec 13, 2022

View reviewed changes

ormergi force-pushed the multus-istio-intergation branch from 317ffef to ee2e69f Compare December 13, 2022 14:45

kubevirt-bot added size/S and removed size/XS labels Dec 13, 2022

phoracek approved these changes Dec 13, 2022

View reviewed changes

kubevirt-bot assigned phoracek Dec 13, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

ormergi force-pushed the multus-istio-intergation branch from ee2e69f to af74d52 Compare December 13, 2022 14:54

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

maiqueb approved these changes Dec 13, 2022

View reviewed changes

kubevirt-bot assigned maiqueb Dec 13, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

phoracek approved these changes Dec 13, 2022

View reviewed changes

ormergi mentioned this pull request Dec 13, 2022

bump, cnao: k8s-1.2[4,5] to cnao v0.82.0 -- take2 #933

Merged

ormergi added a commit to ormergi/kubevirt that referenced this pull request Dec 13, 2022

debug: fetch kubevirt/kubevirtci#932 changes

82c66a4

Signed-off-by: Or Mergi <[email protected]>

ormergi added a commit to ormergi/kubevirt that referenced this pull request Dec 14, 2022

debug: fetch kubevirt/kubevirtci#932 changes

ad3dd6d

Signed-off-by: Or Mergi <[email protected]>

dhiller reviewed Dec 14, 2022

View reviewed changes

oshoval reviewed Dec 14, 2022

View reviewed changes

brianmcarey approved these changes Dec 14, 2022

View reviewed changes

kubevirt-bot assigned brianmcarey Dec 14, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2022

kubevirt-bot merged commit 6b053d7 into kubevirt:main Dec 14, 2022

kubevirt-bot mentioned this pull request Dec 15, 2022

Bump kubevirtci kubevirt/kubevirt#8956

Merged

ormergi added a commit to ormergi/kubevirt that referenced this pull request Jan 6, 2023

debug: fetch kubevirt/kubevirtci#932 changes

63d37ea

Signed-off-by: Or Mergi <[email protected]>

ormergi mentioned this pull request Sep 4, 2024

fix: Wait until istio cni files appear before copying them #1262

Merged

8 tasks

k8s, cluster-up: Copy istio CNI net conf to where Multus expects #932

k8s, cluster-up: Copy istio CNI net conf to where Multus expects #932

Conversation

ormergi commented Dec 13, 2022 • edited Loading

ormergi commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormergi commented Dec 13, 2022

phoracek left a comment

Choose a reason for hiding this comment

ormergi commented Dec 13, 2022 • edited Loading

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb commented Dec 13, 2022

maiqueb commented Dec 13, 2022

dhiller commented Dec 13, 2022

ormergi commented Dec 13, 2022

dhiller commented Dec 13, 2022

ormergi commented Dec 13, 2022

Barakmor1 commented Dec 14, 2022 • edited Loading

brianmcarey commented Dec 14, 2022

dhiller commented Dec 14, 2022

ormergi commented Dec 14, 2022

brianmcarey commented Dec 14, 2022

dhiller commented Dec 14, 2022

dhiller left a comment

Choose a reason for hiding this comment

dhiller commented Dec 14, 2022

kubevirt-bot commented Dec 14, 2022

brianmcarey commented Dec 14, 2022

Choose a reason for hiding this comment

ormergi Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

oshoval Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

maiqueb commented Dec 14, 2022

brianmcarey left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Dec 14, 2022

ormergi commented Dec 14, 2022

kubevirt-commenter-bot commented Dec 14, 2022

ormergi commented Dec 13, 2022 •

edited

Loading

ormergi commented Dec 13, 2022 •

edited

Loading

Barakmor1 commented Dec 14, 2022 •

edited

Loading

ormergi Dec 14, 2022 •

edited

Loading

oshoval Dec 14, 2022 •

edited

Loading

oshoval Dec 14, 2022 •

edited

Loading