apf: introduce v1beta2 #104399

tkashem · 2021-08-16T21:15:27Z

What type of PR is this?

/kind api-change

What this PR does / why we need it:

introduce v1beta2 - no changes in the API.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Introduce v1beta2 for Priority and Fairness with no changes in API spec

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

fedebongio · 2021-08-17T20:15:23Z

/triage accepted

tkashem · 2021-08-17T21:35:49Z

--- FAIL: TestStorageVersionHashes (0.87s)
    instance_test.go:380: expect the storageVersionHash of flowcontrol.apiserver.k8s.io/v1beta1/flowschemas to be "9bSnTLYweJ0=", got "G+8IkrqFuJw="
    instance_test.go:380: expect the storageVersionHash of flowcontrol.apiserver.k8s.io/v1beta1/prioritylevelconfigurations to be "BFVwf8eYnsw=", got "wltM4WMeeXs="

For some reason, storageVersionHash for v1beta1 has been changed. I am trying to figure that out.

Also, I will open a KEP PR for the new type.

tkashem · 2021-08-17T21:36:13Z

/assign @MikeSpreitzer @lavalamp @liggitt @deads2k

test/integration/etcd/data.go

liggitt · 2021-08-24T01:44:44Z

test/integration/etcd/data.go

 		},
 		// --

 		// k8s.io/kubernetes/pkg/apis/flowcontrol/v1beta1
 		gvr("flowcontrol.apiserver.k8s.io", "v1beta1", "prioritylevelconfigurations"): {
 			Stub:             `{"metadata": {"name": "conf2"}, "spec": {"type": "Limited", "limited": {"assuredConcurrencyShares":3, "limitResponse": {"type": "Reject"}}}}`,
 			ExpectedEtcdPath: "/registry/prioritylevelconfigurations/conf2",
+			ExpectedGVK:      gvkP("flowcontrol.apiserver.k8s.io", "v1beta1", "PriorityLevelConfiguration"),


drop this line, the test doesn't expect an override since the storage GVK is still v1beta1

liggitt · 2021-08-24T01:44:51Z

test/integration/etcd/data.go

@@ -273,13 +273,29 @@ func GetEtcdStorageDataForNamespace(namespace string) map[schema.GroupVersionRes
 		gvr("flowcontrol.apiserver.k8s.io", "v1beta1", "flowschemas"): {
 			Stub:             `{"metadata": {"name": "va2"}, "spec": {"priorityLevelConfiguration": {"name": "name1"}}}`,
 			ExpectedEtcdPath: "/registry/flowschemas/va2",
+			ExpectedGVK:      gvkP("flowcontrol.apiserver.k8s.io", "v1beta1", "FlowSchema"),


drop this line as well

MikeSpreitzer · 2021-08-27T05:45:15Z

/retest

tkashem · 2021-09-08T01:16:24Z

test/integration/apiserver/apply/reset_fields_test.go

 	gvr("flowcontrol.apiserver.k8s.io", "v1alpha1", "prioritylevelconfigurations"): `{"metadata": {"labels":{"a":"c"}}, "spec": {"limited": {"assuredConcurrencyShares": 23}}}`,
 	gvr("flowcontrol.apiserver.k8s.io", "v1beta1", "prioritylevelconfigurations"):  `{"metadata": {"labels":{"a":"c"}}, "spec": {"limited": {"assuredConcurrencyShares": 23}}}`,
+	gvr("flowcontrol.apiserver.k8s.io", "v1beta2", "prioritylevelconfigurations"):  `{"metadata": {"labels":{"a":"c"}}, "spec": {"limited": {"assuredConcurrencyShares": 23}}}`,


This PR introduces v1beta2 for priority & fairness - v1beta2 is a straight copy from v1beta1 - no changes in the API spec.

TestApplyResetFields (integration test) is failing for v1beta2 of priorityLevelConfiguration with the following error

reset_fields_test.go:265: Failed to apply obj2: Apply failed with 4 conflicts: conflicts with "fieldmanager1": - .spec.limited.assuredConcurrencyShares - .status.conditions[type="MyStatus"] - .status.conditions[type="MyStatus"].type - .status.conditions[type="MyStatus"].type

but the same test for v1beta1 is passing.

@apelisse I am not familiar with the inner working of server side apply, i would appreciate it if you can provide any pointer for me to troubleshoot this issue further.

Very glad to see the integration test caught the gap. @apelisse, it would be good to document what people should do here and point them to the docs from the test failure

Looks like v1beta2 cases are needed in these methods:

flowSchemaStrategy#GetResetFields()

flowSchemaStatusStrategy#GetResetFields()

priorityLevelConfigurationStrategy#GetResetFields()

priorityLevelConfigurationStatusStrategy#GetResetFields()

thanks @liggitt, I added the required GetResetFields fields for v1beta2 and now the test is passing.

also, opened a new issue #104842 [ TestApplyResetFields test skips flowschemas resource due to flake].

But I ran it locally to make sure reset fields for flowschemas v1beta2 work as expected.

MikeSpreitzer · 2021-09-08T21:22:58Z

pkg/apis/flowcontrol/install/install.go

@@ -36,5 +37,6 @@ func Install(scheme *runtime.Scheme) {
 	utilruntime.Must(flowcontrol.AddToScheme(scheme))
 	utilruntime.Must(flowcontrolv1alpha1.AddToScheme(scheme))
 	utilruntime.Must(flowcontrolv1beta1.AddToScheme(scheme))
-	utilruntime.Must(scheme.SetVersionPriority(flowcontrolv1beta1.SchemeGroupVersion, flowcontrolv1alpha1.SchemeGroupVersion))
+	utilruntime.Must(flowcontrolv1beta2.AddToScheme(scheme))
+	utilruntime.Must(scheme.SetVersionPriority(flowcontrolv1beta1.SchemeGroupVersion, flowcontrolv1beta2.SchemeGroupVersion))


Do we have a reason that we can drop the v1alpha1?

MikeSpreitzer

Looks good to me.
I do not understand what is going wrong with the reset fields test.

tkashem · 2021-09-13T16:38:02Z

I do not understand what is going wrong with the reset fields test.

@MikeSpreitzer GetResetFields was missing v1beta2, after adding the case it is all green now. But then I discovered a separate issue - #104842 which we will need to address in a separate PR.

lavalamp · 2021-09-13T23:25:13Z

/lgtm
/approve

k8s-ci-robot · 2021-09-13T23:28:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, MikeSpreitzer, 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~~ [lavalamp]
~~cmd/kube-apiserver/OWNERS~~ [lavalamp]
~~hack/OWNERS~~ [lavalamp]
~~pkg/apis/OWNERS~~ [lavalamp]
~~pkg/controlplane/OWNERS~~ [lavalamp]
~~pkg/printers/OWNERS~~ [lavalamp]
~~pkg/registry/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/pkg/apis/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]
~~test/OWNERS~~ [lavalamp]
~~vendor/OWNERS~~ [lavalamp]

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 requested review from derekwaynecarr and jennybuckley August 16, 2021 21:15

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

tkashem force-pushed the apf-v1beta2 branch 3 times, most recently from 5871b48 to ba117f4 Compare August 17, 2021 19:31

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 17, 2021

tkashem force-pushed the apf-v1beta2 branch 3 times, most recently from ccf527d to 6c15d40 Compare August 17, 2021 21:27

tkashem changed the title ~~[WIP] [DO NOT REVIEW] apf: enable v1beta2~~ [WIP] apf: enable v1beta2 Aug 17, 2021

k8s-ci-robot assigned deads2k, lavalamp and liggitt Aug 17, 2021

liggitt reviewed Aug 24, 2021

View reviewed changes

test/integration/etcd/data.go Show resolved Hide resolved

liggitt reviewed Aug 24, 2021

View reviewed changes

tkashem force-pushed the apf-v1beta2 branch from 069cb9a to 8d15699 Compare August 24, 2021 15:52

tkashem changed the title ~~[WIP] apf: introduce v1beta2~~ apf: introduce v1beta2 Aug 24, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2021

tkashem force-pushed the apf-v1beta2 branch from 8d15699 to 9eb2534 Compare August 26, 2021 21:47

tkashem added 2 commits September 7, 2021 09:42

apf: copy v1beta1 to v1beta2

54064ff

apf: rename to v1beta2

5d0bf05

tkashem force-pushed the apf-v1beta2 branch 3 times, most recently from db868b4 to 8fc600e Compare September 7, 2021 22:18

tkashem commented Sep 8, 2021

View reviewed changes

tkashem force-pushed the apf-v1beta2 branch from 8fc600e to a91a1a9 Compare September 8, 2021 13:01

MikeSpreitzer reviewed Sep 8, 2021

View reviewed changes

tkashem added 4 commits September 9, 2021 08:28

apf: ebable v1beta2

e2b8701

apf: regenerate for v1beta2

2f19f8c

apf: update e2e test to use v1beta2

d99497e

apf: update apf logic to use v1beta2

28f2b42

tkashem force-pushed the apf-v1beta2 branch from a91a1a9 to 28f2b42 Compare September 9, 2021 12:29

MikeSpreitzer approved these changes Sep 13, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2021

k8s-ci-robot merged commit cdcf2a2 into kubernetes:master Sep 14, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 14, 2021

liggitt moved this from Assigned to API review completed, 1.23 in API Reviews Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apf: introduce v1beta2 #104399

apf: introduce v1beta2 #104399

tkashem commented Aug 16, 2021 •

edited

fedebongio commented Aug 17, 2021

tkashem commented Aug 17, 2021

tkashem commented Aug 17, 2021

liggitt Aug 24, 2021

tkashem Aug 24, 2021

liggitt Aug 24, 2021

tkashem Aug 24, 2021

MikeSpreitzer commented Aug 27, 2021

tkashem Sep 8, 2021 •

edited

liggitt Sep 8, 2021

tkashem Sep 8, 2021

tkashem Sep 8, 2021

MikeSpreitzer Sep 8, 2021

MikeSpreitzer left a comment

tkashem commented Sep 13, 2021 •

edited

lavalamp commented Sep 13, 2021

k8s-ci-robot commented Sep 13, 2021

apf: introduce v1beta2 #104399

apf: introduce v1beta2 #104399

Conversation

tkashem commented Aug 16, 2021 • edited

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.:

fedebongio commented Aug 17, 2021

tkashem commented Aug 17, 2021

tkashem commented Aug 17, 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

MikeSpreitzer commented Aug 27, 2021

tkashem Sep 8, 2021 • edited

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

MikeSpreitzer left a comment

Choose a reason for hiding this comment

tkashem commented Sep 13, 2021 • edited

lavalamp commented Sep 13, 2021

k8s-ci-robot commented Sep 13, 2021

tkashem commented Aug 16, 2021 •

edited

tkashem Sep 8, 2021 •

edited

tkashem commented Sep 13, 2021 •

edited