Kubeadm Networking Configuration E2E Tests #80259

Arvinderpal · 2019-07-17T17:02:23Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Currently, kubeadm e2e tests don't check any of the networking configuration values. This PR adds 2 new e2e_kubeadm tests for network configurations:

podSubnet check: if a podSubnet is specified in kubeadm-config
then the e2e test will check that pod-cidrs of individual nodes
fall within this range.
serviceSubnet check: if a serviceSubnet is specified in
kubeadm-config then the e2e test will check that the kubernetes
service created in the default namespace got a service IP
from the configured range.

Special notes for your reviewer:

There will be follow up dual-stack tests as that feature lands in master. These tests are however single-stack (either IPv4 or IPv6) only.
To run just these tests: make test-e2e-kubeadm FOCUS=setup-networking

Does this PR introduce a user-facing change?:
NONE

k8s-ci-robot · 2019-07-17T17:02:25Z

@Arvinderpal: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2019-07-17T17:02:31Z

Hi @Arvinderpal. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Arvinderpal · 2019-07-17T17:05:02Z

@fabriziopandini @neolit123 @yastij @aojea
These are single-stack tests. I put them into a separate PR. Please let me know if this approach is sound. I will add similar dual-stack tests as that feature is completed.

yastij · 2019-07-17T17:09:16Z

/ok-to-test

yastij · 2019-07-17T17:41:45Z

/assign @neolit123 @yastij

neolit123 · 2019-07-17T18:26:33Z

thanks @Arvinderpal
please poke us for review once the CI tests pass.

/assign @fabriziopandini @neolit123

test/e2e_kubeadm/networking_test.go

fabriziopandini

@Arvinderpal thanks for taking care of improving kubeadm e2e test coverage!

The PR goes definitely in the right direction, but I would like to check with you/other reviewers if:

we can make detection of cluster settings more robust (or set clear expectations)
make this test verify kubeadm behavior directly, without going through the indirection of other components responsible for applying network settings to node/services

test/e2e_kubeadm/networking_test.go

fabriziopandini · 2019-07-18T09:25:03Z

test/e2e_kubeadm/networking_test.go

