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

[YUNIKORN-1647] Dynamic namespace support in admission controller #557

Closed
wants to merge 2 commits into from

Conversation

wilfred-s
Copy link
Contributor

What is this PR for?

Support annotations on a namespace to define processing behaviour in the admission controller. The annotations have preference over the regexp. If no annotations are found fall back on the regexp. If an annotation is found it defines the behaviour and overrides what is defined in the regexp.

Annotations to be supported on a namespace:

  • yunikorn.apache.org/namespace.generateAppId
  • yunikorn.apache.org/namespace.enableYuniKorn

Supported values for the annotation: true and false.

What type of PR is it?

  • - Feature

What is the Jira issue?

How should this be tested?

Unit tests are added.

Support annotations on a namespace to define processing behaviour in the
admission controller. The annotations have preference over the regexp.
If no annotations are found fall back on the regexp. If an annotation is
found it defines the behaviour and overrides what is defined in the
regexp.

Annotations to be supported on a namespace:
* yunikorn.apache.org/namespace.generateAppId
* yunikorn.apache.org/namespace.enableYuniKorn
Supported values for the annotation: true and false.
@wilfred-s wilfred-s self-assigned this Mar 22, 2023
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #557 (12ff450) into master (551f6b7) will increase coverage by 0.23%.
The diff coverage is 90.44%.

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   69.71%   69.94%   +0.23%     
==========================================
  Files          45       46       +1     
  Lines        7782     7880      +98     
==========================================
+ Hits         5425     5512      +87     
- Misses       2155     2164       +9     
- Partials      202      204       +2     
Impacted Files Coverage Δ
pkg/admission/util.go 94.59% <75.00%> (-5.41%) ⬇️
pkg/admission/namespace_cache.go 85.71% <85.71%> (ø)
pkg/admission/admission_controller.go 65.31% <100.00%> (+0.64%) ⬆️
pkg/admission/informers.go 90.32% <100.00%> (+1.03%) ⬆️
pkg/admission/priority_class_cache.go 81.66% <100.00%> (+0.96%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

Not entirely thrilled with the "int as tri-state bool" pattern... It just feels ugly. An enum with constants might be cleaner, and easier to follow in the logic. True/False/Maybe. We get type safety that way too.

I'm +1 on the patch overall, but wouldn't mind a different approach for that.

Comment on lines +536 to +537
if process != -1 {
return process == 1
Copy link
Contributor

@pbacsko pbacsko Mar 23, 2023

Choose a reason for hiding this comment

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

1 / 0 / -1 are special values, could you name them? A bit difficult to reason about the logic without names, comments help, but it's better if we can follow the code without them.

Copy link
Contributor Author

@wilfred-s wilfred-s Mar 24, 2023

Choose a reason for hiding this comment

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

Follow up jira to move to an iota: YUNIKORN-1653

@pbacsko
Copy link
Contributor

pbacsko commented Mar 23, 2023

Not entirely thrilled with the "int as tri-state bool" pattern... It just feels ugly. An enum with constants might be cleaner, and easier to follow in the logic. True/False/Maybe. We get type safety that way too.

+1 on this, naming the values increases readability.

@wilfred-s wilfred-s closed this in d110439 Mar 24, 2023
@wilfred-s wilfred-s deleted the YUNIKORN-1647 branch March 28, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants