-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Runtimeclass admission #78484
Runtimeclass admission #78484
Conversation
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some review comments under package plugin/pkg/admission/...
} | ||
|
||
default: | ||
return fmt.Errorf("invalid value for admissionPhase: %s", admissionPhase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: admissionPhase
sounds a bit odd to me. the mutating and validating logic should better be separated i suppose? ping @liggitt for suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, names are hard. I wanted to deduplicate some of the boiler plate handling that is needed for both mutation and validation phases of the controller. Happy to hear better suggestions for the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: Alternatively, just put the boilerplate in a helper:
pod, runtimeClass, err := r.prepareObjects(attributes)
(needs a better name though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to add the helper. I named it prepareObjects, but am going to go for a two hour bike ride this evening and think of a more appropriate name.
/remove-sig api-machinery |
b7cbeaa
to
3c91769
Compare
/assign @tallclair @yastij |
a637305
to
e860830
Compare
ack. updated. Thanks Tim. |
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
Signed-off-by: Eric Ernst <eric.ernst@intel.com>
Can you add an OWNERS file to the new admission controller. Approver should be me, and you can add yourself as a reviewer :) |
/lgtm |
added owners file |
add initial owners file for RuntimeClass admission controller Signed-off-by: Eric Ernst <eric.ernst@intel.com>
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
} | ||
} | ||
|
||
// admissionAction handles Admit and Validate phases of admission, switching based on the admissionPhase parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment needs fixing
|
||
// getRuntimeClass will return a reference to the RuntimeClass object if it is found. If it cannot be found, or a RuntimeClassName | ||
// is not provided in the pod spec, *node.RuntimeClass returned will be nil | ||
func (r *RuntimeClass) getRuntimeClass(pod *api.Pod, runtimeClassName *string) (runtimeClass *v1beta1.RuntimeClass, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pod argument is unused
nit: this is only called from prepare objects, so I'd just inline it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled in my PR #79565.
|
||
func setOverhead(a admission.Attributes, pod *api.Pod, runtimeClass *v1beta1.RuntimeClass) (err error) { | ||
|
||
if runtimeClass != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer:
if runtimeClass != nil { | |
if runtimeClass == nil || runtimeclass.Overhead == nil { | |
return nil | |
} | |
// ... |
/unassign @thockin |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, egernst, tallclair 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 |
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
return err | ||
} | ||
|
||
if utilfeature.DefaultFeatureGate.Enabled(features.PodOverhead) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be moved ahead of call to prepareObjects.
If the feature gate is disabled, pod, runtimeClass would not be used.
|
||
// getRuntimeClass will return a reference to the RuntimeClass object if it is found. If it cannot be found, or a RuntimeClassName | ||
// is not provided in the pod spec, *node.RuntimeClass returned will be nil | ||
func (r *RuntimeClass) getRuntimeClass(pod *api.Pod, runtimeClassName *string) (runtimeClass *v1beta1.RuntimeClass, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled in my PR #79565.
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
For kubernetes/enhancements#688
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
(captured by #76968)
Depends on:
#76968