Skip to content

Commit 44257e1

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1061 from openshift/revert-996-allow-rollbacks-to-most-recently-previous-version
OCPBUGS-35994: Revert "OCPBUGS-24535: pkg/payload/precondition/clusterversion/rollback: Allow previous version within z-stream"
2 parents db77771 + cd1db93 commit 44257e1

File tree

3 files changed

+33
-227
lines changed

3 files changed

+33
-227
lines changed

‎pkg/cvo/cvo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -972,7 +972,7 @@ func hasReachedLevel(cv *configv1.ClusterVersion, desired configv1.Update) bool
972972

973973
func (optr *Operator) defaultPreconditionChecks() precondition.List {
974974
return []precondition.Precondition{
975-
preconditioncv.NewRollback(optr.cvLister),
975+
preconditioncv.NewRollback(optr.currentVersion),
976976
preconditioncv.NewUpgradeable(optr.cvLister),
977977
preconditioncv.NewRecommendedUpdate(optr.cvLister),
978978
}

‎pkg/payload/precondition/clusterversion/rollback.go

Lines changed: 12 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -5,59 +5,44 @@ import (
55
"fmt"
66

77
"github.com/blang/semver/v4"
8-
configv1listers "github.com/openshift/client-go/config/listers/config/v1"
9-
apierrors "k8s.io/apimachinery/pkg/api/errors"
10-
"k8s.io/apimachinery/pkg/api/meta"
11-
"k8s.io/klog/v2"
8+
configv1 "github.com/openshift/api/config/v1"
129

1310
precondition "github.com/openshift/cluster-version-operator/pkg/payload/precondition"
1411
)
1512

13+
// currentRelease is a callback for returning the version that is currently being reconciled.
14+
type currentRelease func() configv1.Release
15+
1616
// Rollback blocks rollbacks from the version that is currently being reconciled.
1717
type Rollback struct {
18-
key string
19-
lister configv1listers.ClusterVersionLister
18+
currentRelease
2019
}
2120

2221
// NewRollback returns a new Rollback precondition check.
23-
func NewRollback(lister configv1listers.ClusterVersionLister) *Rollback {
22+
func NewRollback(fn currentRelease) *Rollback {
2423
return &Rollback{
25-
key: "version",
26-
lister: lister,
24+
currentRelease: fn,
2725
}
2826
}
2927

3028
// Name returns Name for the precondition.
31-
func (p *Rollback) Name() string { return "ClusterVersionRollback" }
29+
func (pf *Rollback) Name() string { return "ClusterVersionRollback" }
3230

3331
// Run runs the Rollback precondition, blocking rollbacks from the
3432
// version that is currently being reconciled. It returns a
3533
// PreconditionError when possible.
3634
func (p *Rollback) Run(ctx context.Context, releaseContext precondition.ReleaseContext) error {
37-
cv, err := p.lister.Get(p.key)
38-
if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) {
39-
return nil
40-
}
35+
currentRelease := p.currentRelease()
36+
currentVersion, err := semver.Parse(currentRelease.Version)
4137
if err != nil {
4238
return &precondition.Error{
4339
Nested: err,
44-
Reason: "UnknownError",
40+
Reason: "InvalidCurrentVersion",
4541
Message: err.Error(),
4642
Name: p.Name(),
4743
}
4844
}
4945

50-
currentVersion, err := semver.Parse(cv.Status.Desired.Version)
51-
if err != nil {
52-
return &precondition.Error{
53-
Nested: err,
54-
Reason: "InvalidCurrentVersion",
55-
Message: err.Error(),
56-
Name: p.Name(),
57-
NonBlockingWarning: true, // do not block on issues that require an update to fix
58-
}
59-
}
60-
6146
targetVersion, err := semver.Parse(releaseContext.DesiredVersion)
6247
if err != nil {
6348
return &precondition.Error{
@@ -69,42 +54,9 @@ func (p *Rollback) Run(ctx context.Context, releaseContext precondition.ReleaseC
6954
}
7055

7156
if targetVersion.LT(currentVersion) {
72-
var previousVersion *semver.Version
73-
var previousImage string
74-
for _, entry := range cv.Status.History {
75-
if entry.Version != currentVersion.String() || entry.Image != cv.Status.Desired.Image {
76-
version, err := semver.Parse(entry.Version)
77-
if err != nil {
78-
klog.Errorf("Precondition %q previous version %q invalid SemVer: %v", p.Name(), entry.Version, err)
79-
} else {
80-
previousVersion = &version
81-
previousImage = entry.Image
82-
}
83-
break
84-
}
85-
}
86-
87-
if previousVersion != nil {
88-
if !targetVersion.EQ(*previousVersion) {
89-
return &precondition.Error{
90-
Reason: "LowDesiredVersion",
91-
Message: fmt.Sprintf("%s is less than the current target %s, and the only supported rollback is to the cluster's previous version %s (%s)", targetVersion, currentVersion, *previousVersion, previousImage),
92-
Name: p.Name(),
93-
}
94-
}
95-
if previousVersion.Major == currentVersion.Major && previousVersion.Minor == currentVersion.Minor {
96-
klog.V(2).Infof("Precondition %q allows rollbacks from %s to the previous version %s within a z-stream", p.Name(), currentVersion, targetVersion)
97-
return nil
98-
}
99-
return &precondition.Error{
100-
Reason: "LowDesiredVersion",
101-
Message: fmt.Sprintf("%s is less than the current target %s and matches the cluster's previous version, but rollbacks that change major or minor versions are not recommended", targetVersion, currentVersion),
102-
Name: p.Name(),
103-
}
104-
}
10557
return &precondition.Error{
10658
Reason: "LowDesiredVersion",
107-
Message: fmt.Sprintf("%s is less than the current target %s, and the cluster has no previous Semantic Version", targetVersion, currentVersion),
59+
Message: fmt.Sprintf("%s is less than the current target %s, but rollbacks and downgrades are not recommended", targetVersion, currentVersion),
10860
Name: p.Name(),
10961
}
11062
}

‎pkg/payload/precondition/clusterversion/rollback_test.go

Lines changed: 20 additions & 166 deletions
Original file line numberDiff line numberDiff line change
@@ -13,186 +13,40 @@ func TestRollbackRun(t *testing.T) {
1313

1414
tests := []struct {
1515
name string
16-
clusterVersion configv1.ClusterVersion
16+
currVersion string
17+
desiredVersion string
1718
expected string
1819
}{
1920
{
20-
name: "update",
21-
clusterVersion: configv1.ClusterVersion{
22-
Spec: configv1.ClusterVersionSpec{
23-
DesiredUpdate: &configv1.Update{
24-
Version: "1.0.1",
25-
},
26-
},
27-
Status: configv1.ClusterVersionStatus{
28-
Desired: configv1.Release{
29-
Version: "1.0.0",
30-
},
31-
},
32-
},
33-
expected: "",
21+
name: "update",
22+
currVersion: "1.0.0",
23+
desiredVersion: "1.0.1",
24+
expected: "",
3425
},
3526
{
36-
name: "no change",
37-
clusterVersion: configv1.ClusterVersion{
38-
Spec: configv1.ClusterVersionSpec{
39-
DesiredUpdate: &configv1.Update{
40-
Version: "1.0.0",
41-
},
42-
},
43-
Status: configv1.ClusterVersionStatus{
44-
Desired: configv1.Release{
45-
Version: "1.0.0",
46-
},
47-
},
48-
},
49-
expected: "",
27+
name: "no change",
28+
currVersion: "1.0.0",
29+
desiredVersion: "1.0.0",
30+
expected: "",
5031
},
5132
{
52-
name: "rollback no history",
53-
clusterVersion: configv1.ClusterVersion{
54-
Spec: configv1.ClusterVersionSpec{
55-
DesiredUpdate: &configv1.Update{
56-
Version: "1.0.0",
57-
},
58-
},
59-
Status: configv1.ClusterVersionStatus{
60-
Desired: configv1.Release{
61-
Version: "1.0.1",
62-
},
63-
},
64-
},
65-
expected: "1.0.0 is less than the current target 1.0.1, and the cluster has no previous Semantic Version",
66-
},
67-
{
68-
name: "rollback to previous in the same minor version",
69-
clusterVersion: configv1.ClusterVersion{
70-
Spec: configv1.ClusterVersionSpec{
71-
DesiredUpdate: &configv1.Update{
72-
Version: "1.0.0",
73-
},
74-
},
75-
Status: configv1.ClusterVersionStatus{
76-
Desired: configv1.Release{
77-
Version: "1.0.1",
78-
},
79-
History: []configv1.UpdateHistory{
80-
{
81-
State: configv1.CompletedUpdate,
82-
Version: "1.0.1",
83-
},
84-
{
85-
State: configv1.CompletedUpdate,
86-
Version: "1.0.0",
87-
},
88-
},
89-
},
90-
},
91-
expected: "",
92-
},
93-
{
94-
name: "rollback to previous completed with intermediate partial",
95-
clusterVersion: configv1.ClusterVersion{
96-
Spec: configv1.ClusterVersionSpec{
97-
DesiredUpdate: &configv1.Update{
98-
Version: "1.0.0",
99-
},
100-
},
101-
Status: configv1.ClusterVersionStatus{
102-
Desired: configv1.Release{
103-
Version: "1.0.2",
104-
Image: "example.com/image:1.0.2",
105-
},
106-
History: []configv1.UpdateHistory{
107-
{
108-
State: configv1.PartialUpdate,
109-
Version: "1.0.2",
110-
Image: "example.com/image:1.0.2",
111-
},
112-
{
113-
State: configv1.PartialUpdate,
114-
Version: "1.0.1",
115-
Image: "example.com/image:1.0.1",
116-
},
117-
{
118-
State: configv1.CompletedUpdate,
119-
Version: "1.0.0",
120-
Image: "example.com/image:1.0.0",
121-
},
122-
},
123-
},
124-
},
125-
expected: "1.0.0 is less than the current target 1.0.2, and the only supported rollback is to the cluster's previous version 1.0.1 (example.com/image:1.0.1)",
126-
},
127-
{
128-
name: "rollback to previous completed with intermediate image change",
129-
clusterVersion: configv1.ClusterVersion{
130-
Spec: configv1.ClusterVersionSpec{
131-
DesiredUpdate: &configv1.Update{
132-
Version: "1.0.0",
133-
},
134-
},
135-
Status: configv1.ClusterVersionStatus{
136-
Desired: configv1.Release{
137-
Version: "1.0.1",
138-
Image: "example.com/image:multi-arch",
139-
},
140-
History: []configv1.UpdateHistory{
141-
{
142-
State: configv1.CompletedUpdate,
143-
Version: "1.0.1",
144-
Image: "example.com/image:multi-arch",
145-
},
146-
{
147-
State: configv1.CompletedUpdate,
148-
Version: "1.0.1",
149-
Image: "example.com/image:single-arch",
150-
},
151-
{
152-
State: configv1.CompletedUpdate,
153-
Version: "1.0.0",
154-
},
155-
},
156-
},
157-
},
158-
expected: "1.0.0 is less than the current target 1.0.1, and the only supported rollback is to the cluster's previous version 1.0.1 (example.com/image:single-arch)",
159-
},
160-
{
161-
name: "rollback to previous in an earlier minor release",
162-
clusterVersion: configv1.ClusterVersion{
163-
Spec: configv1.ClusterVersionSpec{
164-
DesiredUpdate: &configv1.Update{
165-
Version: "1.0.0",
166-
},
167-
},
168-
Status: configv1.ClusterVersionStatus{
169-
Desired: configv1.Release{
170-
Version: "2.0.0",
171-
},
172-
History: []configv1.UpdateHistory{
173-
{
174-
State: configv1.CompletedUpdate,
175-
Version: "2.0.0",
176-
},
177-
{
178-
State: configv1.CompletedUpdate,
179-
Version: "1.0.0",
180-
},
181-
},
182-
},
183-
},
184-
expected: "1.0.0 is less than the current target 2.0.0 and matches the cluster's previous version, but rollbacks that change major or minor versions are not recommended",
33+
name: "rollback",
34+
currVersion: "1.0.1",
35+
desiredVersion: "1.0.0",
36+
expected: "1.0.0 is less than the current target 1.0.1, but rollbacks and downgrades are not recommended",
18537
},
18638
}
18739

18840
for _, tc := range tests {
18941
t.Run(tc.name, func(t *testing.T) {
190-
tc.clusterVersion.ObjectMeta.Name = "version"
191-
cvLister := fakeClusterVersionLister(t, &tc.clusterVersion)
192-
instance := NewRollback(cvLister)
42+
instance := NewRollback(func() configv1.Release {
43+
return configv1.Release{
44+
Version: tc.currVersion,
45+
}
46+
})
19347

19448
err := instance.Run(ctx, precondition.ReleaseContext{
195-
DesiredVersion: tc.clusterVersion.Spec.DesiredUpdate.Version,
49+
DesiredVersion: tc.desiredVersion,
19650
})
19751
switch {
19852
case err != nil && len(tc.expected) == 0:

0 commit comments

Comments
 (0)