fix: Wait until istio cni files appear before copying them

[aerosouund]

What this PR does / why we need it:

This fixes the issue that appeared with the rewrite of the istio cluster option to go: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/12677/pull-kubevirt-e2e-k8s-1.31-sig-network/1829308375628779520
A brief description of the issue is that during the rewrite the code that copied the istio cni files to /etc/cni/net.d was removed because both issues referenced were closed and because it caused cluster-up to crash

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} -- "until ls /etc/cni/multus > /dev/null 2>&1; do sleep 1; done"
            $ssh node${nodeNum} -- sudo cp -uv /etc/cni/multus/net.d/*istio*.conf /etc/cni/net.d/
        done
    fi
}

The reason why it would crash is because this code was previously being called after a period of waiting to allow for istio cni files to appear (not deliberately, the code was waiting for something else)

    until wait_for_cnao_ready && wait_for_istio_ready && wait_for_cdi_ready && wait_for_multus_ready && wait_for_aaq_ready && wait_for_kwok_ready; do
        echo "Waiting for cluster components..."
        sleep 5
    done

    # FIXME: remove 'copy_istio_cni_conf_files()' as soon as [1] and [2] are resolved
    # [1] https://github.com/kubevirt/kubevirtci/issues/906
    # [2] https://github.com/k8snetworkplumbingwg/multus-cni/issues/982
    copy_istio_cni_conf_files

this pull request introduces this wait again after ensuring that the cni files appeared on the hosts

It also updates CNAO manifests deployed by the gocli to v0.94.1 which is the version used before using the gocli in deploying those manifests.

As well as remove this part from the yaml manifest of istio operator with CNAO

      cniConfFileName: "istio-cni.conf"
    sidecarInjectorWebhook:
      injectedAnnotations:
        "k8s.v1.cni.cncf.io/networks": istio-cni

This part was not previously in the manifest and was incorrectly added in #1217
The effect of this is an error during VMI sync from passt being unable to bind to port 22 and the VMI being stuck in Scheduled state

passt --one-off --socket /var/run/libvirt/qemu/run/passt/1-kubevirt-test-defaul-ua-default.socket --pid /var/run/libvirt/qemu/run/passt/1-kubevirt-test-defaul-ua-default-passt.pid --interface eth0 --log-file /var/run/kubevirt/passt.log --tcp-ports '~15000,~15001,~15004,~15006,~15008,~15009,~15020,~15021,~15053,~15090,1500,1501,22') unexpected exit status 1: Failed to bind port 22 (Permission denied) for option '-t ~15000,~15001,~15004,~15006,~15008,~15009,~15020,~15021,~15053,~15090,1500,1501,22', exiting\n'

And lastly this PR also omits multus from the CNAO CR being deployed in case multus is requested in the cluster to be installed on its own through the KUBEVIRT_WITH_MULTUS_V3 variable, by moving this logic

function create_network_addons_config() {
    local nac="/opt/cnao/network-addons-config-example.cr.yaml"
    if [ "$KUBEVIRT_WITH_MULTUS_V3" == "true" ]; then
        local no_multus_nac="/opt/cnao/no-multus-nac.yaml"
        $ssh node01 -- "awk '!/multus/' ${nac} | sudo tee ${no_multus_nac}"
        nac=${no_multus_nac}
    fi

     $kubectl apply -f ${nac}
}

into this go logic

      if path == "manifests/network-addons-config-example.cr.yaml" && o.multusEnabled {
	      re := regexp.MustCompile("(?m)[\r\n]+^.*multus.*$")
	      res := re.ReplaceAllString(string(yamlDoc), "")
	      yamlDoc = []byte(res)
      }
      
      obj, err := k8s.SerializeIntoObject(yamlDoc)
      if err != nil {
	      logrus.Info(err.Error())
	      continue
      }
      if err := o.client.Apply(obj); err != nil {
	      return fmt.Errorf("error applying manifest %s", err)
      }
      }

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

• Design: A design document was considered and is present (link) or not required
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
• Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
• Community: Announcement to kubevirt-dev was considered

Release note:

NONE

[aerosouund]

cc: @brianmcarey

[dhiller]

/cc @EdDev @oshoval

Could you have a look at this PR? It needs to get in so we can stabilize the bump-kubevirtci which is failing.

/priority critical-urgent

[brianmcarey]

Is there a danger here that it could sleep forever?

[aerosouund]

its unlikely, since if istio is deployed with cnao the cni file then it will create this cni file. if it couldn't then it would crash before getting here

but i will add a timeout for extra safety

[ormergi]

Please do this change in separate PR.

[brianmcarey]

/approve

@aerosouund you mentioned that you ran the sig-network tests locally against this and the tests looked ok?

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianmcarey

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]

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

[aerosouund]

/approve

@aerosouund you mentioned that you ran the sig-network tests locally against this and the tests looked ok?

Yeah indeed, to be exact not all. The ones that were failing. I.E the ones in the file vmi_istio.go

[EdDev]

/cc @EdDev @oshoval

Could you have a look at this PR? It needs to get in so we can stabilize the bump-kubevirtci which is failing.

/priority critical-urgent

Unfortunately, we have no ability or capacity to support the redo of the network changes accepted as part of the bash-to-go migration.
I guess the maintainers who approved the migration to Go will now be owning all this logic.

[xpivarc]

Can we have a test for this?

[dhiller]

@EdDev all we are asking here is to verify whether the changes to the manifests are reasonable - we will make sure that the other changes do work.

[dhiller]

Two commits are to look at here:

• 10f7aaf
• 7b8b9e8

[ormergi]

Hi, thanks of the PR, please see my comment below and inline comments.

In general it seem to do more then what the title and description say, such as CNAO downgrade, and adding timeout for changes around istio files.
For next time, please make sure a PR does exactly what it describe, and if necessary break it down to smaller PRs.

Regarding commit "fix: Use CNAO v0.94", such changes as version upgrade/downgrade for a component should be done in a self contained PR.
Having such changes makes code-review easier and keep reviewers from being distracted by other changes (noise).
In addition it enable quick revert in case of an issue.
For next time, post separate PR for CNAO version change.

[ormergi]

Please add the previous comment and links to tracking issue.

[ormergi]

FWIU changes should be exactly as before, initially introduced by #932

[aerosouund]

added them again

[ormergi]

CNAO should also deploy the dynamic-network-controller for kubevirt/kubevirt e2e tests.
Please ensure the current behavior remains.

[aerosouund]

this parameter is controlled by the overall KUBEVIRT_WITH_MULTUS_V3 param
from a callers perspective everything will work the same.
and what it previously was is that if this variable is passed multus is deployed separately. not by CNAO.
see this comment i left for or shoval inquiring about the same thing: 7b8b9e8#r146222431

of if you're referring to something else let me know

[ormergi]

This change belong to previous commit.

[aerosouund]

if you are referring to the multus issue then it was squashed

[ormergi]

This change is not related to this PR according to title and description.

Please move this change to new PR.

[aerosouund]

Indeed, but it was added as a part of a previous review: #1262 (comment)

[ormergi]

Nevertheless, this change is not related to this PR, right?

[aerosouund]

@ormergi
Yes its not, maybe we can separate it another PR indeed
edit: removed it

[xpivarc]

@aerosouund Let's minimize the changes so we fix the issues first and then we will improve

[xpivarc]

@aerosouund I have left few questions

[xpivarc]

@aerosouund Is this something new or did we have this before?

[aerosouund]

it was there before

[xpivarc]

@aerosouund Can you please explain the reasoning to the code as well. This was something that took me time to connect. Please enhance the commit message as well

[aerosouund]

indeed it can be enhanced, will do

[xpivarc]

Please just mention in commit that this reverts to previous configuration

[aerosouund]

@xpivarc
kindly check if the commit messages now are expressive enough :)

[aerosouund]

@xpivarc
i have added test cases to assert that the CNAO opt is handling the creation of multus properly
let me know if there are any further changes required

[kubevirt-bot]

@aerosouund: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
check-up-kind-1.27-vgpu	e0c0250	link	false	/test check-up-kind-1.27-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-sigs/prow repository. I understand the commands that are listed here.

[oshoval]

iiuc now all the providers have the same file ?
it means we cant have separated cnao anymore for each provider?
at the past the untangling of the folders was done deliberately for that
please consider if this is the desired behavior
if there will be problems, it would block us, we can either fix it or be proactive if it happens
thanks