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

Finish saving test results on failure #76039

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

johnSchnake
Copy link
Contributor

@johnSchnake johnSchnake commented Apr 2, 2019

The conformance image should be saving its results
regardless of the results of the tests. However,
with errexit set, when ginkgo gets test failures
it exits 1 which prevents saving the results
for Sonobuoy to pick up.

Fixes: #76036

/kind bug

Special notes for your reviewer:
Currently working on building this image and testing it works as expected.

Does this PR introduce a user-facing change?:

Ensures the conformance test image saves results before exiting when ginkgo returns non-zero value.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 2, 2019
@johnSchnake
Copy link
Contributor Author

/sig testing
/area conformance

@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. area/conformance Issues or PRs related to kubernetes conformance tests sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/release Categorizes an issue or PR as relevant to SIG Release. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 2, 2019
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

Please follow up with cherry-pick PR for 1.14 branch

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnSchnake, timothysc

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2019
@johnSchnake
Copy link
Contributor Author

/test pull-kubernetes-integration

@johnSchnake
Copy link
Contributor Author

Manually confirmed that the patch does fix the error case mentioned in the ticket; a failing ginkgo run still ends up saving the results and getting marked as completed via Sonobuoy.

@johnSchnake
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

Copy link
Member

@ixdy ixdy left a comment

Choose a reason for hiding this comment

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

/hold

@@ -13,7 +13,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -o errexit
Copy link
Member

Choose a reason for hiding this comment

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

it'd be better to change the wait line to be something like

wait "$(pgrep ginkgo)" && ret=0 || ret=$?

and then the last line of this script would be

exit ${ret}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify to ensure I understand correctly,

is the reason you want to do that so that it isn't just customized for Sonobuoy (waiting for the done file) but also a consumer could wait for the non-zero exit code for indication of pass/fail?

I guess those two things are at odds:

  • if we set errexit and the script fails anywhere results wont be "published" and Sonobuoy would hang
  • if we dont set errexit and the script fails anywhere the exit code wont be properly reported and it might report success

If my understanding above is accurate, I agree that the lesser evil is to do as you suggest. A hang would at least be indicative of a problem whereas a false-positive is a larger issue that could go undiscovered.

Also +1/TIL to that particular bash logic; I dont think I'd seen that exact trick used before but it seems pretty nifty.

Copy link
Member

Choose a reason for hiding this comment

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

we generally like keeping errexit on scripts so that we fail fast on syntax errors or other issues - you're sorta required to handle all error cases, or everything will just fail.

another alternative if we want to make sure logs are always uploaded is to add something like

trap saveResults EXIT

and then you wouldn't even need to explicitly call saveResults - it'd always run on script exit.

@k8s-ci-robot 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 2, 2019
The conformance image should be saving its results
regardless of the results of the tests. However,
with errexit set, when ginkgo gets test failures
it exits 1 which prevents saving the results
for Sonobuoy to pick up.

Fixes: kubernetes#76036
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2019
@dims
Copy link
Member

dims commented Apr 3, 2019

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 3, 2019
@BenTheElder
Copy link
Member

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 3, 2019
@k8s-ci-robot k8s-ci-robot merged commit 41691a0 into kubernetes:master Apr 3, 2019
@johnSchnake johnSchnake deleted the conformanceErrExit branch April 3, 2019 18:35
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 3, 2019
k8s-ci-robot added a commit that referenced this pull request Apr 4, 2019
…76039-upstream-release-1.14

Automated cherry pick of #76039: Finish saving test results on failure
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. area/conformance Issues or PRs related to kubernetes conformance tests cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants