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

Update Aggregated Discovery for 1.26 #3534

Merged
merged 6 commits into from
Oct 5, 2022

Conversation

Jefftree
Copy link
Member

KEP-3352

  • One-line PR description: Updating milestone and comments for 1.26
  • Other comments:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Sep 21, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2022
@Jefftree
Copy link
Member Author

Jefftree commented Oct 3, 2022

Copy link
Member

@apelisse apelisse left a comment

Choose a reason for hiding this comment

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

Sounds good.

9. While we aren't exactly like an unversioned new endpoint (v1 from
start), we want to deliver the feature (improves clients) without
giving the perception that the API is perfect

Copy link
Contributor

Choose a reason for hiding this comment

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

Also worth noting that this API is a GET, so the client doesn't provide a schema'd type, and this is not persisted in the cluster?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is covered by the first point that Discovery is a non-resource API class. Would you like it to be elaborated?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is covered by the first point that Discovery is a non-resource API class. Would you like it to be elaborated?

we have resource APIs that are not persisted, but still have client provided schema. See subjectaccessreviews. It even happens for other GET APIs, like pods/logs.

I think being explicit here will help future us.

@@ -24,16 +25,16 @@ prr-approvers:
- "@deads2k"

# The target maturity stage in the current dev cycle for this KEP.
stage: alpha
stage: beta
Copy link
Contributor

Choose a reason for hiding this comment

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

@smarterclayton my understanding was that this still started with an alpha featuregate, but a beta serialization. So effectively it was off-by-default, but stable for clients if available.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand an alpha feature gate gating a beta API... off by default seems reasonable, but we can do a beta off-by-default gate

Copy link
Contributor

@smarterclayton smarterclayton Oct 4, 2022

Choose a reason for hiding this comment

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

I think from a clarity perspective "off by default" is the first phase of non-resource APIs and the next phase is "on by default and can't evolve it" (i.e. metrics is 'stable' once we turn it on by default since we have no versioning scheme). We have no question here whether we want to evolve our discovery API to add "at once" retrieval (so it's not alpha because we need feedback first). We have a versioning scheme here, so "off by default" but "beta" doesn't prevent us from evolving or killing the API either. So the necessary preconditions for not being treated as an alpha "experiment" are in place, in my mind.

I think based on what we've demonstrated we would be comfortable with users turning this flag on within one release, because the mechanisms give us the ability to evolve, and if we decided not to graduate it would be no different from any other beta feature being dropped. So beta gate is based on assessed confidence in the feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand an alpha feature gate gating a beta API.

The featuregate is also gating the new controllers that are providing the data and the (hopefully settling) etag calculation across multiple HA servers. The serialization is based on experience, so beta stability serialization is logical, but the code providing the data to serialize are fully novel.

If we're going to require an "on by default beta" before going GA, what benefit have we gained by exposing unvetted controllers as beta in their first release? Calling our off-by-default alpha is consistent, doesn't preclude using a more stable serialization, and keeps maturity graduation consistent. Vendors can (and do) turn alpha features on for testing and maybe even production: see p&f and graceful node shutdown.

So beta gate is based on assessed confidence in the feature.

Is this confidence a thing that any sig can say about any feature they wish? Choosing a couple at semi-random

  1. pleg performance improvements
  2. node swap
  3. kubelet credential providers

Copy link
Member

Choose a reason for hiding this comment

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

If we're going to require an "on by default beta" before going GA, what benefit have we gained by exposing unvetted controllers as beta in their first release?

I think the plan was to do "off-by-default" beta followed by "on-by-default" GA.

Is this confidence a thing that any sig can say about any feature they wish?

I think this is a combination of the 9 points enumerated above.

@deads2k
Copy link
Contributor

deads2k commented Oct 5, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2022
@deads2k
Copy link
Contributor

deads2k commented Oct 5, 2022

/label tide-merge-method-squash

@k8s-ci-robot
Copy link
Contributor

@deads2k: The label(s) /label tide-merge-method-squash cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, lead-opted-in, tracked/no, tracked/out-of-tree, tracked/yes. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label tide-merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot merged commit 592ca1f into kubernetes:master Oct 5, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 5, 2022
@rhockenbury rhockenbury mentioned this pull request Oct 7, 2022
11 tasks
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants