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 ResourceQuota admission plugin #2627

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

timuthy
Copy link
Contributor

@timuthy timuthy commented Jul 24, 2020

How to categorize this PR?

/area robustness
/kind enhancement
/priority normal

What this PR does / why we need it:
This PR adds the ResourceQuota admission plugin to the Gardener API server. For the beginning, it supports a simple "object count" quota for API groups that are handled by the Gardener API server (e.g. shoots, seeds, secretbindings, etc.).

Example quota:

apiVersion: v1
kind: ResourceQuota
metadata:
  name: object-quota-demo
  namespace: garden-dev
spec:
  hard:
    count/shoots.core.gardener.cloud: "2"

Please make sure the Kube-Controller-Manager of your Gardener cluster runs with the resourcequota controller. It is responsible for decrementing the count in case of a deletion.

Which issue(s) this PR fixes:
Fixes partly #1650

Special notes for your reviewer:

  • The recommended order of validating admission plugins is ensure by the admission chain. Thus, the ResourceQuota admission plugin was added as the last member of the list to prevent premature status updates to quota resources when other admission plugins haven't taken their decision yet.
  • To reuse the very same admission plugin from the Kube-Apiserver,
    this PR introduces a dependency to k8s.io/kubernetes. I kindly ask for your opinion if and why you see any perils of having this dependency (esp. /cc @mvladev @ialidzhikov @rfranzke).
    this PR copies the necessary logic from k8s.io/kubernetes to /third_party/fork/kubernetes instead of defining a general dependency to k8s.io/kubernetes which turned out to be problematic due to maintainability reasons (thanks @mvladev @rfranzke). This copy can be removed as soon as we vendor the sources to Kubernetes v1.20.x because of Move ResourceQuota admission to k8s.io/apiserver lib kubernetes/kubernetes#93537.
  • Another PR will follow with the ability to define a project-wide default quota and documentation.

Release note:

The Gardener API server now supports the usage of [`ResourceQuota`](https://kubernetes.io/docs/concepts/policy/resource-quotas/)s for Gardener API groups and resources like `Shoot`s, `Seed`s, `SecretBinding`s, etc.. At the moment the quota supports object counts only, e.g. `count/shoots.core.gardener.cloud`.
⚠️ Please make sure the kube-controller-manager in your Garden cluster runs with `--controllers=...,resourcequota,...` if you want to use them.

@gardener-robot gardener-robot added area/robustness Robustness, reliability, resilience related kind/enhancement Enhancement, improvement, extension priority/normal labels Jul 24, 2020
@rfranzke
Copy link
Member

/invite @mvladev

@mvladev
Copy link

mvladev commented Jul 27, 2020

To reuse the very same admission plugin from the Kube-Apiserver, this PR introduces a dependency to k8s.io/kubernetes. I kindly ask for your opinion if and why you see any perils of having this dependency.

I still recall the time when @rfranzke wanted to update to the latest K8S version only to give up after spending an entire day trying. I managed to do it in only 5 hours. I would like to avoid kubernetes/kubernetes as much as possible.

I would rather have a discussion with sig-api-machinery into possible workarounds of that issue as it could benefit aggregated apiservers at all (fixing it in upstream k8s):

  • have ResourceQuota admission plugin apply before sending the request to the aggregated apiserver.
  • extracting the plugin in a separate repo to limit dependencies.

@mvladev
Copy link

mvladev commented Jul 27, 2020

Another thing which would help us reduce the blast radius of having kubernetes/kubernetes dependency is to have a separate golang modules for each component - gardener-apiserver, gardener-controller-manager etc...

@timuthy
Copy link
Contributor Author

timuthy commented Jul 29, 2020

Thanks for your input @mvladev. Moving the resource quota capabilities to k8s.io/apiserver sounds reasonable to me because it follows the webhook approach. I'm currently working on the move out https://github.com/timuthy/kubernetes/tree/enhancement.move-resourcequota and will file a PR. Let's wait then for the community feedback and decide how we continue here.

@mvladev
Copy link

mvladev commented Jul 29, 2020

I'm currently working on the move out https://github.com/timuthy/kubernetes/tree/enhancement.move-resourcequota and will file a PR. Let's wait then for the community feedback and decide how we continue here.

I just had a quick view and I don't thing that moving that would be a viable approach - the apiserver library is a generic API server and it doesn't expose any of the K8S core/v1 APIs. I was thinking if it would be possible to do it for kube-apiserver as it's the one which exposes core/v1 APIs.

@timuthy
Copy link
Contributor Author

timuthy commented Jul 30, 2020

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2020

CLA assistant check
All committers have signed the CLA.

@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 23, 2020
@timuthy
Copy link
Contributor Author

timuthy commented Oct 23, 2020

Update: I've removed the general dependency to "k8s.io/kubernetes" and instead copied the necessary logic for the quota admission plugin to third_party/fork/kubernetes (please look at README.md for more info). This copy can be removed as soon as we vendor the sources to Kubernetes v1.20.x because of kubernetes/kubernetes#93537.

@timuthy timuthy marked this pull request as ready for review October 23, 2020 08:41
@timuthy timuthy requested a review from a team as a code owner October 23, 2020 08:41
@timuthy
Copy link
Contributor Author

timuthy commented Oct 23, 2020

I separated the steps (adding the admission plugin + copying K8s sources) into two commits to make reviewing easier.

rfranzke
rfranzke previously approved these changes Oct 23, 2020
Copy link
Member

@rfranzke rfranzke 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 for all your great work here, well done!

@rfranzke
Copy link
Member

/second-opinion

@timuthy
Copy link
Contributor Author

timuthy commented Oct 26, 2020

PR update after discussing with @timebertt:

  1. Added ref to Kube-Apiserver in doc string in order to explain why an extra plugin initializer is necessary.
  2. Removed registration delegation and instead use registration directly.

Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Thanks, @timuthy!
No further comments from my side. Tested it locally, worked perfectly fine :)
/lgtm

@rfranzke rfranzke merged commit 6919485 into gardener:master Oct 26, 2020
@timebertt
Copy link
Member

@timuthy @rfranzke
Something is broken here, when revendoring g/g@master in g/gcp:

$ make revendor
go: github.com/gardener/gardener@v1.11.1 requires
	k8s.io/kubernetes@v0.18.8: reading k8s.io/kubernetes/go.mod at revision v0.18.8: unknown revision v0.18.8
make: *** [Makefile:100: revendor] Error 1

When I set the k/k dep in go.mod to v1.18.8 it gives me:

$ make revendor
go: github.com/gardener/gardener@v1.11.1 requires
	k8s.io/kubernetes@v1.18.8 requires
	k8s.io/cli-runtime@v0.0.0: reading k8s.io/cli-runtime/go.mod at revision v0.0.0: unknown revision v0.0.0
make: *** [Makefile:100: revendor] Error 1

@timuthy
Copy link
Contributor Author

timuthy commented Oct 27, 2020

Sorry about that @timebertt.

The replace directive is only respected for the main module, which is why it causes trouble with transitive dependencies. Let me file another PR which removes the additional module.

@timebertt
Copy link
Member

Thanks for taking care! 😄

@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
@timuthy timuthy deleted the feature/resourcequota branch December 23, 2022 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/robustness Robustness, reliability, resilience related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants