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

[rebase 1.24] disable tests with time limit to facilitate rebase #27086

Merged
merged 2 commits into from May 7, 2022

Conversation

tkashem
Copy link
Contributor

@tkashem tkashem commented May 2, 2022

No description provided.

@tkashem
Copy link
Contributor Author

tkashem commented May 2, 2022

/assign @deads2k @p0lyn0mial

@p0lyn0mial
Copy link
Contributor

/lgtm

this is what we did in the past to facilitate rebases

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

deads2k commented May 2, 2022

/hold

let's get a "why" for each one please.

@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 May 2, 2022
@deads2k
Copy link
Contributor

deads2k commented May 2, 2022

this is what we did in the past to facilitate rebases

We're working on getting better over time.


// TODO: to facilitate v.14 rebase, must be removed after the rebase PR lands
`\[sig-auth\] ServiceAccounts should allow opting out of API token automount`,
`\[sig-instrumentation\] Events API should ensure that an event can be fetched, patched, deleted, and listed`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking an API contract like this appears suspicious, why exclude it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See bug https://bugzilla.redhat.com/show_bug.cgi?id=2081084, to make sure we don't ship without fixing this.
I think we see this test failing because origin/test (from previous version v1.23) is running against an apiserver (v1.24 with rebase PR)
So hopefully, when re re-vendor v1.24 into openshift/origin it should resolve, otherwise we will need to fix it before we ship 4.11.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Please locate the fix and ideally fix it here. We have previously created blocker+ bugs, which were later deferred. Let's do better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this relates to kubernetes/kubernetes#108047. so the old test in openshift/origin is running against a new apiserver.

@@ -66,6 +66,11 @@ var (

// https://bugzilla.redhat.com/show_bug.cgi?id=2004074
`\[sig-network-edge\]\[Feature:Idling\] Unidling should work with TCP \(while idling\)`,

// TODO: to facilitate v.14 rebase, must be removed after the rebase PR lands
`\[sig-auth\] ServiceAccounts should allow opting out of API token automount`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look proper either. do we need an early pick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See bug https://bugzilla.redhat.com/show_bug.cgi?id=2081087, to make sure we don't ship without fixing this.
I think we see this test failing because origin/test (from previous version v1.23) is running against an apiserver (v1.24 with rebase PR)
So hopefully, when re re-vendor v1.24 into openshift/origin it should resolve, otherwise we will need to fix it before we ship 4.11.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Please locate the fix and ideally fix it here. We have previously created blocker+ bugs, which were later deferred. Let's do better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relates to upstream pr: kubernetes/kubernetes#108309

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 2, 2022
// - [sig-auth\] ServiceAccounts : https//bugzilla.redhat.com/show_bug.cgi?id=2081087
`\[sig-auth\] ServiceAccounts should allow opting out of API token automount`,
`\[sig-instrumentation\] Events API should ensure that an event can be fetched, patched, deleted, and listed`,
`\[sig-api-machinery\] API data in etcd should be stored at the correct location and version for all resources`,
Copy link
Contributor

Choose a reason for hiding this comment

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

find a way to skip this for 3 three weeks and have it auto-come back. This being a variable, seems like a clever init block could do it. This one I can accept will fail until the new level is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the etcd data location relates to kubernetes/kubernetes#108445. v1 of csistoragecapacities has been added to 1.24.

@tkashem
Copy link
Contributor Author

tkashem commented May 2, 2022

I think all three test failures will resolve once we vendor o/k into origin after the rebase PR lands, see the related upstream PRs:

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2022
@tkashem
Copy link
Contributor Author

tkashem commented May 5, 2022

/assign @soltysh
@deads2k @p0lyn0mial added skip until support

@tkashem tkashem changed the title [rebase 1.24] disable tests to facilitate rebase [rebase 1.24] disable tests with time limit to facilitate rebase May 5, 2022
@tkashem
Copy link
Contributor Author

tkashem commented May 5, 2022

/retest

// if the specified date in the tag has not passed yet, the test
// will be skipped by the runner.
func shouldSkipUntil(name string) bool {
_, after, found := cut(name, "[SkippedUntil:")
Copy link
Member

Choose a reason for hiding this comment

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

How about using something like:

re, err := regexp.Compile(`\[SkippedUntil:(\d{6})\]`)
s := re.FindStringSubmatch(name)

this will return you 2-element array, where the first is the entire [SkippedUntil:123456] and the 2nd will be just 123456.

Copy link
Contributor

Choose a reason for hiding this comment

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

SkippedUntil:<Date>,blocker-bz/<number> to make the requirement clear.

// - [sig-api-machinery] API data in etcd should be: https://bugzilla.redhat.com/show_bug.cgi?id=2081021
// - [sig-instrumentation] Events API should ensure that: https://bugzilla.redhat.com/show_bug.cgi?id=2081084
// - [sig-auth] ServiceAccounts : https//bugzilla.redhat.com/show_bug.cgi?id=2081087
"[SkippedUntil:05272022]": {
Copy link
Contributor

Choose a reason for hiding this comment

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

since we check it in isDisabled we could rename to [DisabledUntil...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so we use both Disabled and Skipped - i don't know what the semantic differences are between these two. Also i want to avoid a DisabledUntil: tag get misinterpreted as plain Disabled

func isDisabled(name string) bool {
	return strings.Contains(name, "[Disabled") 
}

@tkashem
Copy link
Contributor Author

tkashem commented May 5, 2022

/retest-required

5 similar comments
@tkashem
Copy link
Contributor Author

tkashem commented May 5, 2022

/retest-required

@tkashem
Copy link
Contributor Author

tkashem commented May 5, 2022

/retest-required

@tkashem
Copy link
Contributor Author

tkashem commented May 5, 2022

/retest-required

@tkashem
Copy link
Contributor Author

tkashem commented May 6, 2022

/retest-required

@tkashem
Copy link
Contributor Author

tkashem commented May 6, 2022

/retest-required

@tkashem
Copy link
Contributor Author

tkashem commented May 6, 2022

/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 May 6, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 6, 2022

@tkashem: The following tests 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-single-node-upgrade f337547 link false /test e2e-aws-single-node-upgrade
ci/prow/e2e-gcp-ovn-rt-upgrade f337547 link false /test e2e-gcp-ovn-rt-upgrade
ci/prow/e2e-aws-csi f337547 link false /test e2e-aws-csi

Full PR test history. Your PR dashboard.

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.

@tkashem
Copy link
Contributor Author

tkashem commented May 6, 2022

/test e2e-metal-ipi-ovn-ipv6

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

10 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit becd645 into openshift:master May 7, 2022
@tkashem tkashem deleted the sa-test-fix branch May 9, 2022 12:37
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.

None yet

6 participants