Skip to content

Update ClusterVersion with a desired field and a history field#153

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
smarterclayton:update_cluster_version
Jan 8, 2019
Merged

Update ClusterVersion with a desired field and a history field#153
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
smarterclayton:update_cluster_version

Conversation

@smarterclayton
Copy link
Copy Markdown
Contributor

The current field is not useful because it doesn't represent level
driven flows well. Instead, track the version the cluster is working
towards as status.desired and each time that target changes update
the status.history field. The status.history field allows us to
know what previous updates were made and what times those completed.

Even though rollback is not desirable, we need to report a field that
indicates what the last successfully applied version was (user starts
at 4.0.0, then upgrades to 4.0.1, which fails, then a new 4.0.2
version is released that also fails, we need to report 4.0.0 as the
version to go back to).

Implemented in openshift/cluster-version-operator#63 and moved here to get final review.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 4, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 4, 2019
@smarterclayton
Copy link
Copy Markdown
Contributor Author

/assign @abhinavdahiya

@smarterclayton
Copy link
Copy Markdown
Contributor Author

@deads2k as well

// This value may be empty during cluster startup, and then will be updated
// when a new update is being applied. Use the conditions array to know whether
// the update is complete. Only a limited amount of update history is preserved.
History []UpdateHistory `json:"history"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

document the ordering. Most recent to least recent or least recent to most recent

Comment thread config/v1/types_cluster_version.go Outdated
CompletionTime *metav1.Time `json:"completionTime"`

// version is a semantic versioning identifying the update version. When this
// field is part of spec, version is optional if payload is specified.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like to have an indication in the payload of what version it is. I don't think it should ever be optional here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's optional if the payload doesn't have a version. Some payloads won't have a version. Also, the CVO may not know what the payload version is before it is requested (you can provide only an image and we have to honor it, and if you provide a false version we have to distinguish that as well).

In general, yes, we will have a version, but we need to be clear you can't always get one.

Comment thread config/v1/types_cluster_version.go Outdated
// +optional
Version string `json:"version"`
// payload is a container image location that contains the update. When this
// field is part of spec, payload is optional if version is specified and the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

even though it's not technically required, don't we always know this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is possible for a release to not have a payload version defined (version must be a semantic version), or for us not to be able to retrieve the payload by image and thus get access to the version number (image -> version)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated with the different godoc (payload is required on update history because a spec update that has version, no payload, and is missing availableUpdate would not result in an update history item).

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Jan 7, 2019

If we have the information available, I'd like to always specify both version and payload.

Comment thread config/v1/types_cluster_version.go
@smarterclayton smarterclayton force-pushed the update_cluster_version branch from db1d49f to 3cd30b1 Compare January 7, 2019 22:01
The current field is not useful because it doesn't represent level
driven flows well. Instead, track the version the cluster is working
towards as `status.desired` and each time that target changes update
the `status.history` field. The `status.history` field allows us to
know what previous updates were made and what times those completed.

Even though rollback is not desirable, we need to report a field that
indicates what the last successfully applied version was (user starts
at 4.0.0, then upgrades to 4.0.1, which fails, then a new 4.0.2
version is released that also fails, we need to report 4.0.0 as the
version to go back to).
@smarterclayton smarterclayton force-pushed the update_cluster_version branch from 3cd30b1 to cae4137 Compare January 8, 2019 17:58
@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Jan 8, 2019

The inability to pull makes sense so clients have to tolerate.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2019
@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [deads2k,smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 14e4f9c into openshift:master Jan 8, 2019
sttts added a commit to sttts/origin that referenced this pull request Jan 11, 2019
abhinavdahiya added a commit to abhinavdahiya/console that referenced this pull request Jan 11, 2019
…sion

openshift/api#153 moved the field for the current version as reported by the cluster version operator.
sttts added a commit to sttts/origin that referenced this pull request Jan 14, 2019
sttts added a commit to sttts/origin that referenced this pull request Jan 14, 2019
rjhowe added a commit to rjhowe/cluster-version-operator that referenced this pull request Apr 15, 2019
Fixing example command that extracts the current update image from the
ClusterVersion object to match chnages to the api <openshift/api#153>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants