add proposal for a process for api-reviews #221
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,202 @@ | ||||||
| # Overview | ||||||
| This proposal outlines the establishment of an API review process within KubeVirt project. The primary objective of this | ||||||
| process is to ensure the quality, consistency, and usability of the project's APIs, enabling developers to effectively | ||||||
| interact with our software. By implementing an API review process, the aim is to enhance the project's overall | ||||||
| development experience, encourage collaboration, and foster community engagement. | ||||||
|
|
||||||
| Kubernetes core APIs have been very successful in maintaining strong backward compatibility along with good usability | ||||||
| over the years. This proposal uses the Kubernetes API review process as a guide for implementing a similar process for | ||||||
| KubeVirt. | ||||||
|
|
||||||
|
|
||||||
| ## Motivation | ||||||
| Since KubeVirt has reached v1 and has some APIs with v1 version, implementing an API review process | ||||||
| ensures code quality, usability, stability, and fosters community collaboration, ultimately leading to a successful and | ||||||
| sustainable software ecosystem. | ||||||
|
|
||||||
| More importantly, it allows for a common framework through which all stake-holders can discuss and decide API facing | ||||||
| changes in KubeVirt. | ||||||
|
|
||||||
| Currently, the project has a very hand-wavy mechanism for contributing, reviewing and discussion API facing changes. This | ||||||
| document attempts to specific focus areas where improvement is needed in the Goals section below and proposal for | ||||||
| implementing the improvements in the design section. | ||||||
|
|
||||||
|
|
||||||
| ## Goals | ||||||
|
|
||||||
| - Establish a policy for API evolution | ||||||
| - Establish a process for API reviews such that: | ||||||
| - contributors have a clear idea about how to implement an API facing change | ||||||
| - reviews have a guideline about the necessary checks required for a successful API facing change | ||||||
| - community has a guideline about how to handle API breakages upon upgrade | ||||||
|
There was a problem hiding this comment. An effort has started to define a feature-lifecycle, discussing the different stages features go through, including API introduction and changes. |
||||||
| - API fields in an object could belong to different features. Depending on the maturity of the feature, the field | ||||||
| could evolve differently. | ||||||
| - propose necessary tools to make process easier and reliable, and avoid human errors | ||||||
|
|
||||||
| ## Non Goals | ||||||
| - Upgrades breaking due to behavioural changes, e.g. change in implementation of a controller | ||||||
|
|
||||||
| ## Definition of Users | ||||||
| - API Reviewers | ||||||
| - Contributors | ||||||
|
|
||||||
| ## User Stories | ||||||
| - As a stakeholder of KubeVirt project, I would like the project to have a crip policy of how APIs will evolve across | ||||||
|
|
||||||
| different versions. | ||||||
| - As a KubeVirt cluster operator, consuming the API by creating objects (manually or programmatically), I want KubeVirt to | ||||||
| maintain an intuitive, stable and simple API, that can be used as a foundational block to build products and projects | ||||||
| - As a contributor, I need guidance on the right way to approach API facing change. Ideally this guidance should include | ||||||
| all the steps: design docs, contributing the change and post contribution steps | ||||||
| - As a reviewer, I need to have a comprehensive list of checks needed for approving an API facing change | ||||||
| - As a reviewer, ideally, I need to be able to leverage automation wherever possible to make reviewing easier | ||||||
|
|
||||||
| ## Repos | ||||||
| - https://github.com/kubevirt/kubevirt | ||||||
|
|
||||||
| # Design | ||||||
|
|
||||||
| In order to achieve the stability, quality and consistency of APIs like the core Kubernetes APIs this document proposes | ||||||
| the following changes: | ||||||
|
|
||||||
| - For all stakeholders: Describe a policy of API evolution | ||||||
| - For API reviewers: There should be a one or more engineers that review api breaking changes on a regular basis. | ||||||
| - For contributors: There should be a guide explaining how to merge and api change | ||||||
| - Tools and tests: Tools and automation that can be helpful to reduce human burden and errors to carry-out api changes | ||||||
|
|
||||||
| More details on each of the aforementioned items is highlighted below in separate sections. | ||||||
|
|
||||||
alaypatel07 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| ## Policy of API Evolution | ||||||
| - Alpha features should always introduce an optional field to APIs that have GA'ed or in Beta | ||||||
| - Alpha features and hence by extensions fields that are introduced behind a feature flag, is not guaranteed to work | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| in all future versions. Hence, they should not be deployed in production. This ensures that new feature development | ||||||
| work is not slowed down by requirement to maintain api-compatibility. Even though compatibility for experimental or | ||||||
| alpha APIs is not strictly required, but breaking compatibility should not be done lightly, as it disrupts all users | ||||||
| of the feature. | ||||||
| - Beta or GA features that introduces new fields to beta or stable APIs should not break compatibility | ||||||
|
There was a problem hiding this comment. Is this really achieveable for our project? It sounds like a bold commitment. There was a problem hiding this comment. We really need to aspire for this goal and make tools/processes that can help make it achievable. It's hard but not impossible. Following are some of the reasons why this is needed:
This proposal talk about some of the tools and processes to make this achievable:
tagging @xpivarc @EdDev @rthallisey if you guys have additional thoughts on the above. |
||||||
|
|
||||||
| ## Contributors responsibilities | ||||||
| - Contributors must explain why the API change is needed, which functionality it adds and who requested it. They should | ||||||
| include links to github issues, email threads or any other public document where users request this functionality. | ||||||
| - Contributors should contact the users who requested the functionality via the channel they have used, to give them the | ||||||
| opportunity to review the API. We do not want to define an API that the user finds inconvenient. | ||||||
|
|
||||||
| ## Process for api reviewers | ||||||
| Recent changes to reviewer guidelines recommend forming small groups in specific areas of expertise. sig-api is | ||||||
| one such group. This group will be responsible for: | ||||||
| - Reviewing all the PRs with `kind/api-change` labels | ||||||
alaypatel07 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. This is a major change in responsibility and ownership. E.g. if the SIG will not have the capacity to review the changes, will it block the PR? There was a problem hiding this comment. In order to make sure that what effect this will have, the implementation is staggered into phases. Phase 1 is where most of the pains of this new process will be evident. In order to solve for the pain, automation tools can be introduced in phase 2 and phase 3. With automation and the best practices guide, I think we can cater to the needed velocity of development. |
||||||
| - Maintaining a high quality, stable and crisp api surface that is backward compatible | ||||||
|
|
||||||
| The focus of this group should be to lay out the guidelines of an intuitive, maintainable and usable set of APIs for | ||||||
| KubeVirt project. | ||||||
|
|
||||||
| ### How to achieve this? | ||||||
|
|
||||||
| Kubernetes has a very well-defined process for api-reviews. Taking an inspiration from that, KubeVirt should have the | ||||||
| following: | ||||||
|
|
||||||
| - A specific community call for sig-api | ||||||
| - In this call all the PRs with api-facing changes will be reviewed. Check list of items to go through in the call | ||||||
| - does the PR introduce breaking changes? | ||||||
| - can the API changes be better? | ||||||
| - Any other communication that is needed for contributor to move forward. | ||||||
| - While goal of the project is to prohibit introducing API breaking changes, automation tools proposed in the Tools section | ||||||
| will help in attaining this goal. However, some APIs have reached v1 (GA) without any such tool. Hence, a well-defined | ||||||
| process is needed to address breaking changes discovered in previous versions: | ||||||
|
There was a problem hiding this comment. I think this needs to be defined with their rules set as part of the feature-lifecycle. There have been discussions on the technicalities of alpha fields which can be removed, are these details going to enter in this proposal or some appendix? There was a problem hiding this comment. I have added the conclusion of the discussions about alpha fields in
In general this topic relates to a change that is in a GA field which is not supposed to break backward compatibility, but breaks compat due to oversight, how does the community react to that? The following lines provide a guideline for dealing with that. |
||||||
| - If a break in API is reported, the next release: | ||||||
| - will introduce the fix | ||||||
| - if the fix is burdensome, the next release will have deprecation warnings | ||||||
| - Depending on the support burden, after letting the fix is in place and appropriate deprecation warnings raised for | ||||||
| a minimum of 3 releases (around a year), the community can start the process of removing the deprecated fields using | ||||||
| the normal API removal process. | ||||||
|
|
||||||
| ## Process for contributors | ||||||
|
|
||||||
| In order for the process to work efficiently, contributors should receive the right support and guidance when | ||||||
| contributing api-facing changes. | ||||||
|
|
||||||
| ### How to achieve this? | ||||||
|
|
||||||
| - Have a document to describe the right process for contributors with the following details: | ||||||
| - Any API-facing change with design doc can be reviewed by the sig-api for initial feedback. | ||||||
| - Any API-facing change PR can be brought up for the discussion in sig-api call | ||||||
| - Link to a conventions document for good practices and guidance | ||||||
|
There was a problem hiding this comment. Is this going to be part of the current proposal? There was a problem hiding this comment. no, it wasnt intended to be added in this proposal. This will be once the sig has collected enough good practices, I can start seeding such a document |
||||||
|
|
||||||
| ## Tools for reviewers | ||||||
|
|
||||||
| The easiest part about checking for good api-facing change is one that maintains backward compatibility. API objects are | ||||||
| serialized to and from JSON when clients interact with API server. The testing for serialization could be automated to | ||||||
| make sure that APIs continue to be serializable upon upgrades | ||||||
|
|
||||||
| ### Tool to test serialization upon upgrade | ||||||
| Here is a [demo](https://github.com/alaypatel07/KubeVirt-api-fuzzer) tool that identifies api breakages while reading | ||||||
| older objects using newer clients. | ||||||
|
|
||||||
| ##### Description and Usage | ||||||
| 1. This tool creates JSON and YAML files for all the API exposed by KubeVirt in group-version "kubevirt.io/v1", | ||||||
| versioned by the release. The current version is in `HEAD` directory, previous versions are in `release-0.yy` release | ||||||
| directory. The following APIs are included, more APIs can be added in the future: | ||||||
| ``` | ||||||
| VirtualMachineInstance | ||||||
| VirtualMachineInstanceList | ||||||
| VirtualMachineInstanceReplicaSet | ||||||
| VirtualMachineInstanceReplicaSetList | ||||||
| VirtualMachineInstancePreset | ||||||
| VirtualMachineInstancePresetList | ||||||
| VirtualMachineInstanceMigration | ||||||
| VirtualMachineInstanceMigrationList | ||||||
| VirtualMachine | ||||||
| VirtualMachineList | ||||||
| KubeVirt | ||||||
| KubeVirtList | ||||||
| ``` | ||||||
| 2. Upon any change to API, the json and YAML files will be updated in HEAD directory. | ||||||
| 3. When KubeVirt cuts a new release of the project, the files in HEAD directory will be copied to the release version and | ||||||
| future development branch will add a unit test for past two releases: | ||||||
| ``` | ||||||
| $ VERSION=release-0.60 | ||||||
| $ cp -fr testdata/{HEAD,${VERSION}} | ||||||
| ``` | ||||||
|
|
||||||
| ##### How will it help? | ||||||
|
|
||||||
| During KubeVirt upgrade, the apiserver is updated last, i.e. for a moment in time until the upgrade rolls out, KubeVirt | ||||||
| components like virt-handler, virt-controller will have newer client, but apiserver will be serving older objects. | ||||||
|
|
||||||
| Using this tests, it can be asserted that the current newer clients can roundtrip (serialized and de-serialized) past | ||||||
| two releases, which will make the upgrade safer. | ||||||
|
|
||||||
|
|
||||||
| ## API Examples | ||||||
| Tool usage example: https://github.com/alaypatel07/kubevirt-api-fuzzer#usage | ||||||
|
|
||||||
| ## Scalability | ||||||
| TODO | ||||||
|
|
||||||
| ## Update/Rollback Compatibility | ||||||
| (does this impact update compatibility and how) | ||||||
|
|
||||||
| ## Functional Testing Approach | ||||||
| (an overview on the approaches used to functional test this design) | ||||||
|
|
||||||
| # Implementation Phases | ||||||
|
|
||||||
| The process for achieving api-stability will involve the following: | ||||||
|
|
||||||
| ### Phase 1 | ||||||
|
|
||||||
| - creation of a sig-api that will meet on a regular cadence for the following: | ||||||
| - triage all the open PRs with `kind/api-change` label | ||||||
| - interested reviewers will pick up PRs and design documents for review | ||||||
| - discuss common observed patterns in the api reviews | ||||||
| - the PRs are not gated on api-review approval | ||||||
| - This is the initial phase where api reviews will be tried in the community. This phase will last atleast 1 release, | ||||||
| currently planned for release 1.2 | ||||||
|
|
||||||
| ### Phase 2 | ||||||
|
|
||||||
| - develop automation to help PR reviewers find breaking changes | ||||||
| - PRs will be gated on api-review approval | ||||||
| - identify additional work like e2e upgrade test to enhance API stability | ||||||
|
|
||||||
| ### Phase 3 | ||||||
| - implementation and addition of upgrade tests | ||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This seems to describe what an API in general provides, i.e. a contract.
What is not very clear to me is what is missing or what needs to be solved. I think it is important to give a good reasoning so we can remind ourself the missing part and their implications.
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.
My intent was to describe the missing parts in the rest of the document. Hence, I have added a statement that the focus-areas where improvement is needed is described in the goals section and how to go about those focus areas in design section.