search

operator-framework / operator-controller

Apache License 2.0
29 stars 47 forks source link

✨ Add upgrade E2E #1003

Open m1kola opened 6 days ago

m1kola commented 6 days ago

Description

Testing upgrade from the latest release to the current commit.

Fixes #856

Reviewer Checklist

netlify[bot] commented 6 days ago

Deploy Preview for olmv1 ready!

Name Link
Latest commit 51e0c3ac5c458f0e033f3aba1a29668ee1172bee
Latest deploy log https://app.netlify.com/sites/olmv1/deploys/6687ea69fb1a3400085eafa1
Deploy Preview https://deploy-preview-1003--olmv1.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov[bot] commented 6 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.19%. Comparing base (8bf5cf4) to head (51e0c3a).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1003 +/- ## ======================================= Coverage 77.19% 77.19% ======================================= Files 17 17 Lines 1206 1206 ======================================= Hits 931 931 Misses 193 193 Partials 82 82 ``` | [Flag](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1003/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | Coverage Δ | | |---|---|---| | [e2e](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `56.54% <ø> (ø)` | | | [unit](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1003/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `51.90% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tmshort commented 5 days ago

My general question is... do we want to use bash scripts for this, or do we want to use golang?

joelanford commented 4 days ago

Do we want to use bash scripts for this, or do we want to use golang?

This is a good question. I don't necessarily think its an either-or thing. I could see something like:

sh <installPreviousRelease>
sh <setupPreUpgradeState>
sh <upgradeIt>
go test ./test/upgrade-e2e/...
joelanford commented 4 days ago

Another interesting upgrade question: At what point do we need to actually define an upgrade process? An upgrade is generally:

  1. Create objects that are net new in the new release
  2. Update objects that are changed from the old release to the new release
  3. Delete objects that were present in the old release, but not the new release

We don't do step 3 right now, right? Is doing step 3 a prereq for valid upgrade testing?

m1kola commented 4 days ago

Another interesting upgrade question: At what point do we need to actually define an upgrade process? An upgrade is generally:

  1. Create objects that are net new in the new release
  2. Update objects that are changed from the old release to the new release
  3. Delete objects that were present in the old release, but not the new release

We don't do step 3 right now, right? Is doing step 3 a prereq for valid upgrade testing?

We were talking about this with @ankitathomas. It sounds a bit like we are testing something that doesn't exist at this moment (the upgrade process).

We decided not to open a can of worms for now and assume that the upgrade process is just:

This should be enough short-term: it should signal when someone adds something breaking to manifests/install script.

But long term we probably want to define upgrade process and probably create tool for that. E.g. we will likely need to some migration tool where it is possible to clean up old in-cluster objects from previous releases. We can use OpenShift's Cluster Version Operator and how it handles deletions.

But IMO - this deserves its own epic. What do you think? Should we put this on hold and define upgrade process & create necessary tools first? Or should we proceed with this naive approach of just applying things on top?

Do we want to use bash scripts for this, or do we want to use golang?

This is a good question. I don't necessarily think its an either-or thing. I could see something like:

sh <installPreviousRelease>
sh <setupPreUpgradeState>
sh <upgradeIt>
go test ./test/upgrade-e2e/...

I decided to keep bash in the draft for now. We can switch to Go when/if we decide to do something more sophisticated here.

joelanford commented 3 days ago

Already have an epic for proper upgrade support. https://github.com/operator-framework/operator-controller/issues/982

I agree that this epic should not be blocked on that one.

I think naive approach is good enough for now, but I suspect there could be some cases where leaving around old manifests causes the upgrade tests to pass when cleaning them up would have made them fail. As an example: a PR that deletes our issuer Certificate from our manifest with no other changes.

m1kola commented 11 hours ago

I suspect there could be some cases where leaving around old manifests causes the upgrade tests to pass when cleaning them up would have made them fail. As an example: a PR that deletes our issuer Certificate from our manifest with no other changes.

Yes, there will be cases like that, but in this specific example, I think, we will catch the issue in the E2E job because a clean setup will fail.