enhancement_template: add API extension sections #933

sttts · 2021-10-14T14:58:07Z

Implement the enhancement template part of #908.

sttts · 2021-10-14T14:58:23Z

/assign @mfojtik @bparees

dhellmann · 2021-10-14T14:59:05Z

/cc @kikisdeliveryservice

guidelines/enhancement_template.md

bparees

my main struggle w/ this right now is it feels like most of the guidance/sections are applicable to the case of someone adding a new webhook or maybe a finalizer, but as an EP author i'm not sure that's obvious (that the sections don't apply to them).

I assume we also want guidance for people adding new api resource types, or new fields to an existing type, but if that's not what your updates are concerned with, i think it would help to clarify in the new sections exactly what kind of "api extensions" we are talking about.

bparees · 2021-10-14T15:44:18Z

guidelines/enhancement_template.md

@@ -8,6 +8,8 @@ reviewers:
 approvers:
  - TBD
  - "@oscardoe"
+api-approvers:
+  - TBD


should this be optional (only applicable if the EP defines a new api or updates an existing one)?

Would like to link to the list of possible approvers. https://github.com/openshift/api/blob/master/OWNERS is not that. An OWNERS_ALIASES file might work as in upstream.

We're planning to change the way we list reviewers generally, to include the scope of review being requested and other details to help the final approver ensure that all areas are covered. I think we can address the question of how to find people for different types of reviews as part of that change.

bparees · 2021-10-14T15:45:24Z

guidelines/enhancement_template.md

+  Examples:
+    - adds a finalizer to namespaces. Namespace cannot be deleted without our controller running.
+    - restricts the label format for objects to X.
+    - defaults field Y on object kind Z.


would you also expect new resource types, and/or new fields being added to existing resource types, to be described here?

(I would, though they also tend to end up in the implementation details section currently)

this is about touching types that are not owned by the enhancement author. E.g. istio injecting a side-car into a pod.

hm, I don't see why "who owns the api" vs "who is writing the EP" matters? That may affect who needs to review it, but not what the design needs to cover.

what matters is the kind of change you are proposing to the product, and the effects it may have that need to be considered.

For which it seems like "api" changes need to be broken down into a few categories, e.g.:

new api type

updating existing api type (e.g. new fields)

adding a webhook (not sure if conversion vs admission need to be divided)

adding/changing validations on an apitype (adding is usually not allowed, of course)

adding/changing rbac associated w/ an api type

i'm not saying you need to cover all of them in your changes here, but i think it's important we make it clear which kinds of changes these new sections are applicable to.

guidelines/enhancement_template.md

bparees · 2021-10-14T15:48:31Z

guidelines/enhancement_template.md

@@ -265,6 +278,78 @@ enhancement:
 - Will any other components on the node change? For example, changes to CSI, CRI
  or CNI may require updating that component before the kubelet.

+### Impact of API Extensions
+
+Describe the API extensions here in detail, especially their impact on a cluster:


this seems very focused on the case where a new api resource type is being added, but doesn't cover the case where someone is trying to extend an existing api with new fields or behavior.

how are "api extensions" defined?

new resource type?

new field on a resource type?

new webhook on an existing type (or being added as part of adding a new resource type)?

actually i guess it's focused on the case where a new webhook or finalizer is being added?

As stated above API Extensions is everything impacting APIs: webhooks, finalizers, CRDs.
The effect of a new API field being added should be describe throughout the enhancement discussing that kind of extension of the existing API, no?

bparees · 2021-10-14T15:50:23Z

guidelines/enhancement_template.md

+Describe the API extensions here in detail, especially their impact on a cluster:
+
+- what are the SLIs (Service Level Indicators) an administrator or support can use to 
+- determine the health of the API extensions


this could use a more explicit example for people to follow, e.g. "when api extension $foo was added, the following SLI was introduced...."

if i'm adding a new config resource for builds(for example), i am not clear what it would mean to add a new SLI for that resource, or if such a thing is even really applicable to that api change.

guidelines/enhancement_template.md

kikisdeliveryservice · 2021-10-14T16:42:46Z

guidelines/enhancement_template.md

@@ -115,6 +117,16 @@ bogged down.

 Include a story on how this proposal will be operationalized:  lifecycled, monitored and remediated at scale.

+### API Extensions


Q: why is this section so far from the Impact of API Extensions section? Is there a reason to seperate them? Does doing so impact readability?

the impact is in the details part, the listing is here, for easier reviewing.

kikisdeliveryservice

Format/cleanup/clarification comments

guidelines/enhancement_template.md

kikisdeliveryservice · 2021-10-14T16:58:47Z

guidelines/enhancement_template.md

+#### Failure Modes
+
+- describe the possible failure modes of the API extensions
+- describe how a failure or behaviour of the extension will impact the overall cluster health


Do we need "or behaviour" here? We're just talking about failure modes no?

there might be behaviours, like slowing down requests or adding finalizers, that influence cluster health.

guidelines/enhancement_template.md

soltysh

this lgtm

soltysh · 2021-10-15T11:23:46Z

guidelines/enhancement_template.md

@@ -265,6 +278,78 @@ enhancement:
 - Will any other components on the node change? For example, changes to CSI, CRI
  or CNI may require updating that component before the kubelet.

+### Impact of API Extensions
+
+Describe the API extensions here in detail, especially their impact on a cluster:


As stated above API Extensions is everything impacting APIs: webhooks, finalizers, CRDs.
The effect of a new API field being added should be describe throughout the enhancement discussing that kind of extension of the existing API, no?

openshift-ci · 2021-10-15T11:26:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh

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~~ [soltysh]

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

bparees · 2021-10-18T17:25:59Z

guidelines/enhancement_template.md

+### API Extensions
+
+API Extensions are CRDs, admission and conversion webhooks, aggregated API servers,
+finalizers, i.e. those mechanisms that change the OCP API surface and behaviour.


I commented on this elsewhere:
#933 (comment)

but at least one of these things (CRDs) is really not like the others, in terms of the "impacts that need to be considered".

Since this update to the template seems primarily concerned with the effects/implications of webhooks, aggregated apiservers, and finalizers, can we remove CRDs (and by implication, updates to existing CRDs or other api types) from this list of what an "api extension" is?

I think the other guidance in this template will make more sense for people writing EPs, if it is clear it does not apply to adding or changing a CRD.

(I think a future update to this template that covered "stuff to think about when adding/updating an api" would also be welcome, but i don't want to hold this up waiting for that. That update would include things like "are you going to have an instance of this resource in every namespace? that is a bad idea" and "what is the rbac model for this new api type?")

CRDs certainly have less operation impact. But listing them is a good idea IMO.

I fear a little that we have to try to keep the balance. We can add more and more to stop certain common mistakes from happening. But the risk is that ppl neglect certain sections because there are just too many. I notice this a lot with the risk, test and version skew section at the bottom. Often they are not seriously filled in.

can we give more guidance about when those sections are applicable? my fear is that someone adding a new CRD is going to have no idea what to put in the "SLI" section (i don't really know what we'd expect them to put there)

added few examples for CRDs.

dhellmann

These generally look like good additions to me. We will want to generalize some of them in a follow-up very soon (the operability and supportability sections, for example), but as an iterative change I think it's an improvement.

dhellmann · 2021-10-18T17:16:37Z

guidelines/enhancement_template.md

@@ -8,6 +8,8 @@ reviewers:
 approvers:
  - TBD
  - "@oscardoe"
+api-approvers:
+  - TBD


We're planning to change the way we list reviewers generally, to include the scope of review being requested and other details to help the final approver ensure that all areas are covered. I think we can address the question of how to find people for different types of reviews as part of that change.

guidelines/enhancement_template.md

sttts · 2021-10-26T12:35:41Z

@dhellmann @bparees what is missing from your side to get this merged?

dhellmann · 2021-10-26T19:14:58Z

@dhellmann @bparees what is missing from your side to get this merged?

I'm happy with this draft.

bparees · 2021-10-26T21:26:11Z

i still feel like most of what is being asked for in the new sections isn't applicable if you're "only" adding a new CRD (or adding a field to one), but i won't hold this up over it, we can see how teams do at filling it out and then iterate.

bparees · 2021-10-26T21:26:25Z

/lgtm

sttts · 2021-10-26T21:40:38Z

i still feel like most of what is being asked for in the new sections isn't applicable if you're "only" adding a new CRD (or adding a field to one)

I don't disagree. The reason is probably that CRDs have less impact on operations. Webhooks are much more critiical.

openshift-ci bot assigned bparees and mfojtik Oct 14, 2021

openshift-ci bot requested review from russellb and sjenning October 14, 2021 14:58

openshift-ci bot requested a review from kikisdeliveryservice October 14, 2021 14:59

dhellmann reviewed Oct 14, 2021

View reviewed changes

guidelines/enhancement_template.md Show resolved Hide resolved

guidelines/enhancement_template.md Outdated Show resolved Hide resolved

sttts force-pushed the sttts-api-extensions-template branch 2 times, most recently from 8ab6b6d to d7a0ce6 Compare October 14, 2021 15:33

bparees reviewed Oct 14, 2021

View reviewed changes

kikisdeliveryservice reviewed Oct 14, 2021

View reviewed changes

soltysh approved these changes Oct 15, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2021

bparees reviewed Oct 18, 2021

View reviewed changes

dhellmann reviewed Oct 18, 2021

View reviewed changes

kikisdeliveryservice reviewed Oct 20, 2021

View reviewed changes

guidelines/enhancement_template.md Outdated Show resolved Hide resolved

enhancement_template: add API extension sections

8d07520

sttts force-pushed the sttts-api-extensions-template branch from d6c03fc to 8d07520 Compare October 26, 2021 12:36

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021

openshift-merge-robot merged commit 6e1a0e5 into openshift:master Oct 26, 2021

wking mentioned this pull request Nov 5, 2021

enhancements/update/update-blocker-lifecycle: Propose a new enhancement #475

Merged

		@@ -115,6 +117,16 @@ bogged down.

		Include a story on how this proposal will be operationalized: lifecycled, monitored and remediated at scale.

		### API Extensions

enhancement_template: add API extension sections #933

enhancement_template: add API extension sections #933

Conversation

sttts commented Oct 14, 2021

sttts commented Oct 14, 2021

dhellmann commented Oct 14, 2021

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 26, 2021

dhellmann commented Oct 26, 2021

bparees commented Oct 26, 2021

bparees commented Oct 26, 2021

sttts commented Oct 26, 2021