+					d := cc["networking"].(map[interface{}]interface{})
+					if ps, ok := d["podSubnet"]; ok {
+						// Check that the pod CIDR allocated to the node(s) is within the kubeadm-config podCIDR.
+						nodes, err := f.ClientSet.CoreV1().Nodes().List(metav1.ListOptions{})


This test is checking kubeadm behaviour indirectly, and I'm wondering if instead we should test the kubeadm bit more specifically

Kubeadm only responsibility for networking is to pass the settings to the control plane components (in this case, to translate podSubnet into cluster-cidr, allocate-node-cidrs and node-cidr-mask-size flags for the controller manager)

What do you have in mind?
I followed this approach based on what I saw in other tests.

Sorry for not being clear.
I think we should have a test that checks kubeadm assigns the expected value to cluster-cidr, allocate-node-cidrs and node-cidr-mask-size (in addition or in replacement to the current test)

@fabriziopandini
allocate-node-cidrs is internal to controller-manager and is not exposed, so I don't see any way to verify that.
node-cidr-mask-size is calcuated by kubeadm (see func calcNodeCidrSize). There are some issues with verifying it:

This func currently assumes IPv4 (assigns /24 to IPv6 -- a known problem).

We'll need to change this func to support dual-stack, so node-cidr-mask-size will actually become a slice.

node-cidr-mask-size is not written to kubeadm config, so we'll have to duplicate this func in the e2e tests in order to verify it.

calcNodeCidrSize is unit tested already.
Because of these reasons, I would prefer not to add e2e tests for this. Perhaps when dual-stack changes land, we can revisit this.

Ok, I got your point and I don't want to block on this.
However, I think this test is useful and doable reading the args in the controller-manager pod, so please add an item on the umbrella issue: "implement a test that checks kubeadm assigns the expected value to cluster-cidr, allocate-node-cidrs and node-cidr-mask-size (in addition or in replacement to the current test"

Arvinderpal · 2019-07-18T19:49:22Z

@fabriziopandini @yastij Thank you for you feedback. Please see my comments.

test/e2e_kubeadm/BUILD

aojea · 2019-07-20T22:01:28Z

xref #79033

test/e2e_kubeadm/networking_test.go

fabriziopandini · 2019-07-25T12:18:53Z

@Arvinderpal thanks for the update; please update the umbrella issue as suggested
/approve
I leave to @rosti / @neolit123 / @yastij final lgtm

Arvinderpal · 2019-07-25T14:24:06Z

@Arvinderpal thanks for the update; please update the umbrella issue as suggested
/approve
I leave to @rosti / @neolit123 / @yastij final lgtm

@fabriziopandini I have created a new umbrella issue for these e2e tests since they are not directly related to the dual-stack work. kubernetes/kubeadm#1686
As requested, I have added tasks for the other config values that you want tested.

test/e2e_kubeadm/networking_test.go

rosti

Thanks @Arvinderpal !
Apart from the incomplete dual stack test, this looks good.

rosti · 2019-07-26T13:35:14Z

test/e2e_kubeadm/networking_test.go

+			})
+		})
+	})
+	Context("dual-stack [Feature:IPv6DualStack]", func() {


Let's omit this test case from this PR and add it instead in its entirety in #79033.

* podSubnet check: if a podSubnet is specified in kubeadm-config then the e2e test will check that pod-cidrs of individual nodes fall within this range. * serviceSubnet check: if a serviceSubnet is specified in kubeadm-config then the e2e test will check that the kubernetes service created in the default namespace got a service IP from the configured range.

Arvinderpal · 2019-07-27T14:58:48Z

@rosti / @neolit123 / @yastij I think this PR can be merged. I have addressed all the comments.

neolit123 · 2019-07-28T18:53:19Z

/release-note-none

neolit123

/priority backlog
/lgtm
/approve

k8s-ci-robot · 2019-07-28T18:54:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Arvinderpal, fabriziopandini, neolit123

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:

~~test/e2e_kubeadm/OWNERS~~ [fabriziopandini,neolit123]

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

fejta-bot · 2019-07-28T23:04:07Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from kad and timothysc July 17, 2019 17:04

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 17, 2019

Arvinderpal force-pushed the kubeadm-e2e-networking-config branch from 6caaef8 to 1ec0290 Compare July 17, 2019 17:22

k8s-ci-robot assigned neolit123 and yastij Jul 17, 2019

k8s-ci-robot assigned fabriziopandini Jul 17, 2019

yastij reviewed Jul 17, 2019

View reviewed changes

test/e2e_kubeadm/networking_test.go Outdated Show resolved Hide resolved

test/e2e_kubeadm/networking_test.go Outdated Show resolved Hide resolved

test/e2e_kubeadm/networking_test.go Show resolved Hide resolved

test/e2e_kubeadm/networking_test.go Show resolved Hide resolved

fabriziopandini reviewed Jul 18, 2019

View reviewed changes

aojea reviewed Jul 18, 2019

View reviewed changes

test/e2e_kubeadm/BUILD Outdated Show resolved Hide resolved

Arvinderpal force-pushed the kubeadm-e2e-networking-config branch from 1ec0290 to bbe1866 Compare July 18, 2019 20:13

Arvinderpal force-pushed the kubeadm-e2e-networking-config branch from bbe1866 to b81b246 Compare July 23, 2019 14:09

timothysc reviewed Jul 24, 2019

View reviewed changes

test/e2e_kubeadm/networking_test.go Outdated Show resolved Hide resolved

Arvinderpal force-pushed the kubeadm-e2e-networking-config branch from b81b246 to a500078 Compare July 24, 2019 21:56

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2019

Arvinderpal mentioned this pull request Jul 25, 2019

[kubeadm] Network Config E2E Tests kubernetes/kubeadm#1686

Closed

4 tasks

SataQiu reviewed Jul 26, 2019

View reviewed changes

test/e2e_kubeadm/networking_test.go Outdated Show resolved Hide resolved

rosti reviewed Jul 26, 2019

View reviewed changes

Arvinderpal force-pushed the kubeadm-e2e-networking-config branch from a500078 to ca3bdfd Compare July 27, 2019 14:56

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 28, 2019

neolit123 reviewed Jul 28, 2019

View reviewed changes

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 28, 2019

k8s-ci-robot merged commit c1d2ac4 into kubernetes:master Jul 29, 2019

Arvinderpal mentioned this pull request Nov 27, 2019

REQUEST: New membership for Arvinderpal kubernetes/org#1443

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubeadm Networking Configuration E2E Tests #80259

Kubeadm Networking Configuration E2E Tests #80259

Arvinderpal commented Jul 17, 2019 •

edited

Loading

k8s-ci-robot commented Jul 17, 2019

k8s-ci-robot commented Jul 17, 2019

Arvinderpal commented Jul 17, 2019 •

edited

Loading

yastij commented Jul 17, 2019

yastij commented Jul 17, 2019

neolit123 commented Jul 17, 2019

fabriziopandini left a comment

fabriziopandini Jul 18, 2019

Arvinderpal Jul 18, 2019

fabriziopandini Jul 22, 2019

Arvinderpal Jul 23, 2019 •

edited

Loading

fabriziopandini Jul 25, 2019

Arvinderpal commented Jul 18, 2019

aojea commented Jul 20, 2019

fabriziopandini commented Jul 25, 2019

Arvinderpal commented Jul 25, 2019

rosti left a comment

rosti Jul 26, 2019

Arvinderpal Jul 27, 2019

Arvinderpal commented Jul 27, 2019

neolit123 commented Jul 28, 2019

neolit123 left a comment

k8s-ci-robot commented Jul 28, 2019

fejta-bot commented Jul 28, 2019

Kubeadm Networking Configuration E2E Tests #80259

Kubeadm Networking Configuration E2E Tests #80259

Conversation

Arvinderpal commented Jul 17, 2019 • edited Loading

k8s-ci-robot commented Jul 17, 2019

k8s-ci-robot commented Jul 17, 2019

Arvinderpal commented Jul 17, 2019 • edited Loading

yastij commented Jul 17, 2019

yastij commented Jul 17, 2019

neolit123 commented Jul 17, 2019

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Jul 18, 2019

Choose a reason for hiding this comment

Arvinderpal Jul 18, 2019

Choose a reason for hiding this comment

fabriziopandini Jul 22, 2019

Choose a reason for hiding this comment

Arvinderpal Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Jul 25, 2019

Choose a reason for hiding this comment

Arvinderpal commented Jul 18, 2019

aojea commented Jul 20, 2019

fabriziopandini commented Jul 25, 2019

Arvinderpal commented Jul 25, 2019

rosti left a comment

Choose a reason for hiding this comment

rosti Jul 26, 2019

Choose a reason for hiding this comment

Arvinderpal Jul 27, 2019

Choose a reason for hiding this comment

Arvinderpal commented Jul 27, 2019

neolit123 commented Jul 28, 2019

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 28, 2019

fejta-bot commented Jul 28, 2019

Arvinderpal commented Jul 17, 2019 •

edited

Loading

Arvinderpal commented Jul 17, 2019 •

edited

Loading

Arvinderpal Jul 23, 2019 •

edited

Loading