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.

Sign up for GitHub

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

Jump to bottom

Bug 1822513: Determine current version by checking for status completed #394

Merged
merged 1 commit into from
Jul 22, 2020
Merged

Bug 1822513: Determine current version by checking for status completed #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/payload/precondition/clusterversion/upgradable_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func TestUpgradeableRun(t *testing.T) {
},
}
if len(tc.currVersion) > 0 {
clusterVersion.Status.History = append(clusterVersion.Status.History, configv1.UpdateHistory{Version: tc.currVersion})
clusterVersion.Status.History = append(clusterVersion.Status.History, configv1.UpdateHistory{Version: tc.currVersion, State: configv1.CompletedUpdate})
}
if tc.upgradeable != nil {
clusterVersion.Status.Conditions = append(clusterVersion.Status.Conditions, configv1.ClusterOperatorStatusCondition{
Expand Down
18 changes: 17 additions & 1 deletion pkg/payload/precondition/clusterversion/upgradeable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,10 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
return nil
}

currentMinor := getEffectiveMinor(cv.Status.History[0].Version)
currentVersion := getCurrentVersion(cv.Status.History)
currentMinor := getEffectiveMinor(currentVersion)
desiredMinor := getEffectiveMinor(releaseContext.DesiredVersion)
klog.V(5).Infof("currentMinor %s releaseContext.DesiredVersion %s desiredMinor %s", currentMinor, releaseContext.DesiredVersion, desiredMinor)

// 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 {
Expand All @@ -82,6 +84,20 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
// Name returns Name for the precondition.
func (pf *Upgradeable) Name() string { return "ClusterVersionUpgradeable" }

// getCurrentVersion determines and returns the cluster's current version by iterating through the
// provided update history until it finds the first version with update State of Completed. If a
// Completed version is not found the version of the oldest history entry, which is the originally
// installed version, is returned.
func getCurrentVersion(history []configv1.UpdateHistory) string {
for _, h := range history {
if h.State == configv1.CompletedUpdate {
klog.V(5).Infof("Cluster current version=%s", h.Version)
return h.Version
}
}
return history[len(history)-1].Version
}
Copy link
Member

Choose a reason for hiding this comment

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

What about partial upgrades? I guess it changes the current version.

Copy link
Member

Choose a reason for hiding this comment

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

We should look at how oc get clusterversion shows the current version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any upgrade, even one that immediately fails preconditions gets inserted into the version history at index 0 and then that is used to check if upgrade is for a z-level upgrade. And as I said it is then comparing desired to itself. If I understand your question correctly, no, a partial update would not change my calculated "current version" because it would not have State Completed. I don't see how a cluster can be considered at a given version if the version upgrade was not completed. So it really comes down to what defines a cluster being at a given version. It can't simply be that the version shows up in the version history and I would hope we would not go down the path of trying to figure out how partial a partial upgrade was and if it got far enough along to consider the cluster at that version.

Copy link
Contributor Author

@jottofar jottofar Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We should look at how oc get clusterversion shows the current version.

Gives the completed version and shows upgrade progress:

NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.5.0-rc.1 True True 6m17s Working towards 4.5.0-rc.2: 70% complete

And when I stepped on this upgrade with another before it was completed (there are three entries in version history now):

NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.5.0-rc.1 True True 8m27s Working towards quay.io/openshift-release-dev/ocp-release-nightly@sha256:cec724b008c35aaf36aeea8fdfc3f22f3f735d772d05f392acd86610de1608cd: downloading update

And finally when Completed:

NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.6.0-0.nightly-2020-07-15-170241 True False 69s Cluster version is 4.6.0-0.nightly-2020-07-15-170241

Copy link
Member

Choose a reason for hiding this comment

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

@jottofar I asked the question because when a cluster is partially updated, customers can try updating again to a newer version e.g. (in 4.4. fast channel)

  • first upgrade attempted from 4.4.3 to 4.4.6.
  • next upgrade 4.4.6 to 4.4.10 which ended as a partial upgrade
  • The upgrade again attempted to 4.4.11. Now when CVO checks if there is an edge exists to 4.4.11 what current version it uses?

Copy link
Member

@LalatenduMohanty LalatenduMohanty Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Or the question which version it should use as current version? You are suggesting that we should use 4.4.6 as the current version as the last successful upgrade was to that version.

Copy link
Member

@LalatenduMohanty LalatenduMohanty Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure what qualifies for a partial upgrade? I am wondering if the operators like api server, etcd has upgraded (control plane) i.e. moved to the next version but others are not. Does it make any difference ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or the question which version it should use as current version? You are suggesting that we should use 4.4.6 as the current version as the last successful upgrade was to that version.

In your example, yes. It's just like my example above it and that's what 'oc get currentversion' would show as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use a partial version we would have to introduce logic to figure out what makes a partial version "good enough" to be considered the current version. And as I said, this would differ from what the system returns as current version now which is the last version with state Completed.


// 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 {
Expand Down