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

Replace minikube for e2e tests #2863

Merged
merged 1 commit into from
Jul 28, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Jul 28, 2018

What this PR does / why we need it:

Replaces minikube with https://github.com/kubernetes-sigs/kubeadm-dind-cluster to build and run e2e tests.
Also, we update the slow threshold from 5 seconds to 40. This value is more real and can help to detect slow tests.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 28, 2018
@aledbf aledbf added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2018
@aledbf
Copy link
Member Author

aledbf commented Jul 28, 2018

@antoineco this approach can help us to run the tests in different cloud providers like gce

@aledbf aledbf force-pushed the replace-minikube branch 10 times, most recently from 0e3e171 to 80ee721 Compare July 28, 2018 03:49
@aledbf
Copy link
Member Author

aledbf commented Jul 28, 2018

With this change, we can also skip minikube locally running ./test/e2e/up.sh to start a dind cluster.

@codecov-io
Copy link

codecov-io commented Jul 28, 2018

Codecov Report

Merging #2863 into master will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
+ Coverage   47.71%   47.84%   +0.12%     
==========================================
  Files          75       75              
  Lines        5434     5434              
==========================================
+ Hits         2593     2600       +7     
+ Misses       2510     2504       -6     
+ Partials      331      330       -1
Impacted Files Coverage Δ
cmd/nginx/main.go 11.18% <0%> (+4.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e2cc65...7032fb7. Read the comment docs.

@aledbf aledbf force-pushed the replace-minikube branch from 80ee721 to 352a6d9 Compare July 28, 2018 04:26
@aledbf aledbf removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2018
"vendor",
"test/e2e/framework/framework.go",
"test/e2e/generated/bindata.go",
"hack/boilerplate/test",
Copy link
Contributor

@antoineco antoineco Jul 28, 2018

Choose a reason for hiding this comment

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

Bindata files are gone.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -26,7 +26,9 @@ GOHOSTOS ?= $(shell go env GOHOSTOS)
# Allow limiting the scope of the e2e tests. By default run everything
FOCUS ?= .*
# number of parallel test
E2E_NODES ?= 3
E2E_NODES ?= 4
Copy link
Contributor

Choose a reason for hiding this comment

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

?= is evil, it allows for empty strings. := is safer.

Copy link
Member Author

Choose a reason for hiding this comment

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

most of the variables could be replaced by env variables, using := avoids that. Also, the scripts we call check for the required variables

Copy link
Contributor

Choose a reason for hiding this comment

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

:= should handle env vars as well. What version of make do you have installed?

Copy link
Member Author

Choose a reason for hiding this comment

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

$ make -v
GNU Make 4.1
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

@@ -34,6 +34,10 @@ if [ -z "${NODE_IP}" ]; then
echo "NODE_IP must be set"
exit 1
fi
if [ -z "${SLOW_E2E_THRESHOLD}" ]; then
Copy link
Contributor

@antoineco antoineco Jul 28, 2018

Choose a reason for hiding this comment

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

Suggestion:

declare -a mandatory
mandatory=(
  NODE_IP
  SLOW_E2E_THRESHOLD
  ...
)

for m in ${mandatory[@]}; do
  if [ -z "${!m}" ]; then
    echo "$m must be set"
    declare missing
  fi
done

if [ -n $missing ]; then
  exit 1
fi

Would show all missing variables at once.

@aledbf aledbf force-pushed the replace-minikube branch 3 times, most recently from 015bca3 to 1fd6ea0 Compare July 28, 2018 12:47
@@ -63,6 +63,7 @@ PWD=${PWD}
BUSTED_ARGS=${BUSTED_ARGS:-""}
REPO_INFO=${REPO_INFO:-local}
NODE_IP=${NODE_IP:-127.0.0.1}
SLOW_E2E_THRESHOLD=${SLOW_E2E_THRESHOLD:-50}
Copy link
Member

Choose a reason for hiding this comment

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

any reason why is this 50 here but 40 in the Makefile?

@ElvinEfendi
Copy link
Member

@aledbf could you describe with a few words why is this new tool better than minikube in our case? (I'm not experienced in either of them)

@antoineco
Copy link
Contributor

I can comment here: I have been using Docker for Mac for a while so I appreciate having batteries included (local dev script) but swappable (I can run e2e in my own setup easily without seeing failures when minikube is absent).

Also minikube is tough to run in unprivileged environments (most CI systems I use don't allow sudo).

@aledbf aledbf force-pushed the replace-minikube branch from 1fd6ea0 to f516dff Compare July 28, 2018 14:32
@aledbf
Copy link
Member Author

aledbf commented Jul 28, 2018

Also, this approach provides a more real cluster, one master, and two workers use just docker and don't require a VM, which means no Virtualbox or QEMU.

@aledbf
Copy link
Member Author

aledbf commented Jul 28, 2018

why is this new tool better than minikube in our case

Not sure (yet) if it's better or not but for sure it's simpler.

build/build.sh Outdated
fi
done

if $missing ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Bash doesn't know about booleans, does it? This may always match.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@aledbf aledbf force-pushed the replace-minikube branch from f516dff to 7032fb7 Compare July 28, 2018 15:05
@antoineco
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, antoineco

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 45ba167 into kubernetes:master Jul 28, 2018
@aledbf aledbf deleted the replace-minikube branch July 28, 2018 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants