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

add v1beta3 for Priority And Fairness #112306

Merged
merged 10 commits into from Oct 3, 2022
Merged

Conversation

tkashem
Copy link
Contributor

@tkashem tkashem commented Sep 7, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #107574
Fixes #104842

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Introduce v1beta3 for Priority and Fairness with the following changes to the API spec:
- rename 'assuredConcurrencyShares' (located under spec.limited') to 'nominalConcurrencyShares'
- apply strategic merge patch annotations to 'Conditions' of flowschemas and prioritylevelconfigurations

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


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 7, 2022
@tkashem tkashem changed the title [WIP] add v1beta3 for Priority And Fairness [WIP] [Do Not Review] add v1beta3 for Priority And Fairness Sep 7, 2022
@k8s-ci-robot k8s-ci-robot added area/apiserver area/code-generation area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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 Sep 7, 2022
@tkashem tkashem force-pushed the v1beta3 branch 2 times, most recently from bc33519 to d46623f Compare September 7, 2022 22:38
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 8, 2022
@tkashem tkashem force-pushed the v1beta3 branch 5 times, most recently from f8c6828 to 8ed3afd Compare September 10, 2022 11:45
@tkashem tkashem changed the title [WIP] [Do Not Review] add v1beta3 for Priority And Fairness add v1beta3 for Priority And Fairness Sep 10, 2022
@k8s-ci-robot 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 Sep 10, 2022
@tkashem tkashem force-pushed the v1beta3 branch 2 times, most recently from dbb4b58 to b8d0b47 Compare September 23, 2022 03:11
@tkashem
Copy link
Contributor Author

tkashem commented Sep 23, 2022

/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-e2e-kind
/test pull-kubernetes-conformance-kind-ipv6-parallel

@tkashem
Copy link
Contributor Author

tkashem commented Sep 23, 2022

{Failed === RUN TestCreateHealthcheck/timeouts_if_response_time_higher_than_default_timeout
factory_test.go:141: healthcheck() missmatch want context deadline exceeded got etcd client connection not yet established
--- FAIL: TestCreateHealthcheck/timeouts_if_response_time_higher_than_default_timeout (1.00s)
}

/test pull-kubernetes-unit

@tkashem
Copy link
Contributor Author

tkashem commented Sep 26, 2022

it's ready for another pass, thanks!

@liggitt liggitt moved this from Changes requested to In progress in API Reviews Sep 26, 2022
Copy link
Member

@MikeSpreitzer MikeSpreitzer left a comment

Choose a reason for hiding this comment

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

/LGTM
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2022
@wojtek-t
Copy link
Member

/cc

Copy link
Contributor

@cici37 cici37 left a comment

Choose a reason for hiding this comment

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

Couple questions:

  1. Since the patch annotation is added and the controller switched to use v1beta3, should [reset_fields_test.go](
    var resetFieldsSkippedResources = map[string]struct{}{
    // TODO: flowschemas is flaking,
    // possible bug in the flowschemas controller.
    "flowschemas": {},
    }
    ) updated now?
  2. Maybe worth to keep in mind(Could be added in a followup as well): test/integration/apiserver/flowcontrol/fs_condition_test.go
  3. If we plan to address 1, maybe mention this PR will fix flowschemas resource is currently skipped by TestApplyResetFields test. #104842
  4. Could be a followup PR: update lifecycle directives on old flowcontrol versions to point to new v1beta3 version
    e.g. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/flowcontrol/v1beta1/types.go#L108 and v1alpha1 and v1beta2 types

Thank you! :)

@liggitt liggitt moved this from In progress to API review completed, 1.26 in API Reviews Sep 29, 2022
@liggitt
Copy link
Member

liggitt commented Sep 29, 2022

thanks for the API review, marking API changes as approved

looks like dropping the exclusion from reset_fields_test.go and making sure the test doesn't flake is the only outstanding thing for this PR

@tkashem
Copy link
Contributor Author

tkashem commented Oct 3, 2022

looks like dropping the exclusion from reset_fields_test.go and making sure the test doesn't flake is the only outstanding thing for this PR

I have an open PR #112575 for that. I also tested locally.

stress test on TestApplyResetFields, on master (flowschemas is skipped) - 1000 runs and 0 failure

$ go test -c ./test/integration/apiserver/apply/
$ stress ./apply.test -test.run=TestApplyResetFields
5s: 0 runs so far, 0 failures
10s: 5 runs so far, 0 failures
...
18m50s: 1014 runs so far, 0 failures

stress test on TestApplyResetFields, on master (flowschemas is not skipped): 2 failures within first 8 runs

$ stress ./apply.test -test.run=TestApplyResetFields
5s: 0 runs so far, 0 failures
10s: 4 runs so far, 1 failures (25.00%)
15s: 8 runs so far, 2 failures (25.00%)

stress test on TestApplyResetFields, on this PR (flowschemas is not skipped): 1000 runs, no failure

$ stress ./apply.test -test.run=TestApplyResetFields
5s: 0 runs so far, 0 failures
10s: 2 runs so far, 0 failures
...
21m35s: 1101 runs so far, 0 failures
21m40s: 1104 runs so far, 0 failures

Once we merge #112575, I will keep an eye on CI just to make sure we don't see the flakes.

@liggitt
Copy link
Member

liggitt commented Oct 3, 2022

/approve

@liggitt
Copy link
Member

liggitt commented Oct 3, 2022

sounds good, can fast-follow this PR with #112575

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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 /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 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9720af2 into kubernetes:master Oct 3, 2022
@tkashem
Copy link
Contributor Author

tkashem commented Oct 3, 2022

@cici37 thanks for the review!

Since the patch annotation is added and the controller switched to use v1beta3, ) should reset_fields_test.go be updated now?

Yes, #112575

Could be a followup PR: update lifecycle directives on old flowcontrol versions to point to new v1beta3 version

#112832

@wojtek-t
Copy link
Member

wojtek-t commented Oct 4, 2022

Great to see it merged - thanks!

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. area/apiserver area/code-generation area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.26
Archived in project
9 participants