-
Notifications
You must be signed in to change notification settings - Fork 185
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
Bug 1797624: loosen upgradeable condition to allow z-level upgrades #291
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
package clusterversion | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/openshift/cluster-version-operator/pkg/payload/precondition" | ||
|
||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
||
configv1 "github.com/openshift/api/config/v1" | ||
configv1listers "github.com/openshift/client-go/config/listers/config/v1" | ||
|
||
"k8s.io/client-go/tools/cache" | ||
) | ||
|
||
func TestGetEffectiveMinor(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
input string | ||
expected string | ||
}{ | ||
{ | ||
name: "empty", | ||
input: "", | ||
expected: "", | ||
}, | ||
{ | ||
name: "invalid", | ||
input: "something@very-differe", | ||
expected: "", | ||
}, | ||
{ | ||
name: "multidot", | ||
input: "v4.7.12.3+foo", | ||
expected: "7", | ||
}, | ||
{ | ||
name: "single", | ||
input: "v4.7", | ||
expected: "7", | ||
}, | ||
} | ||
|
||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
actual := getEffectiveMinor(tc.input) | ||
if tc.expected != actual { | ||
t.Error(actual) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestUpgradeableRun(t *testing.T) { | ||
ptr := func(status configv1.ConditionStatus) *configv1.ConditionStatus { | ||
return &status | ||
} | ||
|
||
tests := []struct { | ||
name string | ||
upgradeable *configv1.ConditionStatus | ||
currVersion string | ||
desiredVersion string | ||
expected string | ||
}{ | ||
{ | ||
name: "first", | ||
desiredVersion: "4.2", | ||
expected: "", | ||
}, | ||
{ | ||
name: "move-y, no condition", | ||
currVersion: "4.1", | ||
desiredVersion: "4.2", | ||
expected: "", | ||
}, | ||
{ | ||
name: "move-y, with true condition", | ||
upgradeable: ptr(configv1.ConditionTrue), | ||
currVersion: "4.1", | ||
desiredVersion: "4.2", | ||
expected: "", | ||
}, | ||
{ | ||
name: "move-y, with unknown condition", | ||
upgradeable: ptr(configv1.ConditionUnknown), | ||
currVersion: "4.1", | ||
desiredVersion: "4.2", | ||
expected: "", | ||
}, | ||
{ | ||
name: "move-y, with false condition", | ||
upgradeable: ptr(configv1.ConditionFalse), | ||
currVersion: "4.1", | ||
desiredVersion: "4.2", | ||
expected: "set to False", | ||
}, | ||
{ | ||
name: "move-z, with false condition", | ||
upgradeable: ptr(configv1.ConditionFalse), | ||
currVersion: "4.1.3", | ||
desiredVersion: "4.1.4", | ||
expected: "", | ||
}, | ||
} | ||
|
||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
clusterVersion := &configv1.ClusterVersion{ | ||
ObjectMeta: metav1.ObjectMeta{Name: "version"}, | ||
Spec: configv1.ClusterVersionSpec{}, | ||
Status: configv1.ClusterVersionStatus{ | ||
History: []configv1.UpdateHistory{}, | ||
}, | ||
} | ||
if len(tc.currVersion) > 0 { | ||
clusterVersion.Status.History = append(clusterVersion.Status.History, configv1.UpdateHistory{Version: tc.currVersion}) | ||
} | ||
if tc.upgradeable != nil { | ||
clusterVersion.Status.Conditions = append(clusterVersion.Status.Conditions, configv1.ClusterOperatorStatusCondition{ | ||
Type: configv1.OperatorUpgradeable, | ||
Status: *tc.upgradeable, | ||
Message: fmt.Sprintf("set to %v", *tc.upgradeable), | ||
}) | ||
} | ||
cvLister := fakeClusterVersionLister(clusterVersion) | ||
instance := NewUpgradeable(cvLister) | ||
|
||
err := instance.Run(context.TODO(), precondition.ReleaseContext{DesiredVersion: tc.desiredVersion}) | ||
switch { | ||
case err != nil && len(tc.expected) == 0: | ||
t.Error(err) | ||
case err != nil && err.Error() == tc.expected: | ||
case err != nil && err.Error() != tc.expected: | ||
t.Error(err) | ||
case err == nil && len(tc.expected) == 0: | ||
case err == nil && len(tc.expected) != 0: | ||
t.Error(err) | ||
} | ||
|
||
}) | ||
} | ||
} | ||
|
||
func fakeClusterVersionLister(clusterVersion *configv1.ClusterVersion) configv1listers.ClusterVersionLister { | ||
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) | ||
if clusterVersion == nil { | ||
return configv1listers.NewClusterVersionLister(indexer) | ||
} | ||
|
||
indexer.Add(clusterVersion) | ||
return configv1listers.NewClusterVersionLister(indexer) | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -2,6 +2,7 @@ package clusterversion | |||
|
||||
import ( | ||||
"context" | ||||
"strings" | ||||
|
||||
configv1 "github.com/openshift/api/config/v1" | ||||
configv1listers "github.com/openshift/client-go/config/listers/config/v1" | ||||
|
@@ -29,7 +30,7 @@ func NewUpgradeable(lister configv1listers.ClusterVersionLister) *Upgradeable { | |||
// Run runs the Upgradeable precondition. | ||||
// If the feature gate `key` is not found, or the api for clusterversion doesn't exist, this check is inert and always returns nil error. | ||||
// Otherwise, if Upgradeable condition is set to false in the object, it returns an PreconditionError when possible. | ||||
func (pf *Upgradeable) Run(ctx context.Context) error { | ||||
func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.ReleaseContext) error { | ||||
cv, err := pf.lister.Get(pf.key) | ||||
if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) { | ||||
return nil | ||||
|
@@ -42,16 +43,43 @@ func (pf *Upgradeable) Run(ctx context.Context) error { | |||
Name: pf.Name(), | ||||
} | ||||
} | ||||
if up := resourcemerge.FindOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable); up != nil && up.Status == configv1.ConditionFalse { | ||||
return &precondition.Error{ | ||||
Nested: err, | ||||
Reason: up.Reason, | ||||
Message: up.Message, | ||||
Name: pf.Name(), | ||||
} | ||||
|
||||
// if we are upgradeable==true we can always upgrade | ||||
up := resourcemerge.FindOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable) | ||||
if up == nil || up.Status != configv1.ConditionFalse { | ||||
return nil | ||||
} | ||||
|
||||
// we can always allow the upgrade if there isn't a version already installed | ||||
if len(cv.Status.History) == 0 { | ||||
return nil | ||||
} | ||||
|
||||
currentMinor := getEffectiveMinor(cv.Status.History[0].Version) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of pulling this out of the cluster's ClusterVersion history, I'd rather pull it from the local state. But then you'd have to pipe it through to this precondition code. Including it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not the way I would do it since we're already getting the information here (pre-existing gets) and you're already getting cached data, so it seems unnecessary and odd. Could you do it as a follow-up instead if you really want it after? |
||||
desiredMinor := getEffectiveMinor(releaseContext.DesiredVersion) | ||||
|
||||
// if there is no difference in the minor version (4.y.z where 4.y is the same for current and desired), then we can still upgrade | ||||
if currentMinor == desiredMinor { | ||||
return nil | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may want to build the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that seems a little weird to do, since people are unlikely to wonder why their desired update was allowed and succeeded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I can see component authors wondering why an update was initiated despite them having set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was discussing bug https://bugzilla.redhat.com/show_bug.cgi?id=1822513 with @wking and he suggested I update here as a follow-up to this thread.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @deads2k thoughts on the above? |
||||
} | ||||
|
||||
return &precondition.Error{ | ||||
Nested: err, | ||||
Reason: up.Reason, | ||||
Message: up.Message, | ||||
Name: pf.Name(), | ||||
} | ||||
return nil | ||||
} | ||||
|
||||
// Name returns Name for the precondition. | ||||
func (pf *Upgradeable) Name() string { return "ClusterVersionUpgradeable" } | ||||
|
||||
// getEffectiveMinor attempts to do a simple parse of the version provided. If it does not parse, the value is considered | ||||
// empty string, which works for the comparison done here for equivalence. | ||||
func getEffectiveMinor(version string) string { | ||||
splits := strings.Split(version, ".") | ||||
if len(splits) < 2 { | ||||
return "" | ||||
} | ||||
return splits[1] | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cute. this was merged without tests initially :)