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_enhanced_livenessProbe_webhook #1467

Merged

Conversation

BH4AWS
Copy link
Collaborator

@BH4AWS BH4AWS commented Dec 12, 2023

Ⅰ. Describe what this PR does

Q1: How to convert the standard livenessProbe configuration to this schema?
A webhook can convert the standard livenssProbe to the special field in pod annotations. The detail key is "apps.kruise.io/livenessprobe-context".

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it?

Pod within "annotation: apps.kruise.io/using-enhanced-liveness = true" is created in Kubernetes apiServer. The livenessProbe configuration will be cleaned and backuped in Pod "annotation: apps.kruise.io/livenessprobe-context".

Ⅳ. Special notes for reviews

@kruise-bot kruise-bot added the size/L size/L: 100-499 label Dec 12, 2023
utilfeature "github.com/openkruise/kruise/pkg/util/feature"
)

const (
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving the api to the common package like ./pkg/util or apis?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

func (h *PodCreateHandler) enhancedLivenessProbeWhenPodCreate(req admission.Request, pod *v1.Pod) (skip bool, err error) {

if len(req.AdmissionRequest.SubResource) > 0 ||
req.AdmissionRequest.Operation != admissionv1.Create ||
Copy link
Member

Choose a reason for hiding this comment

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

What if users modify LivenessProbe when updating pods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

v1 version, the native version just supports the creation process.

return true, nil
}

if !util.IsPodOwnedByKruise(pod) && !utilfeature.DefaultFeatureGate.Enabled(features.EnhancedLivenessProbe) {
Copy link
Member

Choose a reason for hiding this comment

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

The condition utilfeature.DefaultFeatureGate.Enabled(features.EnhancedLivenessProbe) seems to be duplicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Considering the determinacy of configuration, a feature gate EnhancedLivenessProbe will be set in our project.


const (
AnnotationUsingEnhancedLiveness = "apps.kruise.io/using-enhanced-liveness"
AnnotationNativeLivenessContext = "apps.kruise.io/livenessprobe-context"
Copy link
Member

Choose a reason for hiding this comment

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

apps.kruise.io/container-probe-context ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@ls-2018
Copy link
Member

ls-2018 commented Dec 20, 2023

@BH4AWS BH4AWS force-pushed the add_enhanced_livenessProbe_webhook branch from 0b69996 to 12178e2 Compare March 15, 2024 15:16
@kruise-bot kruise-bot added size/XL size/XL: 500-999 and removed size/L size/L: 100-499 labels Mar 15, 2024
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 56.14035% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 47.92%. Comparing base (c33088b) to head (26efbda).
Report is 2 commits behind head on master.

Files Patch % Lines
...hook/pod/mutating/enhancedlivenessprobe_handler.go 62.74% 14 Missing and 5 partials ⚠️
.../webhook/pod/mutating/pod_create_update_handler.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1467      +/-   ##
==========================================
+ Coverage   47.89%   47.92%   +0.02%     
==========================================
  Files         161      162       +1     
  Lines       23425    23482      +57     
==========================================
+ Hits        11220    11254      +34     
- Misses      10990    11009      +19     
- Partials     1215     1219       +4     
Flag Coverage Δ
unittests 47.92% <56.14%> (+0.02%) ⬆️

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.

@BH4AWS BH4AWS force-pushed the add_enhanced_livenessProbe_webhook branch from 12178e2 to b5316f8 Compare March 18, 2024 03:59
}

func removeAndBackUpPodContainerLivenessProbe(pod *v1.Pod) (string, error) {
if len(pod.Spec.Containers) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

this check is unnecessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@BH4AWS BH4AWS force-pushed the add_enhanced_livenessProbe_webhook branch from b5316f8 to e19bee7 Compare March 18, 2024 07:07
return false, err
}
if context == "" {
klog.Warningf("No found the native container livenessProbe config for pod: %s/%s", pod.Namespace, pod.Name)
Copy link
Member

Choose a reason for hiding this comment

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

the warning seems unnecessary, consider remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@BH4AWS BH4AWS force-pushed the add_enhanced_livenessProbe_webhook branch 2 times, most recently from c03fc48 to 797df4b Compare March 19, 2024 03:22
Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com>
@BH4AWS BH4AWS force-pushed the add_enhanced_livenessProbe_webhook branch from 797df4b to 26efbda Compare March 19, 2024 09:09
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

@zmberg zmberg added this to the 1.7 milestone Mar 20, 2024
@zmberg
Copy link
Member

zmberg commented Mar 20, 2024

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

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

@kruise-bot kruise-bot merged commit 7270f40 into openkruise:master Mar 20, 2024
34 checks passed
ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024
Co-authored-by: jicheng.sk <jicheng.sk@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants