Add initial SemVer upgrade support #444

m1kola · 2023-10-06T16:21:55Z

Description

OLM will now use SemVer to determine next upgrade. In this iteration we only support upgrade within the same major versions: e.g 1.0.1 can be upgraded to 1.0.2 or 1.3.2, but not 2.0.0.

Closes #398

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

internal/resolution/variablesources/installed_package.go

codecov · 2023-10-10T12:58:29Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c9d387e) 83.82% compared to head (9910d29) 83.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #444      +/-   ##
==========================================
- Coverage   83.82%   83.64%   -0.19%     
==========================================
  Files          23       23              
  Lines         847      862      +15     
==========================================
+ Hits          710      721      +11     
- Misses         94       96       +2     
- Partials       43       45       +2

Flag	Coverage Δ
e2e	`65.19% <75.00%> (-0.81%)`	⬇️
unit	`76.55% <75.00%> (-0.07%)`	⬇️

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

Files	Coverage Δ
...al/resolution/variablesources/installed_package.go	`67.44% <75.00%> (+3.15%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

m1kola

I think it is ready for review now

Makefile

m1kola · 2023-10-10T14:25:17Z

internal/resolution/variablesources/installed_package.go

+	}
+
+	// Based on current version create a caret range comparison constraint
+	// to allow only major and patch version as successors and exclude current version.


Re: "exclude current version" see #444 (comment)

internal/resolution/variablesources/installed_package.go

Makefile

m1kola

@joelanford please take another look. I updated Makefile to use tags instead of builds.

Also update the PR to address feedback on testing (see comment below).

Makefile

awgreene

I have a couple of open questions regarding the user experience around 0.minor.patch and 0.0.patch releases, but overall this looks great.

internal/resolution/variablesources/installed_package.go

internal/resolution/variablesources/installed_package_test.go

test/e2e/install_test.go

awgreene · 2023-10-19T03:32:22Z

test/e2e/install_test.go

+					cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorv1alpha1.TypeResolved)
+					g.Expect(cond).ToNot(BeNil())
+					g.Expect(cond.Reason).To(Equal(operatorv1alpha1.ReasonResolutionFailed))
+					g.Expect(cond.Message).To(MatchRegexp(`^constraints not satisfiable:.*; installed package prometheus requires at least one of.*1.2.0[^;]*;.*`))


What does this error message look like in practice?

It is good that you commented on this line! While working on #469 yesterday I wanted to re-use this regex, but realised that it is too permissive and can give false positives. We should probably use "contains" with exact strings (especially since order of dependencies in the message must be deterministic). Should be easier and more reliable.

I was going to update this PR to be similar to #469, but forgot about it. Will do it now.

Also now I see evem more issues with this regex (e.g. . not being escaped).

Answering your question - the message looks like this:

constraints not satisfiable: installed package prometheus is mandatory; installed package prometheus requires at least one of test-catalog-prometheus-prometheus-operator.1.2.0, test-catalog-prometheus-prometheus-operator.1.0.1, test-catalog-prometheus-prometheus-operator.1.0.0; prometheus package uniqueness permits at most 1 of test-catalog-prometheus-prometheus-operator.2.0.0, test-catalog-prometheus-prometheus-operator.1.2.0, test-catalog-prometheus-prometheus-operator.1.0.1, test-catalog-prometheus-prometheus-operator.1.0.0; required package prometheus is mandatory; required package prometheus requires at least one of test-catalog-prometheus-prometheus-operator.2.0.0

Ideally I want the message to contain only things which failed resolution (similar to pip install) but at the moment we kinda dump everything in here. @perdasilva has experimented with differente approaches to printing errors here. I think we will use this as a starter pack.

I have a bunch of notes from a sync with Per, Joe and Daniel which I need to turn into issues in operator-controller, deppy and catalogd. I'm planing to wrap up efforts related to semver RFC and get back to it.

test/e2e/install_test.go

OLM will now use SemVer to determine next upgrade. In this iteration we only support upgrade within the same major versions: e.g 1.0.1 can be upgraded to 1.0.2 or 1.3.2, but not 2.0.0. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

* Add tests for semver upgrades * Label semver and legacy tests so it is possible to filter one or the other out * Update Makefile to no run legacy tests by default to match default deployment of operator-controller Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola

Updated the PR to get rid of MatchRegexp. Answered some questions.

internal/resolution/variablesources/installed_package_test.go

test/e2e/install_test.go

m1kola · 2023-10-19T09:45:57Z

test/e2e/install_test.go

+					cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorv1alpha1.TypeResolved)
+					g.Expect(cond).ToNot(BeNil())
+					g.Expect(cond.Reason).To(Equal(operatorv1alpha1.ReasonResolutionFailed))
+					g.Expect(cond.Message).To(MatchRegexp(`^constraints not satisfiable:.*; installed package prometheus requires at least one of.*1.2.0[^;]*;.*`))


It is good that you commented on this line! While working on #469 yesterday I wanted to re-use this regex, but realised that it is too permissive and can give false positives. We should probably use "contains" with exact strings (especially since order of dependencies in the message must be deterministic). Should be easier and more reliable.

I was going to update this PR to be similar to #469, but forgot about it. Will do it now.

Also now I see evem more issues with this regex (e.g. . not being escaped).

Answering your question - the message looks like this:

constraints not satisfiable: installed package prometheus is mandatory; installed package prometheus requires at least one of test-catalog-prometheus-prometheus-operator.1.2.0, test-catalog-prometheus-prometheus-operator.1.0.1, test-catalog-prometheus-prometheus-operator.1.0.0; prometheus package uniqueness permits at most 1 of test-catalog-prometheus-prometheus-operator.2.0.0, test-catalog-prometheus-prometheus-operator.1.2.0, test-catalog-prometheus-prometheus-operator.1.0.1, test-catalog-prometheus-prometheus-operator.1.0.0; required package prometheus is mandatory; required package prometheus requires at least one of test-catalog-prometheus-prometheus-operator.2.0.0

Ideally I want the message to contain only things which failed resolution (similar to pip install) but at the moment we kinda dump everything in here. @perdasilva has experimented with differente approaches to printing errors here. I think we will use this as a starter pack.

I have a bunch of notes from a sync with Per, Joe and Daniel which I need to turn into issues in operator-controller, deppy and catalogd. I'm planing to wrap up efforts related to semver RFC and get back to it.

internal/resolution/variablesources/installed_package.go

test/e2e/install_test.go

awgreene

/approve

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2023

m1kola force-pushed the semver_upgrades branch from aacbfba to 0b8465b Compare October 9, 2023 10:26

joelanford reviewed Oct 9, 2023

View reviewed changes

internal/resolution/variablesources/installed_package.go Outdated Show resolved Hide resolved

m1kola force-pushed the semver_upgrades branch 3 times, most recently from 15d9b72 to 40a094f Compare October 10, 2023 12:48

m1kola mentioned this pull request Oct 10, 2023

Refactor legacy upgrade constraint semantics #450

Merged

4 tasks

m1kola force-pushed the semver_upgrades branch 2 times, most recently from f4188c8 to 8d7dc4a Compare October 10, 2023 14:20

m1kola marked this pull request as ready for review October 10, 2023 14:33

m1kola requested a review from a team as a code owner October 10, 2023 14:33

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2023

m1kola commented Oct 10, 2023

View reviewed changes

m1kola requested a review from joelanford October 10, 2023 14:53

stevekuznetsov reviewed Oct 10, 2023

View reviewed changes

internal/resolution/variablesources/installed_package.go Show resolved Hide resolved

grokspawn reviewed Oct 11, 2023

View reviewed changes

internal/resolution/variablesources/installed_package.go Outdated Show resolved Hide resolved

grokspawn reviewed Oct 11, 2023

View reviewed changes

internal/resolution/variablesources/installed_package.go Show resolved Hide resolved

m1kola force-pushed the semver_upgrades branch 2 times, most recently from ddc70b2 to b0c17cb Compare October 11, 2023 14:42

tmshort reviewed Oct 11, 2023

View reviewed changes

Makefile Show resolved Hide resolved

m1kola mentioned this pull request Oct 12, 2023

E2E tests for different feature gate states #447

Open

m1kola force-pushed the semver_upgrades branch from b0c17cb to 1e8ed0a Compare October 18, 2023 12:26

m1kola mentioned this pull request Oct 18, 2023

Upgrade tests on controller reconcile level #468

Closed

m1kola commented Oct 18, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

m1kola mentioned this pull request Oct 18, 2023

Operator update tests with semver and legacy upgrade constraints #469

Merged

4 tasks

awgreene reviewed Oct 19, 2023

View reviewed changes

test/e2e/install_test.go Show resolved Hide resolved

m1kola added 2 commits October 19, 2023 11:42

Add initial SemVer upgrade support

73bd44c

OLM will now use SemVer to determine next upgrade. In this iteration we only support upgrade within the same major versions: e.g 1.0.1 can be upgraded to 1.0.2 or 1.3.2, but not 2.0.0. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

Update E2E

9910d29

* Add tests for semver upgrades * Label semver and legacy tests so it is possible to filter one or the other out * Update Makefile to no run legacy tests by default to match default deployment of operator-controller Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

m1kola force-pushed the semver_upgrades branch from 1e8ed0a to 9910d29 Compare October 19, 2023 12:17

m1kola commented Oct 19, 2023

View reviewed changes

awgreene approved these changes Oct 19, 2023

View reviewed changes

m1kola added this pull request to the merge queue Oct 19, 2023

Merged via the queue into operator-framework:main with commit fd02095 Oct 19, 2023
10 of 12 checks passed

m1kola deleted the semver_upgrades branch October 19, 2023 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial SemVer upgrade support #444

Add initial SemVer upgrade support #444

m1kola commented Oct 6, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading

m1kola left a comment

m1kola Oct 10, 2023

m1kola left a comment

awgreene left a comment

awgreene Oct 19, 2023

m1kola Oct 19, 2023

m1kola left a comment

m1kola Oct 19, 2023

awgreene left a comment

Add initial SemVer upgrade support #444

Add initial SemVer upgrade support #444

Conversation

m1kola commented Oct 6, 2023 • edited Loading

Description

Reviewer Checklist

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

m1kola left a comment

Choose a reason for hiding this comment

m1kola Oct 10, 2023

Choose a reason for hiding this comment

m1kola left a comment

Choose a reason for hiding this comment

awgreene left a comment

Choose a reason for hiding this comment

awgreene Oct 19, 2023

Choose a reason for hiding this comment

m1kola Oct 19, 2023

Choose a reason for hiding this comment

m1kola left a comment

Choose a reason for hiding this comment

m1kola Oct 19, 2023

Choose a reason for hiding this comment

awgreene left a comment

Choose a reason for hiding this comment

m1kola commented Oct 6, 2023 •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading