fix kinder smoke test #2118

prasadkatti · 2020-04-27T01:46:59Z

Replace kubectl run with kubectl create deployment
Wait for service to be reachable before trying to access it

k8s-ci-robot · 2020-04-27T01:47:07Z

Hi @prasadkatti. 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.

fabriziopandini

@prasadkatti thanks for this PR

kinder/pkg/cluster/manager/actions/smoke-test.go

fabriziopandini · 2020-04-27T08:32:14Z

/ok-to-test

neolit123 · 2020-04-29T01:03:17Z

kinder/pkg/cluster/manager/actions/waiter.go

+// nodePortIsReady implements a function that tests if a nodePort is ready
+func nodePortIsReady(n *status.Node, port string) func(c *status.Cluster, n *status.Node) bool {
+	return func(c *status.Cluster, n *status.Node) bool {
+		fmt.Printf("checking node port %s on node %s...\n", port, n.Name())


i think we should remove this call.
and only leave the call on line 289. it prints only on a positive outcome (return true)

kinder/pkg/cluster/manager/actions/waiter.go

neolit123 · 2020-04-29T01:13:28Z

kinder/pkg/cluster/manager/actions/smoke-test.go

-	time.Sleep(3 * time.Second)
-
-	err = checkNodePort(c, nodePort)
+	err = waitForNodePort(c, cp1, 30*time.Second, nodePort)


the old code iterated over all the nodes.

so this should be:

for _, n := range c.K8sNodes() { err = waitForNodePort(c, n, wait, nodePort) if err != nil { return err } }

this makes wait is a bit odd in the context of SmokeTest() as a whole, because it is now used in a couple of "wait" locations, so it can wait for a maximum of wait * 2

one wait of handling that is passing wait / 2 to the 2 wait* calls.
it's default value is 5 minutes:

kubeadm/kinder/cmd/kinder/do/do.go

Line 104 in cf387fe

"wait", time.Duration(5*time.Minute),

I just kept the timeout to 30s for each of those calls. From what I saw the NodePort svc is ready within a couple of seconds. So the shorter timeout will help to fail fast.
The cluster that you get with when following the getting started guide has 3 nodes.

How difficult would it be to pass multiple try funcs to the waitFor so that this happens in parallel?

waitFor already supports this, but is a bit difficult as this is the sequence ATM:

create deployment

wait for pod

expose port

wait for port

if we swap "expose port" / "wait for pod" the pod might not be ready yet and expose will fail.
so i'd say leave it the way it is.

prasadkatti · 2020-04-29T05:16:52Z

I had to change the label used to search for the pod from run=nginx to app=nginx. This was a result of changing kubectl run to kubectl create deployment.

fabriziopandini · 2020-04-29T07:59:49Z

/lgtm
/approve

@prasadkatti please squash commits, after that I will lift the hold
/hold

Replace `kubectl run` with `kubectl create deployment` Wait for service to be reachable before trying to access it Add a waiter to wait for NodePort to be ready

prasadkatti · 2020-04-29T16:00:17Z

@fabriziopandini - I have squashed the commits and force-pushed.

prasadkatti · 2020-04-29T16:21:21Z

/retest

neolit123 · 2020-04-29T17:41:38Z

the previous failure in pull-kubeadm-kinder-upgrade-master is a flake in pulling artifacts, so this seems fine.

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2020-04-29T17:41:54Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~kinder/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

prasadkatti · 2020-04-29T18:32:56Z

@neolit123 @fabriziopandini - Do we want to add smoke-test to ci?

neolit123 · 2020-04-29T18:40:49Z

i think its not a bad idea.
should be added above this line https://github.com/kubernetes/kubeadm/blob/master/kinder/ci/workflows/presubmit-upgrade-master.yaml#L117

fabriziopandini · 2020-04-29T19:40:46Z

Smoke tests are more for the Devs than for Ci; in CI we are running the compliance suite and this is far more reliable than smoke tests, so IMO it is not necessary.

neolit123 · 2020-04-29T19:46:34Z

the only benefit would be to catch regressions in the smoke test, but i see your point.

fabriziopandini · 2020-04-29T19:50:44Z

What about having smoke test running in tests that are not required to do a full e2e (so use it as a light-weight alternative to e2e)

neolit123 · 2020-04-29T20:01:51Z

true, a separate CI job seems better.

fabriziopandini · 2020-04-29T20:04:14Z

I was thinking at existing jobs not running e2e (e.g Kustomize if I remember well)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 27, 2020

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 27, 2020

k8s-ci-robot requested review from fabriziopandini and kad April 27, 2020 01:47

prasadkatti force-pushed the master branch from df646f6 to 21eb717 Compare April 27, 2020 06:51

prasadkatti changed the title ~~try to fix the smoke test~~ fix kinder smoke test Apr 27, 2020

fabriziopandini requested changes Apr 27, 2020

View reviewed changes

fabriziopandini reviewed Apr 27, 2020

View reviewed changes

kinder/pkg/cluster/manager/actions/smoke-test.go Outdated Show resolved Hide resolved

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 Apr 27, 2020

prasadkatti force-pushed the master branch from 4a981bd to c3ae385 Compare April 27, 2020 20:19

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 29, 2020

neolit123 reviewed Apr 29, 2020

View reviewed changes

kinder/pkg/cluster/manager/actions/waiter.go Outdated Show resolved Hide resolved

neolit123 reviewed Apr 29, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020

k8s-ci-robot assigned fabriziopandini Apr 29, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 29, 2020

Fix kinder smoke-test

dd73c08

Replace `kubectl run` with `kubectl create deployment` Wait for service to be reachable before trying to access it Add a waiter to wait for NodePort to be ready

prasadkatti force-pushed the master branch from 35ab2a2 to dd73c08 Compare April 29, 2020 15:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020

k8s-ci-robot assigned neolit123 Apr 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2020

k8s-ci-robot merged commit 03018bb into kubernetes:master Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix kinder smoke test #2118

fix kinder smoke test #2118

prasadkatti commented Apr 27, 2020 •

edited

Loading

k8s-ci-robot commented Apr 27, 2020

fabriziopandini left a comment •

edited

Loading

fabriziopandini commented Apr 27, 2020

neolit123 Apr 29, 2020

prasadkatti Apr 29, 2020

neolit123 Apr 29, 2020

neolit123 Apr 29, 2020 •

edited

Loading

prasadkatti Apr 29, 2020

prasadkatti Apr 29, 2020

neolit123 Apr 29, 2020

prasadkatti commented Apr 29, 2020

fabriziopandini commented Apr 29, 2020

prasadkatti commented Apr 29, 2020

prasadkatti commented Apr 29, 2020

neolit123 commented Apr 29, 2020 •

edited

Loading

k8s-ci-robot commented Apr 29, 2020

prasadkatti commented Apr 29, 2020

neolit123 commented Apr 29, 2020

fabriziopandini commented Apr 29, 2020

neolit123 commented Apr 29, 2020

fabriziopandini commented Apr 29, 2020 •

edited

Loading

neolit123 commented Apr 29, 2020 •

edited

Loading

fabriziopandini commented Apr 29, 2020

fix kinder smoke test #2118

fix kinder smoke test #2118

Conversation

prasadkatti commented Apr 27, 2020 • edited Loading

k8s-ci-robot commented Apr 27, 2020

fabriziopandini left a comment • edited Loading

Choose a reason for hiding this comment

fabriziopandini commented Apr 27, 2020

neolit123 Apr 29, 2020

Choose a reason for hiding this comment

prasadkatti Apr 29, 2020

Choose a reason for hiding this comment

neolit123 Apr 29, 2020

Choose a reason for hiding this comment

neolit123 Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

prasadkatti Apr 29, 2020

Choose a reason for hiding this comment

prasadkatti Apr 29, 2020

Choose a reason for hiding this comment

neolit123 Apr 29, 2020

Choose a reason for hiding this comment

prasadkatti commented Apr 29, 2020

fabriziopandini commented Apr 29, 2020

prasadkatti commented Apr 29, 2020

prasadkatti commented Apr 29, 2020

neolit123 commented Apr 29, 2020 • edited Loading

k8s-ci-robot commented Apr 29, 2020

prasadkatti commented Apr 29, 2020

neolit123 commented Apr 29, 2020

fabriziopandini commented Apr 29, 2020

neolit123 commented Apr 29, 2020

fabriziopandini commented Apr 29, 2020 • edited Loading

neolit123 commented Apr 29, 2020 • edited Loading

fabriziopandini commented Apr 29, 2020

prasadkatti commented Apr 27, 2020 •

edited

Loading

fabriziopandini left a comment •

edited

Loading

neolit123 Apr 29, 2020 •

edited

Loading

neolit123 commented Apr 29, 2020 •

edited

Loading

fabriziopandini commented Apr 29, 2020 •

edited

Loading

neolit123 commented Apr 29, 2020 •

edited

Loading