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
apf: introduce v1beta2 #104399
apf: introduce v1beta2 #104399
Conversation
5871b48
to
ba117f4
Compare
/triage accepted |
ccf527d
to
6c15d40
Compare
For some reason, Also, I will open a KEP PR for the new type. |
/assign @MikeSpreitzer @lavalamp @liggitt @deads2k |
test/integration/etcd/data.go
Outdated
}, | ||
// -- | ||
|
||
// 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"), |
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.
drop this line, the test doesn't expect an override since the storage GVK is still v1beta1
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.
done
test/integration/etcd/data.go
Outdated
@@ -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"), |
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.
drop this line as well
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.
done
/retest |
db868b4
to
8fc600e
Compare
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}}}`, |
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 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.
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.
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()
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.
thanks @liggitt, I added the required GetResetFields
fields for v1beta2
and now the test is passing.
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.
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.
@@ -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)) |
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.
Do we have a reason that we can drop the v1alpha1?
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.
Looks good to me.
I do not understand what is going wrong with the reset fields test.
@MikeSpreitzer |
/lgtm |
[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:
Approvers can indicate their approval by writing |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: