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.

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

Versioning cincinnati api and json schema #870

Merged

Conversation

PratikMahajan
Copy link
Contributor

No description provided.

@PratikMahajan PratikMahajan changed the title Versioning cincinnati api and json schema [WIP]Versioning cincinnati api and json schema Aug 19, 2021
@openshift-ci 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 Aug 19, 2021
@PratikMahajan PratikMahajan force-pushed the versioning-cincinnati branch 3 times, most recently from 9d40c5e to 4871b92 Compare August 23, 2021 21:13
Copy link
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

The linter job is failing because some headers that are required from the template are missing in this doc. Please restore those headers and leave an explicit comment if you believe that section does not apply to this change.

enhancements/update/versioning-cincinnati-api.md missing "### User Stories"
enhancements/update/versioning-cincinnati-api.md missing "#### Dev Preview -> Tech Preview"
enhancements/update/versioning-cincinnati-api.md missing "#### Tech Preview -> GA"
enhancements/update/versioning-cincinnati-api.md missing "#### Removing a deprecated feature"
enhancements/update/versioning-cincinnati-api.md missing "### Upgrade / Downgrade Strategy"

enhancements/update/versioning-cincinnati-api.md Outdated Show resolved Hide resolved
@LalatenduMohanty
Copy link
Member

Mostly looks good to me. Also we should include #870 (comment)

@PratikMahajan PratikMahajan force-pushed the versioning-cincinnati branch 3 times, most recently from 603db2c to f96d7c6 Compare August 27, 2021 20:50
@PratikMahajan PratikMahajan changed the title [WIP]Versioning cincinnati api and json schema Versioning cincinnati api and json schema Aug 30, 2021
@openshift-ci 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 Aug 30, 2021
Version the Cincinnati API and JSON for easy pass around
and managing new version releases.
Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2021
#### URI Change
The current URI follows `{URL}/v1/graph`. The new un-versioned URI will follow `{URL}/graph`. The existing URI can be
redirected to the new URI. This can be done through cincinnati. In the case where an old URI is being called, cincinnati
will by default return the graph that is compatible with the oldest supported version.
Copy link
Member

Choose a reason for hiding this comment

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

The oldest being v1 specifically or would a backwards compatible v1.1 be available at this endpoint too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are gonna consider both the endpoints as one. Backward compatible v1.1 will be available at this endpoint as well. Currently, we will be supporting v1 as that's the oldest graph we know. But going forward we can make decisions when to drop specific versions based on their usage.



### Risks and Mitigations
* As there are some customers that have old URI hardcoded while creating the cluster, we will have to support old URI
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider installer validation to block new installs using the versioned URI when the client expects a newer version than v1? For example if 4.10 CVO uses a v2 API we don't want to allow them to inadvertantly downrev to v1 and lose targetted edge blocking due to their configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are moving away from versioned URI to unversioned. So all requests from /v1/graph will be handled as if they're coming to /graph. Going forward we will be accepting the version using Media Type Versioning.
So, if someone hits the old URI with a correct media type version, they'll end up getting the intended graph or if someone hits the new URI without the media type version, they'll end up getting the oldest supported graph.
Installer validation will speed up the process of shifting to the new URI!

Copy link
Member

Choose a reason for hiding this comment

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

So the conclusion is that we'd never get rid of the legacy URL. That's fine.

Cincinnati should also implement a way to understand how many clients are consuming the various versions.
This can be done by implementing metrics for old URI vs new URI in Cincinnati. Similarly, metrics should be implemented
to understand the consumption of various media types (versions). This will help us to make decisions on deprecating the
old versions and URI. For example, a single `cincinnati_pe_http_response_total` counter with labels for `uri` and `version`
Copy link
Member

Choose a reason for hiding this comment

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

nit: line is missing a terminal period ..

@sdodson
Copy link
Member

sdodson commented Sep 28, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LalatenduMohanty, sdodson

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [LalatenduMohanty,sdodson]

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2021
@openshift-merge-robot openshift-merge-robot merged commit 553585b into openshift:master Sep 28, 2021
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants