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

[E2E] Use weave as default cni for all k8s and kind versions. #123

Merged
merged 1 commit into from
Jul 31, 2019
Merged

[E2E] Use weave as default cni for all k8s and kind versions. #123

merged 1 commit into from
Jul 31, 2019

Conversation

dimaunx
Copy link

@dimaunx dimaunx commented Jul 29, 2019

This PR uses Cluster.networking.disableDefaultCNI option for clusters 2 and 3. After the container nodes are up we are installing weave manually. In previous setup with k8s version 1.14.1, weave was ignoring podCidr provided in kind config file and used its default 10.32.0.0/12 cidr as podCidr. This PR fixes this issue.The result is a cluster with weave and podCidr that is taken from kind config file. This removes the need to be constrained to specific k8s version or kind version. After this is merged kind can be safely upgraded. Other types of cni plugins can be tested more easily.

@dimaunx dimaunx changed the title Use weave as default cni for all k8s and kind versions. [E2E] Use weave as default cni for all k8s and kind versions. Jul 29, 2019
@@ -19,13 +19,14 @@ function kind_clusters() {
echo Creating cluster${i}, logging to ${logs[$i]}...
(
if [[ -n ${version} ]]; then
kind create cluster --image=kindest/node:v${version} --name=cluster${i} --wait=5m --config=${PRJ_ROOT}/scripts/kind-e2e/cluster${i}-config.yaml
kind create cluster --image=kindest/node:v${version} --name=cluster${i} --config=${PRJ_ROOT}/scripts/kind-e2e/cluster${i}-config.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we remove the wait=5m? :)

Copy link
Author

Choose a reason for hiding this comment

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

The control plain comes up without cni, --wait waits for control plain to be up and its never up until we apply weave cni.

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh, makes sense, thanks :-D

echo "Weave already deployed cluster${i}."
else
echo "Applying weave network in to cluster${i}..."
kubectl --context=cluster${i} apply -f "https://cloud.weave.works/k8s/net?k8s-version=$(kubectl version | base64 | tr -d '\n')&env.IPALLOC_RANGE=${POD_CIDR[cluster${i}]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow! :D that's neat of them!

Copy link
Contributor

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

I have a few questions inline but looks good to me.

It'd be awesome to be able to control on an env var if we want kindnet or weave, or eventually something else. But let's get into that complication the day we really need it.

Awesome work! :)

@mangelajo
Copy link
Contributor

@sridhargaddam will love this ;)

@dimaunx
Copy link
Author

dimaunx commented Jul 29, 2019

@sridhargaddam will love this ;)

Yes :)

@dimaunx
Copy link
Author

dimaunx commented Jul 29, 2019

I have a few questions inline but looks good to me.

It'd be awesome to be able to control on an env var if we want kindnet or weave, or eventually something else. But let's get into that complication the day we really need it.

Yes that's nice idea. I have also tested calico, but had some issues related to kernel modules. Seems to be fixable. Didn't want to waste too much time and used weave instead. Seems as long as cni supports podCidr customization we can utilize most of them.

Copy link
Member

@sridhargaddam sridhargaddam left a comment

Choose a reason for hiding this comment

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

Looks good. Verified the patch and it works fine :)
Great fix @dimaunx

@mangelajo mangelajo merged commit 5cfe40d into submariner-io:master Jul 31, 2019
@dimaunx dimaunx deleted the custom-cni branch September 5, 2019 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants