Promote APF API to v1 #121089

tkashem · 2023-10-09T18:25:42Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The flowcontrol.apiserver.k8s.io/v1beta3 FlowSchema and PriorityLevelConfiguration APIs has been promoted to flowcontrol.apiserver.k8s.io/v1, with the following changes:
* PriorityLevelConfiguration: the `.spec.limited.nominalConcurrencyShares` field defaults to `30` only if the field is omitted (v1beta3 also defaulted an explicit `0` value to `30`). Specifying an explicit `0` value is not allowed in the `v1` version in v1.29 to ensure compatibility with 1.28 API servers. In v1.30, explicit `0` values will be allowed in this field in the `v1` API.
The flowcontrol.apiserver.k8s.io/v1beta3 APIs are deprecated and will no longer be served in v1.32. All existing objects are available via the `v1` APIs. Transition clients and manifests to use the `v1` APIs before upgrading to v1.32.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-10-09T18:25:44Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tkashem · 2023-10-09T19:15:23Z

/ok-to-test

tkashem · 2023-10-10T17:53:30Z

/ok-to-test

tkashem · 2023-10-10T18:26:08Z

/ok-to-test

… API

…trol v1

tkashem · 2023-10-30T12:21:56Z

I had to rebase on top of #118886, the only change due to the rebase was:

diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/feature.go b/staging/src/k8s.io/apiserver/pkg/server/options/feature.go
index 0f2ba362704..f484be0b570 100644
--- a/staging/src/k8s.io/apiserver/pkg/server/options/feature.go
+++ b/staging/src/k8s.io/apiserver/pkg/server/options/feature.go
@@ -79,7 +79,7 @@ func (o *FeatureOptions) ApplyTo(c *server.Config, clientset kubernetes.Interfac
                }
                c.FlowControl = utilflowcontrol.New(
                        informers,
-                       clientset.FlowcontrolV1beta3(),
+                       clientset.FlowcontrolV1(),
                        c.MaxRequestsInFlight+c.MaxMutatingRequestsInFlight,
                )
        }

tkashem · 2023-10-30T13:12:44Z

pkg/controlplane/apiserver/options/validation_test.go

@@ -86,8 +98,16 @@ func TestValidateAPIPriorityAndFairness(t *testing.T) {
 			if errs := validateAPIPriorityAndFairness(options); len(errs) > 0 {
 				errMessageGot = errs[0].Error()
 			}
-			if !strings.Contains(errMessageGot, test.errShouldContain) {
-				t.Errorf("Expected error message to contain: %q, but got: %q", test.errShouldContain, errMessageGot)


this test was always broken, strings.Contains(errMessageGot, "") is always true, it's fixed below.

MikeSpreitzer · 2023-10-30T19:12:20Z

pkg/apis/flowcontrol/types.go

@@ -419,6 +419,10 @@ type LimitedPriorityLevelConfiguration struct {
 	// Bigger numbers mean a larger nominal concurrency limit,
 	// at the expense of every other priority level.
 	// This field has a default value of 30.
+	//
+	// Setting this field to zero allows for the construction of a


can we say "supports" rather than "allows"? I think "allows" suggests that this is all that is needed to make that jail, and this is not true.

tkashem · 2023-10-30T20:48:02Z

pkg/registry/flowcontrol/prioritylevelconfiguration/strategy_test.go

+			errExpected:        nil,
+		},
+		{
+			name:               "v1beta3, feature enabled, update, zero value, existing has non-zero, error expected",


typo: it should be "no error expected"

liggitt · 2023-10-30T21:11:47Z

pkg/apis/flowcontrol/validation/validation.go

@@ -340,10 +348,11 @@ func ValidateFlowSchemaCondition(condition *flowcontrol.FlowSchemaCondition, fld
 }

 // ValidatePriorityLevelConfiguration validates priority-level-configuration.
-func ValidatePriorityLevelConfiguration(pl *flowcontrol.PriorityLevelConfiguration, requestGV schema.GroupVersion) field.ErrorList {
+func ValidatePriorityLevelConfiguration(pl *flowcontrol.PriorityLevelConfiguration, requestGV schema.GroupVersion, opts PriorityLevelValidationOptions) field.ErrorList {


We want to make sure the annotation used to indicate a zero value that should not be defaulted in v1beta3 never makes it through conversion to any other versions. To do that, check for and forbid the annotation here.

liggitt · 2023-10-30T21:15:22Z

staging/src/k8s.io/api/flowcontrol/v1beta3/types.go

+	// set the annotation to an empty string:
+	//   "flowcontrol.k8s.io/concurrency-shares-defaults-to-zero": ""
+	//
+	PriorityLevelConcurrencyShareDefaultKey = "flowcontrol.k8s.io/concurrency-shares-defaults-to-zero"


This annotation is specific to v1beta3... we no longer serve or store in v1beta1 or v1beta2, so we don't have to worry about round-tripping zero values to those, and we don't want this annotation to be used in v1 (see comment about forbidding it in validation after we've converted to the internal version). I'd suggest putting v1beta3 in the annotation key and documenting it is only for use in v1beta3.

Also, "defaults-to-zero" isn't quite right, when this annotation is present we don't default zero.

I'd suggest flowcontrol.k8s.io/v1beta3-preserve-zero-concurrency-shares

liggitt · 2023-10-30T21:24:08Z

API bits are really close. One comment on the name of the conversion annotation, and making sure the annotation doesn't escape conversion in validation.

As a fast follow to this PR (e.g. ~tomorrow), an e2e test for each type that walks through exercising discovery and CRUD of every verb / endpoint for both APIs is needed. All GA APIs are expected to be exercised by conformance tests by default unless there is a compelling reason they should not be included in conformance. https://apisnoop.cncf.io/conformance-progress shows the progress of adding conformance tests for existing GA APIs. An example that runs through verbs / operations for PV / PVC is at https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/persistent_volumes.go#L417-L655

Getting that written / merged ~tomorrow would let it soak and get promoted to conformance by test freeze November 15th.

tkashem · 2023-10-30T22:29:35Z

API bits are really close. One comment on the name of the conversion annotation, and making sure the annotation doesn't escape conversion in validation.

The changes are in this commit - b8cd792

tkashem · 2023-10-30T23:32:11Z

2/7426 Tests Failed. | expand_less
-- | --
Kubernetes e2e suite: [It] [sig-cli] Kubectl client Simple pod should contain last line of the log [sig-cli] expand_less26s{ failed [FAILED] Expected     <string>: failed to create fsnotify watcher: too many open files to contain substring     <string>: EOF In [It] at: test/e2e/kubectl/kubectl.go:877 @ 10/30/23 23:11:12.88 } | Kubernetes e2e suite: [It] [sig-cli] Kubectl client Simple pod should contain last line of the log [sig-cli] expand_less | 26s | { failed [FAILED] Expected     <string>: failed to create fsnotify watcher: too many open files to contain substring     <string>: EOF In [It] at: test/e2e/kubectl/kubectl.go:877 @ 10/30/23 23:11:12.88 }
Kubernetes e2e suite: [It] [sig-cli] Kubectl client Simple pod should contain last line of the log [sig-cli] expand_less | 26s
{ failed [FAILED] Expected     <string>: failed to create fsnotify watcher: too many open files to contain substring     <string>: EOF In [It] at: test/e2e/kubectl/kubectl.go:877 @ 10/30/23 23:11:12.88 }

/test pull-kubernetes-e2e-gce

liggitt · 2023-10-31T00:01:14Z

/lgtm
/approve

k8s-ci-robot · 2023-10-31T00:01:21Z

LGTM label has been added.

Git tree hash: 0e03c5ccb9f8b7b1555618569059ab31d2309e2b

k8s-ci-robot · 2023-10-31T00:01:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tkashem

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:

~~api/OWNERS~~ [liggitt]
~~cmd/kube-apiserver/OWNERS~~ [liggitt]
~~hack/OWNERS~~ [liggitt]
~~pkg/api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/controlplane/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~pkg/printers/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt]
~~test/e2e/apimachinery/OWNERS~~ [liggitt]
~~test/integration/apiserver/OWNERS~~ [liggitt]
~~test/integration/etcd/OWNERS~~ [liggitt]
~~vendor/OWNERS~~ [liggitt]

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

MikeSpreitzer · 2023-10-31T00:20:49Z

Thanks, @tkashem and @liggitt !

sftim · 2023-11-18T18:12:32Z

Changelog suggestion

-The flowcontrol.apiserver.k8s.io/v1beta3 FlowSchema and PriorityLevelConfiguration APIs has been promoted to flowcontrol.apiserver.k8s.io/v1, with the following changes:
-* PriorityLevelConfiguration: the `.spec.limited.nominalConcurrencyShares` field defaults to `30` only if the field is omitted (v1beta3 also defaulted an explicit `0` value to `30`). Specifying an explicit `0` value is not allowed in the `v1` version in v1.29 to ensure compatibility with 1.28 API servers. In v1.30, explicit `0` values will be allowed in this field in the `v1` API.
-The flowcontrol.apiserver.k8s.io/v1beta3 APIs are deprecated and will no longer be served in v1.32. All existing objects are available via the `v1` APIs. Transition clients and manifests to use the `v1` APIs before upgrading to v1.32.
+The flowcontrol.apiserver.k8s.io/v1beta3 FlowSchema and PriorityLevelConfiguration APIs have been promoted to flowcontrol.apiserver.k8s.io/v1, with the following changes:
+* PriorityLevelConfiguration: the `.spec.limited.nominalConcurrencyShares` field defaults to `30` only if the field is omitted (v1beta3 also defaulted an explicit `0` value to `30`). Specifying an explicit `0` value is not allowed in the `v1` version in v1.29 to ensure compatibility with 1.28 API servers. In v1.30, explicit `0` values will be admitted.
+
+Deprecated the flowcontrol.apiserver.k8s.io/v1beta3 API group. These deprecated APIs will no longer be served in Kubernetes v1.32. All existing API priority and fairness objects are available via the `v1` APIs. Transition clients and manifests to use the `v1` APIs before upgrading to v1.32.

How does that sound? (please see following text for context)

Do we need to support someone dumping a PriorityLevelConfiguration to YAML using flowcontrol.apiserver.k8s.io/v1beta3 and then to round-trip that object back to the API server? If we do, v1.30 would need to allow explicit 0 values for .spec.limited.nominalConcurrencyShares even if the beta API is used.
I've assumed a Yes and the diff above reflects that.

k8s-ci-robot requested review from dchen1107 and mwielgus October 9, 2023 18:25

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 9, 2023

tkashem changed the title ~~Apf v1~~ Promote APF API to v1 Oct 9, 2023

tkashem changed the title ~~Promote APF API to v1~~ [WIP] Promote APF API to v1 Oct 9, 2023

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 9, 2023

tkashem force-pushed the apf-v1 branch 2 times, most recently from c787a3d to 61e067b Compare October 10, 2023 15:54

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. area/code-generation labels Oct 10, 2023

tkashem force-pushed the apf-v1 branch from e79c018 to 5028640 Compare October 10, 2023 18:15

tkashem mentioned this pull request Oct 10, 2023

APF API: change storage version to v1 in 1.30 #121119

Closed

tkashem force-pushed the apf-v1 branch from 5028640 to eac7098 Compare October 10, 2023 18:25

tkashem force-pushed the apf-v1 branch from eac7098 to 05ec706 Compare October 11, 2023 13:29

tkashem added 8 commits October 30, 2023 08:18

apiserver: apf controller, bootstrap, tests should use flowcontrol v1…

17bda3c

… API

apiserver: add pre-lifecycle replacement for flowcontrol v1beta3

90c091d

apiserver: fix lint issue, defaulting and validation test for flowcon…

430c226

…trol v1

apiserver: allow zero value for the 'nominalConcurrencyShares' field

9fd2ab4

apiserver: add generated files for flowcontrol api change

730d10a

apiserver: add testdata for flowcontrol v1

25447b9

apiserver: remove prerelease annotation for flowcontrol v1

acd6089

apiserver: fix apf enablement with runtime-config

233bc24

tkashem force-pushed the apf-v1 branch from 3459ef5 to 233bc24 Compare October 30, 2023 12:20

tkashem commented Oct 30, 2023

View reviewed changes

MikeSpreitzer reviewed Oct 30, 2023

View reviewed changes

tkashem commented Oct 30, 2023

View reviewed changes

liggitt reviewed Oct 30, 2023

View reviewed changes

rename the roundtrip annotation, forbid it in v1

b8cd792

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2023

k8s-ci-robot merged commit f5a5d83 into kubernetes:master Oct 31, 2023
17 checks passed

tkashem mentioned this pull request Nov 5, 2023

Priority and Fairness for API Server Requests kubernetes/enhancements#1040

Closed

4 tasks

tkashem mentioned this pull request Nov 15, 2023

Update APF documentation for GA kubernetes/website#43946

Merged

pacoxu mentioned this pull request Dec 25, 2023

flowcontrol: set the serialization version to v1 #122472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote APF API to v1 #121089

Promote APF API to v1 #121089

tkashem commented Oct 9, 2023 •

edited by liggitt

k8s-ci-robot commented Oct 9, 2023

tkashem commented Oct 9, 2023

tkashem commented Oct 10, 2023

tkashem commented Oct 10, 2023

tkashem commented Oct 30, 2023

tkashem Oct 30, 2023

MikeSpreitzer Oct 30, 2023

tkashem Oct 30, 2023

liggitt Oct 30, 2023

liggitt Oct 30, 2023

liggitt commented Oct 30, 2023 •

edited

tkashem commented Oct 30, 2023

tkashem commented Oct 30, 2023

liggitt commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

MikeSpreitzer commented Oct 31, 2023

sftim commented Nov 18, 2023

Promote APF API to v1 #121089

Promote APF API to v1 #121089

Conversation

tkashem commented Oct 9, 2023 • edited by liggitt

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 9, 2023

tkashem commented Oct 9, 2023

tkashem commented Oct 10, 2023

tkashem commented Oct 10, 2023

tkashem commented Oct 30, 2023

tkashem Oct 30, 2023

Choose a reason for hiding this comment

MikeSpreitzer Oct 30, 2023

Choose a reason for hiding this comment

tkashem Oct 30, 2023

Choose a reason for hiding this comment

liggitt Oct 30, 2023

Choose a reason for hiding this comment

liggitt Oct 30, 2023

Choose a reason for hiding this comment

liggitt commented Oct 30, 2023 • edited

tkashem commented Oct 30, 2023

tkashem commented Oct 30, 2023

liggitt commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

MikeSpreitzer commented Oct 31, 2023

sftim commented Nov 18, 2023

tkashem commented Oct 9, 2023 •

edited by liggitt

liggitt commented Oct 30, 2023 •

edited