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

quota controller fixes #74747

Merged
merged 9 commits into from Mar 27, 2019

Conversation

@liggitt
Copy link
Member

commented Feb 28, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:

Fixes a panoply of quota issues that are relevant now that we intend to quota based on discovery

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #71450

  • Improves quota logging output around waiting for informers to resync
  • Fixes an existing race condition in the quota test
  • Generates a missing networkpolicies client so that all in-tree types actually have informers
  • Updates quota status with hard limits even in the face of calculation errors (otherwise, a single calculation error would prevent quota status from ever reflecting limits, and ever enforcing anything in admission)
  • Updates quota status with resources that could be calculated in the face of partial calculation errors (otherwise a calculation error on a single resource could prevent usage info for other resources from ever being populated, and block admission for those resources)
  • Avoids rejecting admission based on missing usage info when the missing info isn't relevant (otherwise a single missing resource usage could block all resources addressed by that quota)
  • Avoids deadlocks in quota resync
  • Avoids letting quota evaluators use unsynced informers

Does this PR introduce a user-facing change?:

* Fixed a potential deadlock in resource quota controller
* Enabled recording partial usage info for quota objects specifying multiple resources, when only some of the resources' usage can be determined.
@roycaihw

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

/cc @yliaog

@k8s-ci-robot k8s-ci-robot requested a review from yliaog Feb 28, 2019

@liggitt liggitt added this to the v1.15 milestone Mar 7, 2019

@liggitt liggitt force-pushed the liggitt:quota-deadlock branch 2 times, most recently from 13e89f2 to 66dd91c Mar 14, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 26, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Mar 26, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 26, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@tnozicka

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

/hold
(just until you fix the verify job, so it doesn't spam with retests)

@liggitt liggitt force-pushed the liggitt:quota-deadlock branch from f7d46ff to b46b938 Mar 27, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 27, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

fixed bazel error, should be all set now

@liggitt liggitt force-pushed the liggitt:quota-deadlock branch from b46b938 to bef996d Mar 27, 2019

@yliaog

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton, yliaog

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

@liggitt

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit a8cbb22 into kubernetes:master Mar 27, 2019

17 checks passed

cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@liggitt liggitt deleted the liggitt:quota-deadlock branch Mar 27, 2019

result := []corev1.ResourceName{}
for _, resourceName := range setC.List() {
result = append(result, corev1.ResourceName(resourceName))
result := make([]corev1.ResourceName, 0, len(a))

This comment has been minimized.

Copy link
@tedyu

tedyu Mar 28, 2019

Contributor

Current code has complexity O(M*N)

Can we do this:

setA := ToSet(a)
for _, item := range b {
  if (setA.Has(item)) {
    result = append(result, item)
  }
}

The complexity is O(M+N).

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 28, 2019

Author Member

I'm not opposed but would like to know the size at which the difference becomes meaningful. Avoiding allocations is useful and iterating over small lists is not problematic.

This comment has been minimized.

Copy link
@tedyu

tedyu Mar 28, 2019

Contributor

This is my understanding:
without dynamic code (which switches algorithm according to input sizes), we should be prepared for large input sizes where performance becomes first concern (the additional memory is linear in input size).

If the input sizes are small, either approach is acceptable.

So I would opt for the version I proposed above.

This comment has been minimized.

Copy link
@tedyu

tedyu Mar 28, 2019

Contributor

We can change the return type of ResourceNames to map[corev1.ResourceName]struct{}.
This way, by the time Intersection is called, it has two Sets available - facilitating the above algorithms.
@liggitt
What do you think ?

// Difference returns the list of resources resulting from a-b, deduped and sorted
func Difference(a []corev1.ResourceName, b []corev1.ResourceName) []corev1.ResourceName {
result := make([]corev1.ResourceName, 0, len(a))
for _, item := range a {

This comment has been minimized.

Copy link
@tedyu

tedyu Mar 28, 2019

Contributor

Can we do this:

setA := ToSet(a)
for _, item := range b {
  if (!setA.Has(item)) {
    result = append(result, item)
  }
}

Similar operation for finding elements from a which are not in b

The complexity is O(M+N)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.