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

Upgrade Downgrade Testing #9300

Merged
merged 21 commits into from
Jan 6, 2022
Merged

Conversation

frouioui
Copy link
Member

@frouioui frouioui commented Nov 30, 2021

Description

This pull request introduces a more comprehensive upgrade/downgrade test strategy in order to comply with VEP-3. The new strategy is thoroughly discussed in #7344 (specifically in this comment: #7344 (comment)).

This pull request focuses on testing the query serving part of Vitess. The backup and cluster management parts will follow the same approach as the one introduced by this pull request.

The methodology used to perform the upgrade downgrade test is the following:

  1. Build Vitess binaries for versions n (the current commit) and n-1 (the latest major release)
  2. Move the binaries to a n_bin and n-1_bin directory, so we can reuse them later
  3. We use a bin directory with all the binaries that our test will use
  4. Keep the binaries from n in the bin directory
  5. Start the test suite once. This will test with both vtgate and vttablet using version n
  6. Swap the binaries. We want to use vtgate version n-1 now. Copy the vtgate binary from the n-1_bin we created earlier into the bin directory
  7. Start the test suite once. This will test with vtgate using version n-1 and vttablet using version n
  8. Swap the binaries one more time. We want to use vtgate version n and vttablet version n-1. Use the binaries stored in the n_bin and n-1_bin directories.
  9. Start the test suite once. This will test with vtgate using version n and vttablet using version n-1

This methodology is similar to the one detailed in @deepthi's comment, but instead of creating a global cluster, we use the already-existing end-to-end tests.

The tests ran in this new workflow are defined using the config.json file. All the tests in this file that are tagged with upgrade_downgrade_query_serving will automatically be tested by the new workflow. Example below:

"vtgate_schematracker_unsharded": {
	"File": "unused.go",
	"Args": ["vitess.io/vitess/go/test/endtoend/vtgate/schematracker/unsharded"],
	"Command": [],
	"Manual": false,
	"Shard": "vtgate_schema_tracker",
	"RetryMax": 1,
	"Tags": ["upgrade_downgrade_query_serving"]
},

Trigger upgrade downgrade testing

The label Upgrade Downgrade enables the run of the workflow. Without the label on your pull request, the workflow will not run. This is done to limit the number of long and not-always-necessary workflows.

cc @deepthi @GuptaManan100 ; What do you think about this label check?

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui marked this pull request as ready for review January 4, 2022 16:00
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui frouioui requested review from deepthi and removed request for rohit-nayak-ps January 4, 2022 16:31
@deepthi
Copy link
Member

deepthi commented Jan 4, 2022

What do you think about this label check?

In general, opt-in CI is not a good idea. PR authors may not realize that they are making a change that can break upgrade/downgrade.

@@ -748,7 +748,7 @@
"Manual": false,
"Shard": "vtgate_schema_tracker",
"RetryMax": 1,
"Tags": []
"Tags": ["upgrade_downgrade_query_serving"]
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected that there will be more tags of this kind?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am expecting to have at least two more, one for backups and one for cluster management

@GuptaManan100
Copy link
Member

I think that the opt-in CI would be useful, in the sense that for smaller PRs or for PRs which do not require running the upgrade downgrade testing, we can avoid doing so. But this would mean that it is the responsibility of the maintainer merging the PR to add the label and run this CI before merging if deemed necessary and not already present

@frouioui
Copy link
Member Author

frouioui commented Jan 5, 2022

@deepthi @GuptaManan100 How about an opt-out CI strategy? With the query serving's upgrade downgrade testing workflow taking approximately an hour, I can see the case where we want to skip the workflow.

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@GuptaManan100
Copy link
Member

I think an opt-out strategy might be okay. For the PRs which we don't want to run them, then we can specifically prevent running them

…able it

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants