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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Dec 14, 2022
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions cluster-up/cluster/k8s-provider-common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,16 @@ function wait_for_istio_ready() {
fi
}

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ormergi ormergi Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@oshoval oshoval Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@oshoval oshoval Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

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/
done
fi
}

function deploy_cdi() {
if [ "$KUBEVIRT_DEPLOY_CDI" == "true" ]; then
$kubectl create -f /opt/cdi-*-operator.yaml
Expand Down Expand Up @@ -155,4 +165,8 @@ function up() {
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}