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

feat: add upgrade tests to circleCI #9970

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

carolinaecalderon
Copy link
Contributor

@carolinaecalderon carolinaecalderon commented Sep 23, 2024

Ticket

CM-449

Description

Add a new suite of test-e2e-release-upgrade tests that:

  • start a devcluster on a past Determined version (i.e., 0.34.0, 0.35.0, 0.36.0, etc)
  • stop the cluster
  • upgrade the cluster to the commit SHA
  • start the cluster again
  • run a subset of the e2e tests

For this iteration, I chose to omit a subset of the "regular" e2e tests that use SLURM, aws, or gcp -- I can add these back in later, if desired.

Commentary

This ticket is blocked on three fronts:

  1. I don't know how to put this behind a "Needs Approval" check OR only run this when the release and release-ee jobs are run. test-e2e-release-upgrade shouldn't be run for every branch or every commit. @davidfluck-hpe can help me identify where we insert this into the new release party workflow.
  2. In @davidfluck-hpe 's changes https://github.com/determined-ai/determined/pull/10002/files, he removes the explicitly defined pipeline.parameters.det-version, replacing it with a blank string. Since I reference this string in several places, I'm not sure how this would affect the functionality of the tests once his PR is merged.
  3. @mackrorysd has alluded to a document/file that will track the supported Determined versions from which we upgrade from -- I'd like to reference that file instead of explicitly defining supported versions in a yaml anchor.

Test Plan

Observe that CircleCI passes: https://app.circleci.com/pipelines/gh/determined-ai/determined?branch=carolinac%2Fupgrade-tests

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@cla-bot cla-bot bot added the cla-signed label Sep 23, 2024
Copy link

netlify bot commented Sep 23, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit ceedd2f
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66fee771aa29010008c3328d

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.58%. Comparing base (a0cc818) to head (ceedd2f).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9970      +/-   ##
==========================================
- Coverage   54.59%   54.58%   -0.02%     
==========================================
  Files        1259     1259              
  Lines      157245   157245              
  Branches     3620     3619       -1     
==========================================
- Hits        85843    85825      -18     
- Misses      71269    71287      +18     
  Partials      133      133              
Flag Coverage Δ
backend 45.32% <ø> (-0.04%) ⬇️
harness 72.74% <ø> (ø)
web 54.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes

@carolinaecalderon carolinaecalderon changed the title Carolinac/upgrade tests feat: add upgrade tests to circleCI Oct 3, 2024
@@ -64,6 +64,11 @@ parameters:
type: string
default: ""

det-versions: &det-versions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mackrorysd I need to figure out how we're going to sync this up to the list of supported releases. Can we talk about where this is going to live in the repo? Or how this list is bumped between releases?

Copy link
Member

Choose a reason for hiding this comment

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

As I think about this, I think we'll need someone to do SOME manual step periodically no matter what. And that's fine because it's a small step. This person can be me - I have to maintain the release schedule, etc. anyway. If the step fits nicely into the release captain process it could be them. But either way, I think we need to do something manual when we "retire" a minor release line after 9 months or so.

So this list could be what we maintain and update directly, and we update it occasionally. I'm fine to own that. Or we could infer the list from the current list of branches and tags that fit a certain pattern (in which case, after 9 months we need to delete a branch that has hit EOL or rename it to NOT fit the pattern anymore).

What are your thoughts about those options?

Copy link

@davidfluck-hpe davidfluck-hpe Oct 3, 2024

Choose a reason for hiding this comment

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

If we want to do this for a subset of releases, one option would be reading in a separate file that's a newline-delimited list of supported releases. At least then we wouldn't have to commit changes directly to the CircleCI config, and it kind of contains the blast radius a bit.

Another option would be to munge the output of git tag to filter out the three (or however many we want to support) latest minor releases, which shouldn't be too difficult, assuming I understand the problem correctly. (I'd vote for this one, personally, because it's dynamic and means we could fetch supported versions without having to commit to a file each time.)

A neat alternative would be to use commit notes, which can include arbitrary metadata, like Is-Supported: true. This requires annotated tags, though, and I wrote all of the new release stuff to use lightweight tags. I don't think that would be a difficult change, but it would be something for a bit further down the road.

Choose a reason for hiding this comment

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

Regarding the git tag option, I could even update version.sh to provide a flag or two to return an arbitrary number of release versions back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 to David's idea to read a newline-delimited text from a separate file
i don't expect release captains to have thorough knowledge of CircleCI, so it would be preferable to contain the changes OUTSIDE of this file.

regarding the git tag option, I think we'd have to connect @mackrorysd 's supported version logic to that and I don't think it's as simple as the last n-many minor releases. Please correct me if I'm wrong though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the supported versions will also vary between branches. Main wants to make sure we can upgrade from the latest commit on release branches, and patch releases want to make sure you can upgrade from their predecessors too.

I love the idea of a newline-delimited text file. Let's go with that!

@carolinaecalderon carolinaecalderon marked this pull request as ready for review October 3, 2024 18:51
@carolinaecalderon carolinaecalderon requested a review from a team as a code owner October 3, 2024 18:51
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