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.

Sign up for GitHub

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

Jump to bottom

Machine resource quota webhook #9650

Merged
merged 9 commits into from Apr 26, 2022
Merged

Machine resource quota webhook #9650

merged 9 commits into from Apr 26, 2022

Conversation

lsviben
Copy link
Contributor

@lsviben lsviben commented Apr 20, 2022

What does this PR do / Why do we need it:
Part of #9412. This PR adds a machine validating webhook for resource quotas.

As we dont have yet the ResourceQuotas for projects implemented, or its calculation, so for now getting the quotas are hardcoded with some fake value. Also getting the resource requirements for the machine, per provider, will be implemented in the next PRs, so for now no providers are supported - meaning the webhook wont do anything yet and will just pass the validation.

What this PR brings is the scaffolding of the resource quota validation, with the checks for requested resources vs. quota and current quota usage.

Does this PR close any issues?:
Fixes #9625

Does this PR introduce a user-facing change?:

Added a Machine validating webhook which checks the Machine resource request(CPU, Memory, Storage) against its projects resource quota(if set). This is an EE feature only.

@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 20, 2022
@lsviben lsviben force-pushed the machine-resource-quota-webhook branch from 918eff1 to eac438b Compare April 20, 2022 10:31
@lsviben lsviben changed the title WIP Machine resource quota webhook Machine resource quota webhook Apr 20, 2022
@kubermatic-bot kubermatic-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2022
@lsviben lsviben requested a review from xrstf April 20, 2022 10:50
xrstf
xrstf reviewed Apr 21, 2022
View reviewed changes
pkg/ee/validation/machine/resourcequota.go Outdated
combinedUsage.Storage().Add(*quotaReq.Storage())

if quota.Cpu().Cmp(*combinedUsage.Cpu()) < 0 {
log.Debugf("requested CPU %q would exceed current quota (quota/used %q/%q)",
Copy link
Contributor

Choose a reason for hiding this comment

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

I love readable log messages, but should we use structured logging here? Honest question. "No." is a valid answer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to structured, looks better

xrstf
xrstf reviewed Apr 21, 2022
View reviewed changes
pkg/ee/validation/machine/resourcequota.go Outdated
NewResourceQuota(cpuUsed, memUsed, storageUsed), nil
}

type ResourceQuota struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This struct seems to be used not just for quotas, but also to express the current consumption. So the name for this struct might be a bit unfortunate. What about ResourceDetails maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about using corev1.ResourceList? That one looks super similar, maybe similar enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first thought and implementation was with corev1.ResourceList, but it has some additional things like ResourceEphemeralStorage and Pods() which I thought could make it confusing. So I decided on something simpler

xrstf
xrstf reviewed Apr 21, 2022
View reviewed changes
pkg/resources/resources.go Outdated Show resolved Hide resolved
xrstf
xrstf requested changes Apr 21, 2022
View reviewed changes
Copy link
Contributor

@xrstf xrstf left a comment

Choose a reason for hiding this comment

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

A few nits here and there.

@lsviben lsviben force-pushed the machine-resource-quota-webhook branch from eac438b to 87570be Compare April 22, 2022 08:58
@lsviben
Copy link
Contributor Author

lsviben commented Apr 22, 2022

/retest

2 similar comments
@lsviben
Copy link
Contributor Author

lsviben commented Apr 22, 2022

/retest

@lsviben
Copy link
Contributor Author

lsviben commented Apr 25, 2022

/retest

@lsviben lsviben requested a review from xrstf April 25, 2022 08:47
@lsviben
Copy link
Contributor Author

lsviben commented Apr 25, 2022

/retest

xrstf
xrstf approved these changes Apr 26, 2022
View reviewed changes
Copy link
Contributor

@xrstf xrstf left a comment

Choose a reason for hiding this comment

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

/approve

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2022
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 258e9f13f8b41d274676dd40ca59ec036fd14b92

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsviben, xrstf

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

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2022
@kubermatic-bot kubermatic-bot merged commit faa147e into kubermatic:master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xrstf xrstf xrstf approved these changes

Assignees

@xrstf xrstf

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add machine validating webhook
3 participants
@lsviben @kubermatic-bot @xrstf