KUBE_JUNIT_REPORT_DIR fixes #73581

krzysied · 2019-01-31T10:44:20Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR fixes k8s.io/kubernetes/hack/jenkins/benchmark-dockerized.sh script.

If KUBE_JUNIT_REPORT_DIR is provided, benchmark-dockerized.sh script should ensure that there is go-junit-report installed.
KUBE_JUNIT_REPORT_DIR is nor provided, k8s.io/kubernetes/hack/make-rules/test.sh should write log to sdtout.

Which issue(s) this PR fixes:
ref #72363

Special notes for your reviewer:
reverting some changes from #72218

Does this PR introduce a user-facing change?:

NONE

krzysied · 2019-01-31T10:46:10Z

/hold
@cblecker I'm reverting some changes from PR. PTAL

krzysied · 2019-01-31T10:56:15Z

/priority important-soon
/sig testing

fejta · 2019-01-31T19:50:25Z

hack/jenkins/benchmark-dockerized.sh

@@ -34,6 +34,9 @@ retry() {
 export PATH=${GOPATH}/bin:${PWD}/third_party/etcd:/usr/local/go/bin:${PATH}

 retry go get github.com/cespare/prettybench
+if [[ -n "${KUBE_JUNIT_REPORT_DIR:-}" ]]; then
+  go get github.com/jstemmer/go-junit-report


Is this (and prettybench) vendored? If not they should be.

We should not be downloading code (flaky and/or disapper) during test execution.

I agree with @fejta. Also not sure testing on this is right.. we either call go-junit-report or we don't.. I don't think that hinges on if we have the report dir set, does it? If it does, can you link to that?

About vendoring prettybench, I agree with @fejta.
However, I would like to focus on fixing hack/jenkins/benchmark-dockerized.sh first.
I can file an issue about downloading golang modules instead of vendoring it. For example go-junit-report is also downloaded in hack/jenkins/gotest.sh and hack/jenkins/test-dockerized.sh.

If benchmark-dockerized.sh needs go-junit-report, it should go install the vendored version like our other utilities do:

kubernetes/hack/update-bazel.sh

Line 28 in 27d4958

go install k8s.io/kubernetes/vendor/github.com/bazelbuild/bazel-gazelle/cmd/gazelle

As long as we're fixing this, we should also fix prettybench to be a go install command rather than go get, but feel free to punt on that if you'd prefer.

I created separate PR #73740 to add go-junit-report and prettybench to the vendor.

Thanks! Gave that one a review.

cblecker · 2019-01-31T22:47:16Z

hack/make-rules/test.sh

@@ -120,10 +120,6 @@ KUBE_TEST_API_VERSIONS="${KUBE_TEST_API_VERSIONS:-${ALL_VERSIONS_CSV}}"
 # once we have multiple group supports
 # Create a junit-style XML test report in this directory if set.
 KUBE_JUNIT_REPORT_DIR=${KUBE_JUNIT_REPORT_DIR:-}
-# If KUBE_JUNIT_REPORT_DIR is unset, and ARTIFACTS is set, then have them match.


I don't understand why this is being removed.

Took me some time, but I think I finally understood what's the issue.
The problem is cause by

kubernetes/hack/make-rules/test.sh

Line 185 in a3c14ec

if [[ -n "${KUBE_JUNIT_REPORT_DIR}" ]] ; then

and to be more precise

kubernetes/hack/make-rules/test.sh

Line 188 in a3c14ec

go_test_grep_pattern="^[^[:space:]]\+[[:space:]]\+[^[:space:]]\+/[^[[:space:]]\+"

When junit reporter is enabled, it filters out most of the logs.
I see two possible fixes:

remove setting KUBE_JUNIT_REPORT_DIR to ARTIFACTS as default

remove this go_test_grep_pattern that filters out logs

Hmm, we probably don't want to remove this, as this is an important system in our CI system.

If ARTIFACTS is also unset, then this won't be set (the local use case).

This is important for CI and shouldn't be removed (agree with @fejta).
/lgtm cancel

Ok, so lets proceed then with changing go_test_grep_pattern.
I added flag that will force printing full log. hack/jenkins/benchmark-dockerized.sh will set this flag, get full log and convert it into benchmark. At the same time, there should be no impact on your CI.
Wdyt?

cblecker · 2019-01-31T22:49:17Z

hack/jenkins/benchmark-dockerized.sh

@@ -34,6 +34,9 @@ retry() {
 export PATH=${GOPATH}/bin:${PWD}/third_party/etcd:/usr/local/go/bin:${PATH}

 retry go get github.com/cespare/prettybench
+if [[ -n "${KUBE_JUNIT_REPORT_DIR:-}" ]]; then
+  go get github.com/jstemmer/go-junit-report


I agree with @fejta. Also not sure testing on this is right.. we either call go-junit-report or we don't.. I don't think that hinges on if we have the report dir set, does it? If it does, can you link to that?

cblecker · 2019-01-31T22:49:32Z

/assign

krzysied · 2019-02-05T13:57:26Z

/hold
Blocked until #73740 is merged. [Done]

krzysied · 2019-02-06T11:12:16Z

/hold cancel
#73740 is merged. PR rebased.

fejta

/lgtm
/approve

fejta

/hold

fejta · 2019-02-06T22:41:25Z

hack/make-rules/test.sh

@@ -120,10 +120,6 @@ KUBE_TEST_API_VERSIONS="${KUBE_TEST_API_VERSIONS:-${ALL_VERSIONS_CSV}}"
 # once we have multiple group supports
 # Create a junit-style XML test report in this directory if set.
 KUBE_JUNIT_REPORT_DIR=${KUBE_JUNIT_REPORT_DIR:-}
-# If KUBE_JUNIT_REPORT_DIR is unset, and ARTIFACTS is set, then have them match.


Hmm, we probably don't want to remove this, as this is an important system in our CI system.

fejta

/lgtm
/approve

fejta · 2019-02-08T06:56:05Z

/hold cancel

k8s-ci-robot · 2019-02-08T06:56:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, krzysied

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:

~~hack/OWNERS~~ [fejta]

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

krzysied · 2019-02-08T09:35:17Z

/retest

k8s-ci-robot requested review from fejta and ixdy January 31, 2019 10:45

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

krzysied force-pushed the test_KUBE_JUNIT_REPORT_DIR_revert branch from 0b64682 to feec2c1 Compare January 31, 2019 10:52

fejta reviewed Jan 31, 2019

View reviewed changes

cblecker reviewed Jan 31, 2019

View reviewed changes

k8s-ci-robot assigned cblecker Jan 31, 2019

krzysied mentioned this pull request Feb 5, 2019

Adding prettybench and go-junit-report to the vendor #73740

Merged

krzysied force-pushed the test_KUBE_JUNIT_REPORT_DIR_revert branch from feec2c1 to 97654b1 Compare February 6, 2019 10:11

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

fejta reviewed Feb 6, 2019

View reviewed changes

k8s-ci-robot assigned fejta Feb 6, 2019

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 Feb 6, 2019

fejta reviewed Feb 6, 2019

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 Feb 6, 2019

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2019

krzysied force-pushed the test_KUBE_JUNIT_REPORT_DIR_revert branch from 97654b1 to 9256a59 Compare February 7, 2019 10:39

adding go-junit-report install

f461759

krzysied force-pushed the test_KUBE_JUNIT_REPORT_DIR_revert branch from 9256a59 to d138e08 Compare February 7, 2019 10:49

adding FULL_LOG flag

c20262e

krzysied force-pushed the test_KUBE_JUNIT_REPORT_DIR_revert branch from d138e08 to c20262e Compare February 7, 2019 11:05

fejta reviewed Feb 8, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2019

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

k8s-ci-robot merged commit 999e2e0 into kubernetes:master Feb 8, 2019

krzysied deleted the test_KUBE_JUNIT_REPORT_DIR_revert branch February 8, 2019 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KUBE_JUNIT_REPORT_DIR fixes #73581

KUBE_JUNIT_REPORT_DIR fixes #73581

krzysied commented Jan 31, 2019

krzysied commented Jan 31, 2019

krzysied commented Jan 31, 2019

fejta Jan 31, 2019

cblecker Jan 31, 2019

krzysied Feb 1, 2019

fejta Feb 1, 2019

krzysied Feb 5, 2019

fejta Feb 5, 2019

krzysied Feb 6, 2019

cblecker Jan 31, 2019

krzysied Feb 1, 2019

fejta Feb 6, 2019

cblecker Feb 7, 2019 •

edited

krzysied Feb 7, 2019

cblecker Jan 31, 2019

cblecker commented Jan 31, 2019

krzysied commented Feb 5, 2019 •

edited

krzysied commented Feb 6, 2019

fejta left a comment

fejta left a comment

fejta Feb 6, 2019

fejta left a comment

fejta commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

krzysied commented Feb 8, 2019

KUBE_JUNIT_REPORT_DIR fixes #73581

KUBE_JUNIT_REPORT_DIR fixes #73581

Conversation

krzysied commented Jan 31, 2019

krzysied commented Jan 31, 2019

krzysied commented Jan 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker commented Jan 31, 2019

krzysied commented Feb 5, 2019 • edited

krzysied commented Feb 6, 2019

fejta left a comment

Choose a reason for hiding this comment

fejta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta left a comment

Choose a reason for hiding this comment

fejta commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

krzysied commented Feb 8, 2019

cblecker Feb 7, 2019 •

edited

krzysied commented Feb 5, 2019 •

edited