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

split verify out of unit/integration suite #27289

Merged
merged 2 commits into from
Jun 17, 2016

Conversation

mikedanese
Copy link
Member

"$@"
}

# Runs the unit and integration tests, producing JUnit-style XML test
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment is outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@mikedanese
Copy link
Member Author

@k8s-bot unit test this please, issue: #27290

@spxtr
Copy link
Contributor

spxtr commented Jun 13, 2016

You'll also need to do this for the post-commit jobs, and then add the new job to merge-blockers in the contrib repo. It's a pain, I know.

Also might be worth doing similar with the integration tests, since those take 15-20 min.

@wojtek-t
Copy link
Member

@mikedanese - thanks for working on it.

Also might be worth doing similar with the integration tests, since those take 15-20 min.

This is not worth doing for now in my opinion. Currently, e2e tests take ~35 minutes. After splitting verify-all to another job, both tests and verify will be shorter than that. So splitting integration tests for now will not improve merging throughput.

@mikedanese mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 14, 2016
@mikedanese
Copy link
Member Author

@ixdy @fejta @spxtr this will cause the verify scripts to only test run ./hack/verify-all.sh. Now that the testinfra PR is merged, this is ready to merge.

@mikedanese mikedanese changed the title WIP: split verify out of unit/integration suite split verify out of unit/integration suite Jun 15, 2016
export PATH=${GOPATH}/bin:${PWD}/third_party/etcd:/usr/local/go/bin:${PATH}

retry go get github.com/tools/godep && godep version
retry go get github.com/jstemmer/go-junit-report
Copy link
Member

Choose a reason for hiding this comment

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

we don't need go-junit-report for verification checks

@mikedanese mikedanese force-pushed the split-verify branch 2 times, most recently from 9c55786 to 0610b3c Compare June 15, 2016 19:43
@mikedanese
Copy link
Member Author

@ixdy, PTAL

@mikedanese mikedanese assigned spxtr and ixdy and unassigned mikedanese Jun 15, 2016
@ixdy
Copy link
Member

ixdy commented Jun 15, 2016

Should we update test-dockerized.sh too, since we've enabled the verification job on Jenkins already?

(Not sure if SQ is looking at that result yet.)

@mikedanese
Copy link
Member Author

SQ is not requiring verify to pass. I want to make sure this isn't broken before making it a pr blocker. If I'm being too cautious let me know and we can fast path this.

@ixdy
Copy link
Member

ixdy commented Jun 15, 2016

The verify job appears to be generally passing, so it's probably OK to make it a blocker already.

We'll also know if your changes here work, since Jenkins is using your PR to run the tests.

@ixdy
Copy link
Member

ixdy commented Jun 15, 2016

So yes, I think we should
a) make SQ look at both results (and make both required in GitHub)
b) remove verify from unit/integration in this PR
c) assuming all checks here pass, merge

@ixdy
Copy link
Member

ixdy commented Jun 16, 2016

@spxtr it failed again, so maybe?

+ curl -fsS --retry 3 https://raw.githubusercontent.com/kubernetes/kubernetes/master/hack/jenkins/upload-to-gcs.sh
/tmp/hudson753422643669560184.sh: line 3: ./_tmp/upload-to-gcs.sh: Permission denied

@ixdy
Copy link
Member

ixdy commented Jun 16, 2016

Oh, I think I know why it's failing, and it's the stupidest thing.

verify-swagger-spec.sh creates _tmp, and it normally deletes it, but it doesn't trap this cleanup. Since this script failed due to etcd not being installed, it left _tmp around. And because this is running in docker, _tmp was owned by root, so Jenkins couldn't write to it.

@spxtr
Copy link
Contributor

spxtr commented Jun 16, 2016

That's brilliant.

Unfortunately, when that download fails, we don't get build logs.

@mikedanese mikedanese force-pushed the split-verify branch 2 times, most recently from 9f71608 to 1661a77 Compare June 16, 2016 17:07
@mikedanese
Copy link
Member Author

This now depends on kubernetes-retired/contrib#1222 being merged and deployed

@mikedanese
Copy link
Member Author

Now looks like a new govet verifier is failing but I can't reproduce locally:

Verifying ./hack/../hack/verify-govet.sh
pkg/kubelet/rkt/rkt_test.go:84: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:130: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:144: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:851: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:982: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1040: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1103: appctypes.Exec composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1815: appctypes.Annotations composite literal uses unkeyed fields
pkg/kubelet/rkt/rkt_test.go:1827: appctypes.Annotations composite literal uses unkeyed fields
FAILED   ./hack/../hack/verify-govet.sh 133s

@@ -29,14 +29,14 @@ API_REFERENCE_DOCS_ROOT="${KUBE_ROOT}/docs/api-reference"
OUTPUT_DIR="${KUBE_ROOT}/_tmp/api-reference"
mkdir -p ${OUTPUT_DIR}
TMP_ROOT="${KUBE_ROOT}/_tmp"
trap "rm -rf ${TMP_ROOT}" EXIT SIGINT
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to TMP_ROOT=$(mktemp -d) instead? It would give us better guarantee that it won't collide again (process could die for lots of different reasons beside EXIT and SIGINT)

mktemp(1) is not POSIX, but I understand that it's not a problem :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of @ixdy's commit, but what we have here is a strict improvement

@mikedanese mikedanese force-pushed the split-verify branch 3 times, most recently from 7522396 to 21240ff Compare June 17, 2016 00:36
@ixdy
Copy link
Member

ixdy commented Jun 17, 2016

LGTM. I love how quickly the verification pass finished. :)

@ixdy ixdy added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2016
@ixdy
Copy link
Member

ixdy commented Jun 17, 2016

actually, I guess it wasn't that fast. still. better.

@@ -24,6 +24,8 @@ source "${KUBE_ROOT}/hack/lib/init.sh"

cd "${KUBE_ROOT}"

go install ./cmd/...
Copy link
Member

Choose a reason for hiding this comment

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

actually, one nit (somehow missed this) - might be worth adding a comment why this is necessary?

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.

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2016

GCE e2e build/test failed for commit 21240ffb17d80e33e735134274c6db2fa94336ab.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@@ -24,6 +24,10 @@ source "${KUBE_ROOT}/hack/lib/init.sh"

cd "${KUBE_ROOT}"

# This is required before we run govet for the results to be correct.
# See https://github.com/golang/go/issues/16086 for details.
go install ./cmd/...
Copy link
Member

Choose a reason for hiding this comment

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

fun: we don't need to fix this in this PR, but I think building only cmd/... is causing us to miss some stuff:

$ time go list ./... | grep -v 'third_party\|vendor' | xargs go install

real    4m2.226s
user    20m37.690s
sys     1m45.396s
$ time go list ./... | grep -v 'third_party\|vendor' | xargs go vet
federation/pkg/dnsprovider/providers/aws/route53/testing/route53api.go:79: arg change.ResourceRecordSet.Name for printf verb %s of wrong type: *string
federation/pkg/dnsprovider/providers/aws/route53/testing/route53api.go:84: arg change.ResourceRecordSet.Name for printf verb %s of wrong type: *string
federation/pkg/dnsprovider/providers/aws/route53/testing/route53api.go:105: arg input.Name for printf verb %s of wrong type: *string
exit status 1
federation/pkg/dnsprovider/providers/google/clouddns/clouddns.go:110: k8s.io/kubernetes/federation/pkg/dnsprovider/providers/google/clouddns/internal/stubs.ManagedZone composite literal uses unkeyed fields
exit status 1

real    1m39.439s
user    1m52.376s
sys     0m10.650s

Copy link
Member Author

Choose a reason for hiding this comment

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

go install ./... doesn't even work

$ go install ./...
# k8s.io/kubernetes/vendor/github.com/Microsoft/go-winio
vendor/github.com/Microsoft/go-winio/file.go:45: undefined: syscall.Overlapped

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that's why I did go list ./... and grep'd out vendor and third_party.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2016
@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2016
@mikedanese mikedanese merged commit 2c93ea5 into kubernetes:master Jun 17, 2016
@mikedanese mikedanese deleted the split-verify branch June 17, 2016 02:37
@k8s-bot
Copy link

k8s-bot commented Jun 17, 2016

GCE e2e build/test passed for commit ee34c76.

mikedanese added a commit to mikedanese/kubernetes that referenced this pull request Feb 4, 2020
I have 140 commits in this directory and I get a lot of cleanup reviews
and want to be able to approve changes to hack/.golint_failures.

0e69316 delete unused cache
b9c7007 enable token review when openapi is generated
d5bbc35 make deps-approvers the approvers of sample-cli-plugin/Godeps
4186abf bzl: fix update-bazel.sh
7b47229 remove deprecated /proxy paths
b973840 gke-certificates-controller: rm -rf
4961065 cluster: remove unused functions
1e2b644 cluster: move logging library to hack/
bef68f7 cluster: build gci mounter like other go binaries
fe7ba9e kubeadm: use kubelet bootstrap instead of reimplementing
3c39173 fixit: break sig-cluster-lifecycle tests into subpackage
64f77eb enable race detection on integration tests
cdcfa35 promote tls-bootstrap to beta
ff4a814 migrate set generation to go genrule
3600d49 delete benchmark integration tests that don't work at all
21617a6 don't use build tags to mark integration tests
59fc948 bump rules_go and go version for bazel builds
ba5c285 bazel: implement git build stamping
ad42b42 move kubeadm api group testing to kubeadm package
c8ce55f Revert "Merge pull request kubernetes#41132 from kubernetes/revert-40893-kubelet-auth"
cbe5bd9 bump gazel to v14
86d9493 remove second CA used for kubelet auth in favor of webhook auth
04a7880 update repo local config to allow redirects from gopkg.in
44b7246 autogenerated
96c146c promote certificates.k8s.io to beta
087016d update gazel to v8
837eee4 pin gazel to v3
e225625 add a configuration for kubelet to register as a node with taints
584689f implement kubectl procelain csr commands
93f737e fix verify-bazel.sh on mac and windows
5dc7554 bazel: implement set-gen as a bazel genrule
61bd6aa remove docs/user-guide from bindata search path
224e32b make godep licenses/copyright check case insensitive
1cd2968 godep: vendor go-bindata
d380cb1 fix realpath issue on mac
ea632fa Revert "disable bazel build"
27116c6 rename build/ to build-tools/
ee15c80 disable bazel build
999c967 ignore BUILD in the flags-underscore.py validation
b250a88 don't check BUILD file when verifying godeps
a2eec91 add bazel presubmits to verify BUILD files are up to date
c17a8a7 kubectl: apply prune should fallback to basic delete when a resource has no reaper
25e4dcc kubeadm: fix conversion macros and add kubeadm to round trip testing
6d17a87 kubectl: add two more test of kubectl apply --prune
62960aa add a test for kubectl apply --prune
6339d91 add a test to test-cmd.sh for apply -f with label selector
b421bf4 build kube-discovery and kubeadm with release
0c76cf5 fix hack/verify-codegen.sh
9f379df add an option to controller-manager to auto approve all CSRs
95e2e29 move kube-dns to the cluster/addons/ directory
f3de21b move integration tests into individual pacakges
af0177e cleanup hack/verify-govet.sh to throttle process creation
2c93ea5 Merge pull request kubernetes#27289 from mikedanese/split-verify
ee34c76 split verify out of unit/integration suite
d046275 now that go test runs iteration loops, use that instead of custom executor
1ef1906 Merge pull request kubernetes#26197 from wonderfly/update_default_master_image
fbf6bbc Merge pull request kubernetes#25596 from derekparker/inotify
3e1c0b5 run kube-addon-manager in a pod
c5cc0c3 Merge pull request kubernetes#24277 from ihmccreery/upgrade-timeout
132c427 add linux fastbuild option to ./build/release.sh
2857baa use defaults in test-dockerized for etcd prefix and api versions
695211e Merge pull request kubernetes#21105 from caesarxuchao/watchCacheForIntegration
2172e0d Merge pull request kubernetes#21108 from mml/slow-flake
1478cf3 Merge pull request kubernetes#21090 from ihmccreery/feature-reboot
b3172a4 kubelet: add a pidfile
b1743a6 this is a manual reversion of kubernetes#20702
5b27055 Merge pull request kubernetes#19378 from ihmccreery/remove-update-jobs
b743827 Merge pull request kubernetes#19659 from ihmccreery/timeout-reboot
a6589f7 hack: ignore cluster/env.sh in boilerplate check
f71657d retrofit the scheduler with the leader election client.
bf763bb Merge pull request kubernetes#19498 from pwittrock/nodelabels
22cfa5e build: move some of hack/lib/ into a new cluster/lib/
b174fc9 Merge pull request kubernetes#18994 from bprashanth/flannel_suite
a09d85b expose master count configuration in a cli option on apiserver
c2753d7 bump ci go version to 1.5.2
0655e65 fall back to old behavior when deciding mem availablity during build
1d9d11c run kube-proxy in a static pod
91de3a1 cleanup some nits in hack/get-build.sh
cd79c6c fix unbound variable error in hace/get-build.sh
5e64590 renable enable var to correct name and only use it when needed
9bdb860 add apigroup installer and tests
e6d3b47 add componentconfig api group to autogen stuff
88008de Merge pull request kubernetes#16459 from mikedanese/enable-exp
d28d134 Merge pull request kubernetes#16533 from ihmccreery/upgrade-test-fixes
3343522 enable deployment and daemonset in gce upgrade tests
7cbf249 Merge pull request kubernetes#15836 from wojtek-t/codecgen_from_godeps
92404e7 add upgrade test between 1.0 and 1.1 for gce
95b8394 Merge pull request kubernetes#15861 from mikedanese/upgrade-num-minion
ece5779 increase NUM_MINIONS for jenkins gce upgrade test
b8b35af actually promote daemonset simple test out of flaky and skip all daemonset tests in gke
d379a36 copy directory not contents of directory
402e68e add slow test for terminated pod garbage collection
c0943f1 add intermediate e2e runs to gce upgrade
10d56ff promote simple daemonset test out of flaky
b635fc5 Merge pull request kubernetes#15228 from mesosphere/sttts-conformance-tags
392f33e Merge pull request kubernetes#14054 from mikedanese/register-master
fa60bbe add flag to kubelet to ignore the cidr passed down by the apiserver on the master
53e14c7 diff all of pkg/ when verifying swagerspec instead of just pkg/api/
05ef8ed Merge pull request kubernetes#15104 from mikedanese/ds-e2e
fe820fc break up daemonset test into two tests
833be48 enable all experimental flags with one controller
905e971 be explicit about minion group size in upgrade test
ae7d3d5 add gce-upgrade to jenkins/e2e.sh
376faea add pod garbage collection
b0457be Merge pull request kubernetes#13058 from mvdan/go1.5
a48f218 Merge pull request kubernetes#13754 from tummychow/labels-deps
1fec199 Merge pull request kubernetes#13824 from kubernetes/revert-13547-hpa-kubeup
fa40ced move contrib/for-tests to test/images
f061875 updating all references in .sh scripts
8326697 rewrite all links to prs to k8s links
fb02b33 fix build
8e48431 Revert "demote to flaky tests from parallel e2e"
b56edd1 Merge pull request kubernetes#11727 from ZJU-SEL/build-nonstatic-hyperkube
cf4cb1a Merge pull request kubernetes#10474 from kargakis/scale-multiple-controllers
e376a09 demote to flaky service tests from parallel e2e
7c47d6b Merge pull request kubernetes#12009 from smarterclayton/fix_cmd_config
0269e2b Merge pull request kubernetes#11941 from GoogleCloudPlatform/enact_version_md
94a387d Revert "Improve conversion to support multiple packages"
1a613c4 Merge pull request kubernetes#9971 from smarterclayton/make_conversion_more_flexible
0ae48c4 Merge pull request kubernetes#11927 from wojtek-t/remove_shell_services
59a1dd4 Merge pull request kubernetes#11789 from mbforbes/nodesNetwork
6294070 Merge pull request kubernetes#11803 from wojtek-t/move_back_from_flaky
daa6d4d Merge pull request kubernetes#11285 from liggitt/ca
9f16fd9 Merge pull request kubernetes#11860 from ingvagabund/delimiter-for-X-option-eparis
c0acfbc Merge pull request kubernetes#11421 from nikhiljindal/exposeServcPort
ae1c8e5 Merge pull request kubernetes#11737 from thockin/cleanup-remove-v1beta3
01ee1b8 Merge pull request kubernetes#10840 from jbeda/master
d4d99de make mungedoc exit 1 if manual changes are needed and wire up erro message.
337772a fix all tests
055115a fake realpath, and standardize treatment of trailing / of dirs in gendoc
b4514ee fix run-gendocs to point to new repo location
c053b9a add documentation and script on how to get recent and "nightly" builds
719870f add publishing of latest-green.txt to jenkins e2e tests on success
1e130e0 remove --machines from code and docs
dbb47fe remove e2e run before cluster upgrade
de55e17 e2e test cluster stability during upgrade
c9fcf45 fix bad cmd-test for patch.
9f91532 fix error where we can't use patch and add cmd-test for patch and file update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants