Add upgrade tests to test main against previous release #6690

JeromeJu · 2023-05-19T14:51:29Z

Changes

This commit adds the upgrade tests for testing main against the e2e tests from the previous release to prevent regression. Incompatible changes made in the current release cycle are expected to be catched.

This should also catch the case where existing resources become invalid with tests from previous releases.

/kind misc
fixes #6728

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
[n/a] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
[n/a] Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-05-19T14:51:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign imjasonh after the PR has been reviewed.
You can assign the PR to them by writing /assign @imjasonh in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

dibyom · 2023-05-22T18:11:50Z

test/e2e-tests-upgrade.sh

@@ -24,6 +24,10 @@
 # Scenario 2: install the previous release, create the pipelines and tasks, upgrade
 # to the current release, and validate whether the Tekton pipeline works.

+# Scenario 3: install the current pipelines, use the integration tests from the previous release(s)
+# to prevent regression. Incompatible changes made in the current release cycle are expected to fail
+# and will be skipped after failing once.


How does the skipping work? Is there a way to mark as test as skipped for the upgrade test suite?

We could introduce a new build tag for upgrade tests, but this would require updates to the e2e test tag when a feature is introduced or there are any breaking changes made to it and spotted by the upgrade test.

We could have a 'skip list'(probably in the e2e-test-upgrade.sh) and use regex and mv command to move around skipped tests in the ./test folder.

I thought this could be in a followup PR? if not so happy to add in this PR if any of the above sounds ok to add

I think we should do it in this PR, so we have a way to make backwards incompatible changes if needed, or at least some plan for how we would do so without skipping upgrade tests entirely. I think it's confusing for this comment to describe this functionality if it's not part of this PR.

I'm not sure what it means exactly to skip upgrade tests after they fail once? Does this mean the first time CI is run for one PR, within a CI run after failing once, or between all CI runs for all PRs? I'm not sure if we'd want to just skip failing tests without some user intervention to signal that an incompatible change is intentional.

I'm also not sure what the best way of doing this is. Can you elaborate a bit more on how your ideas would work? Is the intention that you'd have to modify the skip list when making an incompatible change?

I think either tekton chains or chainguard (not sure which) has tests like this already so it might be worth talking to @wlynch about how they work.

I'm not sure what it means exactly to skip upgrade tests after they fail once? Does this mean the first time CI is run for one PR, within a CI run after failing once, or between all CI runs for all PRs?

The intentions for skipping here is to prevent the upgrade test from failing consistently when there are known incompatible changes made on devel (of which there should be only few cases that we run into ideally).

I'm not sure what it means exactly to skip upgrade tests after they fail once? Does this mean the first time CI is run for one PR, within a CI run after failing once, or between all CI runs for all PRs?

The upgrade tests now we have is a CRON job. The case I am picturing that if it is observed to fail for the first time during this dev cycle, then we would want to investigate if the failed test is a regression and then decide to either escalate or to skip it(if it is an intentional breaking change).

I was suggesting the 2 alternatives based on the above assumptions if the case is reasonable 🤔 .

I had the chance to learn from @wlynch a bit ago when working on #5784, IIRC some mapping of certain versions and enabled features has been one of the suggestions to take. I am not sure if a list of e2e tests with 'breaking changes' would be a good idea at this stage for our features since there shouldn't be many?

Oh, I forgot upgrade tests are a cron job. I wonder if it would make sense to run them as part of CI instead? We might be constrained by our test cluster bandwidth though just based on our experience with integration tests.

Of the two approaches you mentioned, I think the "skip list" is better because it's opt-out rather than opt-in.

Thanks @lbernick , I do agree that it would be nice and responsive to detect breaking changes in CI. Added #6709 as a followup to track moving upgrade tests to CI.

I have also tried out the "skip list" approach in the current commit. Moving forward when we are running against multiple previous releases, we could change the 'skip list' to a mapping of specific releases and incompatible features/tests.

Oh, I forgot upgrade tests are a cron job. I wonder if it would make sense to run them as part of CI instead? We might be constrained by our test cluster bandwidth though just based on our experience with integration tests.

One idea would be to run these tests as part of the release pipeline

Thanks Dibyo, tracked the upcoming goals at #6709

Thinking about this more-- it would be nice to at least run upgrade tests as part of CI if this file is changed-- otherwise it's hard to say with confidence that this PR works as intended

test/e2e-tests-upgrade.sh

lbernick · 2023-05-25T14:52:41Z

test/e2e-tests-upgrade.sh

+header "Testing main against the latest release to detect regression"
+get_tests_from_release $PREVIOUS_PIPELINE_VERSION
+
+skip_breaking_tests


@JeromeJu curious if you've been able to test this locally, especially the test skipping part. Do you have an example output?

I have tried it locally with 1-2 tests name in the list, it just removed those tests from our tests suites(skipping seems to work fine).
May I ask what would an ideal example output to be, it feels that has to be a long output as the logs we have in prow?

maybe just comment in this thread with a link to a github gist?

failed test with TestExamples/v1/pipelineruns/pipelinerun, TestWaitCustomTask_V1Beta1_PipelineRun, which are known due to config not at local

skip tests failed - after

test/e2e-tests-upgrade.sh

lbernick · 2023-05-25T14:55:31Z

test/e2e-tests-upgrade.sh

@@ -24,6 +24,10 @@
 # Scenario 2: install the previous release, create the pipelines and tasks, upgrade
 # to the current release, and validate whether the Tekton pipeline works.

+# Scenario 3: install the current pipelines, use the integration tests from the previous release(s)
+# to prevent regression. Incompatible changes made in the current release cycle are expected to fail
+# and will be skipped after failing once.


Thinking about this more-- it would be nice to at least run upgrade tests as part of CI if this file is changed-- otherwise it's hard to say with confidence that this PR works as intended

test/e2e-tests-upgrade.sh

lbernick · 2023-06-01T17:23:12Z

test/e2e-tests-upgrade.sh

-# Upgrade to the current release.
-header "Upgrade to the current release of Tekton pipeline"
-install_pipeline_crd
+# Run the integration tests.c


Suggested change

# Run the integration tests.c

# Run the integration tests.

lbernick · 2023-06-01T17:25:14Z

test/e2e-tests-upgrade.sh

+function skip_breaking_examples {
+  for TEST in "${BACKWARD_INCOMPATIBLE_EXAMPLES[@]}"; do
+    echo "Remove $TEST since it is a breaking change at devel"
+    rm ./examples/v1/"$TEST".yaml


I wonder if instead of removing the examples tests from the local copy of the repo, would it instead make sense to update the examples tests to accept a flag for which tests should be skipped?

test/e2e-tests-upgrade.sh

This commit adds the upgrade tests for testing main against the e2e tests from the previous release to prevent regression. Incompatible changes made in the current release cycle are expected to be catched.

JeromeJu · 2023-06-12T21:42:34Z

Has been prioritizing on other issues and just got back onto this. So far I haven't figured out a way to only trigger such upgrade test when an actual regression happen? Seems the only way rn is to wait for the nightly CRON job.

lbernick · 2023-06-14T15:46:13Z

Has been prioritizing on other issues and just got back onto this. So far I haven't figured out a way to only trigger such upgrade test when an actual regression happen? Seems the only way rn is to wait for the nightly CRON job.

Can you clarify what you're trying to do? I think running these checks in CI is tracked in #6709, or you can run it locally to test. Arguably, we should be running these tests as part of CI before making changes to them, otherwise it's hard to know if the changes work as intended, but we could justify saving this for future work.

JeromeJu · 2023-06-15T18:26:24Z

Can you clarify what you're trying to do? I think running these checks in CI is tracked in #6709, or you can run it locally to test.

I was in the middle of trying to open a new PR and left the current branch as an example for upgrade test changes only, but now let's just keep it in this one.

Arguably, we should be running these tests as part of CI before making changes to them, otherwise it's hard to know if the changes work as intended, but we could justify saving this for future work.

I am trying to seek for a way to figure a way out to put this as part of the CI and circumvent the flakes which is apparent in our CI. Is it suggesting that making the upgrade test part of the CI ideally prior to the current PR?

tekton-robot · 2023-07-10T23:05:04Z

@JeromeJu: PR needs rebase.

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.

tekton-robot · 2023-10-08T23:18:40Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. release-note-none Denotes a PR that doesnt merit a release note. labels May 19, 2023

tekton-robot requested review from afrittoli and lbernick May 19, 2023 14:51

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 19, 2023

JeromeJu mentioned this pull request May 19, 2023

Add client/server compatibility tests #5782

Closed

1 task

dibyom reviewed May 22, 2023

View reviewed changes

JeromeJu force-pushed the upgrade-test branch from c8cbdfe to 2f89ea2 Compare May 23, 2023 19:37

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 23, 2023

JeromeJu mentioned this pull request May 23, 2023

Improve Upgrade tests to be part of CI and maintain in community #6709

Closed

JeromeJu force-pushed the upgrade-test branch from 2f89ea2 to e087cca Compare May 23, 2023 20:47

JeromeJu requested a review from dibyom May 23, 2023 21:08

JeromeJu force-pushed the upgrade-test branch from e087cca to 19526e0 Compare May 24, 2023 19:12

Yongxuanzhang reviewed May 24, 2023

View reviewed changes

test/e2e-tests-upgrade.sh Outdated Show resolved Hide resolved

test/e2e-tests-upgrade.sh Outdated Show resolved Hide resolved

JeromeJu force-pushed the upgrade-test branch from 19526e0 to 6c0fb51 Compare May 24, 2023 20:21

JeromeJu requested a review from Yongxuanzhang May 25, 2023 14:28

lbernick reviewed May 25, 2023

View reviewed changes

JeromeJu force-pushed the upgrade-test branch from 6c0fb51 to 1848248 Compare May 31, 2023 14:58

lbernick reviewed Jun 1, 2023

View reviewed changes

JeromeJu force-pushed the upgrade-test branch from 1848248 to 02aa689 Compare June 9, 2023 21:42

Add upgrade tests to test main against previous release

71c446d

This commit adds the upgrade tests for testing main against the e2e tests from the previous release to prevent regression. Incompatible changes made in the current release cycle are expected to be catched.

JeromeJu force-pushed the upgrade-test branch from 02aa689 to 71c446d Compare June 12, 2023 21:39

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 8, 2023

JeromeJu closed this Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add upgrade tests to test main against previous release #6690

Add upgrade tests to test main against previous release #6690

JeromeJu commented May 19, 2023 •

edited

Loading

tekton-robot commented May 19, 2023

dibyom May 22, 2023

JeromeJu May 23, 2023

lbernick May 23, 2023

JeromeJu May 23, 2023 •

edited

Loading

lbernick May 23, 2023

JeromeJu May 23, 2023 •

edited

Loading

dibyom May 24, 2023

JeromeJu May 24, 2023

lbernick May 25, 2023

lbernick May 25, 2023

JeromeJu May 25, 2023 •

edited

Loading

lbernick May 25, 2023

JeromeJu May 31, 2023

lbernick May 25, 2023

lbernick Jun 1, 2023

lbernick Jun 1, 2023

JeromeJu commented Jun 12, 2023

lbernick commented Jun 14, 2023

JeromeJu commented Jun 15, 2023

tekton-robot commented Jul 10, 2023

tekton-robot commented Oct 8, 2023

Add upgrade tests to test main against previous release #6690

Add upgrade tests to test main against previous release #6690

Conversation

JeromeJu commented May 19, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeromeJu May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeromeJu May 23, 2023 • edited Loading

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

JeromeJu May 25, 2023 • edited Loading

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

JeromeJu commented Jun 12, 2023

lbernick commented Jun 14, 2023

JeromeJu commented Jun 15, 2023

tekton-robot commented Jul 10, 2023

tekton-robot commented Oct 8, 2023

JeromeJu commented May 19, 2023 •

edited

Loading

JeromeJu May 23, 2023 •

edited

Loading

JeromeJu May 23, 2023 •

edited

Loading

JeromeJu May 25, 2023 •

edited

Loading