Skip to content

Conversation

@dgoodwin
Copy link
Contributor

@dgoodwin dgoodwin commented Oct 21, 2022

This change begins writing out a brief test failure summary artifact in a format that is logical and also deserializable by sippy.

It also adds a new sub-command that can be used to merge several of these files, submit them to sippy, and write the resulting risk analysis to the artifacts directory.

Requires openshift/sippy#657

TRT-602

@dgoodwin dgoodwin changed the title write test failures WIP: write test failures Oct 21, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2022
@openshift-ci openshift-ci bot requested review from csrwng and jwforres October 21, 2022 14:48
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2022
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2022
@dgoodwin
Copy link
Contributor Author

@dgoodwin dgoodwin force-pushed the write-test-failures branch from 8843bf4 to cb8d796 Compare October 26, 2022 17:43
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022
@dgoodwin dgoodwin changed the title WIP: write test failures Write test failure summary, and add new command to request failure risk analysis from sippy. Oct 26, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022
@dgoodwin
Copy link
Contributor Author

Depends on openshift/sippy#657

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2022
@dgoodwin dgoodwin force-pushed the write-test-failures branch from cb8d796 to 296e32d Compare October 26, 2022 17:49
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 26, 2022
This will be used to submit to a new sippy risk analysis API to
determine how risky the test failures in this job run are. As with junit
xmls, there can be multiple as we often invoke openshift-tests multiple
times in one job run.

Includes a change to make timestamp suffixes on junit artifacts more
consistent.
… and upload.

This merges test-flake files, submits them to sippy, and writes the
returned risk analysis as an additional artifact. It is intended to be
called from the workflow steps that run openshift-tests in the release
repo.
@dgoodwin dgoodwin force-pushed the write-test-failures branch from 296e32d to 19740f0 Compare October 31, 2022 10:57
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2022
}

// TODO: query sippy
url := "http://localhost:8080/api/jobs/runs/risk_analysis"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want this to be a config option so we could change down the road without updating the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that should be the real URL.

If sippy moves we'll have to make a commit somewhere, would that be better in release repo to update config, or in origin? Release repo might be a little better as it's probably exempt from code freeze. I could do a CLI flag for this with a default, so it's there if we ever need it.

Copy link
Member

Choose a reason for hiding this comment

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

Having it in the test step (maybe with a default here) makes sense. If it ever changes we'll avoid having to backport the changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of cli flag that can be defaulted and then you can change it in release repo when you call this as a step. Also makes it nice so you can call it locally for testing.


jr := ProwJobRun{
ProwJob: ProwJob{Name: os.Getenv("JOB_NAME")},
URL: os.Getenv("JOB_URL"), // just a guess, may not exist
Copy link
Contributor

Choose a reason for hiding this comment

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

In this sample podinfo.json from this prow job, there is no JOB_URL. It does have BUILD_ID if you wanted to construct an URL later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will get this updated in my followup.

@xueqzhan
Copy link
Contributor

Looking good to me except the Sippy URL string.

@DennisPeriquet
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@stbenjam
Copy link
Member

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2022
@stbenjam
Copy link
Member

/hold

Sippy URL needs fixing

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@dgoodwin
Copy link
Contributor Author

URL fixed, I will follow up with the CLI arg.

@stbenjam
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@dgoodwin
Copy link
Contributor Author

Apologies, I'm going to have to bother folks for another lgtm anyhow, I made the requested modifications in this PR.

@stbenjam
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DennisPeriquet, dgoodwin, stbenjam

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@dgoodwin
Copy link
Contributor Author

dgoodwin commented Nov 1, 2022

/hold cancel
/retest

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2022
@dgoodwin
Copy link
Contributor Author

dgoodwin commented Nov 1, 2022

/override ci/prow/e2e-gcp-ovn-upgrade

Failing on disruption.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 1, 2022

@dgoodwin: Overrode contexts on behalf of dgoodwin: ci/prow/e2e-gcp-ovn-upgrade

Details

In response to this:

/override ci/prow/e2e-gcp-ovn-upgrade

Failing on disruption.

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.

@openshift-merge-robot openshift-merge-robot merged commit 8451d66 into openshift:master Nov 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 1, 2022

@dgoodwin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-csi b3a776a link false /test e2e-aws-csi

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@dgoodwin
Copy link
Contributor Author

dgoodwin commented Nov 3, 2022

/cherry-pick release-4.12

@openshift-cherrypick-robot

@dgoodwin: new pull request created: #27520

Details

In response to this:

/cherry-pick release-4.12

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.

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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants