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

webhook: support elasticquota enable update resource key #2323

Conversation

lijunxin559
Copy link
Contributor

@lijunxin559 lijunxin559 commented Jan 17, 2025

Ⅰ. Describe what this PR does

Currently, the consistency check for resource types in elastic quota's add, delete and update is insufficient and too rigid. For example, maxResourceKey requires parent-child consistency, which can lead to the following problems:

  1. There is no unified logic to check the various resource types of min/max, which can easily lead to inconsistencies
  2. Unable to flexibly support changing resource types, especially when adding new resources, limited by the initial creation status

However, considering the original intention of quota design and current computing logic, it does not affect related calculations while supporting resource type checks(also already added more unitTest cases). Among them:

  1. The setting intentions of min and max are different and the calculation logic is not dependent. The resource types need to be checked separately
  2. Guaranteed is affected by min calculation, so there is no need to check separately

Ⅱ. Does this pull request fix one issue?

I propose to add a feature called ElasticQuotaEnableUpdateResourceKey to support changes to ElasticQuota Resource Key, with the following modifications:

  1. Support min and max resourceKey checks during elasticQuota add and update: checkSubAndParentGroupQuotaKey()
  2. Support AnnotationSharedWeight updated: when SharedWeight has a key which is not included in max, delete it; when SharedWeight does not have the key of max, it defaults to be filled in the quantity of max
  3. Maintain compatibility of resourceKeys update logic: updateResourceKeyNoLock()
  4. Propose standard process operations for elastic quota resourceKey: add resourceKey from parent node to child node, delete resourceKey from child node to parent node

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 86.58537% with 11 lines in your changes missing coverage. Please review.

Project coverage is 66.05%. Comparing base (6f6ef82) to head (222da91).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
pkg/webhook/elasticquota/quota_topology.go 79.31% 4 Missing and 2 partials ⚠️
pkg/webhook/elasticquota/quota_topology_check.go 88.09% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2323      +/-   ##
==========================================
- Coverage   66.09%   66.05%   -0.05%     
==========================================
  Files         458      461       +3     
  Lines       54197    54610     +413     
==========================================
+ Hits        35823    36071     +248     
- Misses      15801    15928     +127     
- Partials     2573     2611      +38     
Flag Coverage Δ
unittests 66.05% <86.58%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lijunxin559 lijunxin559 force-pushed the support-elasticquota-enable-update-resourceKey branch from 01bf3f6 to 081511d Compare January 21, 2025 09:38
@saintube saintube requested a review from shaloulcy January 22, 2025 01:44
@shaloulcy
Copy link
Contributor

group_quota_manager.go里面的UpdateQuota、RefreshRuntime 需要加单侧覆盖这种情况

@lijunxin559 lijunxin559 force-pushed the support-elasticquota-enable-update-resourceKey branch from 081511d to 3a72f52 Compare January 23, 2025 06:42
@lijunxin559
Copy link
Contributor Author

group_quota_manager.go里面的UpdateQuota、RefreshRuntime 需要加单侧覆盖这种情况

added

@lijunxin559 lijunxin559 force-pushed the support-elasticquota-enable-update-resourceKey branch from 3a72f52 to f44afa6 Compare January 23, 2025 09:06
@lijunxin559 lijunxin559 force-pushed the support-elasticquota-enable-update-resourceKey branch from f44afa6 to 83ca151 Compare February 6, 2025 07:57
Signed-off-by: lijunxin <lijunxin.ljx@alibaba-inc.com>
@lijunxin559 lijunxin559 force-pushed the support-elasticquota-enable-update-resourceKey branch from 83ca151 to 222da91 Compare February 10, 2025 06:26
@shaloulcy
Copy link
Contributor

/lgtm

@saintube saintube added the lgtm label Feb 13, 2025
Copy link
Member

@saintube saintube left a comment

Choose a reason for hiding this comment

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

Good job! @lijunxin559

@koordinator-bot koordinator-bot bot merged commit a12423b into koordinator-sh:main Feb 14, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants