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

the CloneSet pod lifecycle can be transformed to Normal only if ContainerReady is true #1489

Conversation

chenpeicheng9
Copy link

@chenpeicheng9 chenpeicheng9 commented Jan 21, 2024

Ⅰ. Describe what this PR does

The CloneSet pod lifecycle can be transformed to Normal only if ContainerReady is true.

Sorry for being late.
Last week After I created issue #1485, I appreciate that the Kruise team replied quickly and pointed out that it may be better "to Normal after Pod Ready" @zmberg
I've been thinking about this issue these days. I believe using the 'PodReady'(k8s.io/api/core/v1.PodReady, 'Ready' in yaml, same as below) condition may not be appropriate. Consider the scenario with the 'markPodNotReady' set: the Pod's lifecycle can only be transformed to 'Normal' if 'PodReady' is true. On the other hand, the 'PodReady' condition will be set to false at preparingDelete/preparingUpdate state. This bidirectional influence adds complexity and can make the lifecycle confusing.
So I suggest that we can use the "ContainerReady" condition, it would be simple enough.
A pod's lifecycle will be transformed from PreparingNormal to Normal only if its containers are all ready.
Furthermore, it is ok that a pod's lifecycle will be transformed to Normal only if its containers are all ready.

Ⅱ. Does this pull request fix one issue?

fixes #1485

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign fei-guo for approval by writing /assign @fei-guo in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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
Copy link

Welcome @chenpeicheng9! It looks like this is your first PR to openkruise/kruise 🎉

@kruise-bot kruise-bot added the size/M size/M: 30-99 label Jan 21, 2024
Copy link

codecov bot commented Jan 21, 2024

Codecov Report

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

Project coverage is 48.54%. Comparing base (30a660b) to head (924e7a6).
Report is 52 commits behind head on master.

Files Patch % Lines
pkg/util/pods.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1489      +/-   ##
==========================================
+ Coverage   48.40%   48.54%   +0.13%     
==========================================
  Files         157      157              
  Lines       22440    22485      +45     
==========================================
+ Hits        10862    10915      +53     
+ Misses      10387    10364      -23     
- Partials     1191     1206      +15     
Flag Coverage Δ
unittests 48.54% <75.00%> (+0.13%) ⬆️

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.

if cs.Spec.Lifecycle != nil && !lifecycle.IsPodAllHooked(cs.Spec.Lifecycle.InPlaceUpdate, pod) {
state = appspub.LifecycleStateUpdated
} else {
if util.IsRunningAndContainerReady(pod) && lifecycle.IsInPlaceUpdateHookNilOrAllHooked(cs.Spec.Lifecycle, pod) {
Copy link
Member

Choose a reason for hiding this comment

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

For now, just consider determining if it's ready from preNormal to normal. It is not recommended to consider from update to normal.

@zmberg
Copy link
Member

zmberg commented Jan 23, 2024

@chenpeicheng9 Can you communicate through dingding?

@chenpeicheng9
Copy link
Author

@chenpeicheng9 Can you communicate through dingding?

sure

@chenpeicheng9 chenpeicheng9 requested a review from zmberg January 23, 2024 12:21
@furykerry
Copy link
Member

plz squash and sign the commits

@chenpeicheng9 chenpeicheng9 force-pushed the cloneset-lifecycle-to-normal branch from 5b12ff5 to 924e7a6 Compare January 27, 2024 09:42
@chenpeicheng9
Copy link
Author

As discussed, for now, I have removed the update-to-normal part. And made slight modifications to the comments. @zmberg.
Thanks, @furykerry I've squashed and signed it now.

Copy link

stale bot commented Apr 26, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Apr 26, 2024
@stale stale bot closed this May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M size/M: 30-99 wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IMPROVEMENT] [CloneSet] [Lifecycle] Kruise v1.4+ works not so well with kube-scheduler <= v1.21
4 participants