Skip to content

Update statefulset.md #48240

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ayushpatil2122
Copy link
Contributor

Description
The documentation states that setting the podManagementPolicy to Parallel only affects scaling operations, not updates (like changes to the pod template). This suggests that during updates, pods should still be managed sequentially.

However, in practice, when you set podManagementPolicy to Parallel, updates are happening in parallel, meaning pods are updated without waiting for others to finish, which contradicts what the docs say.

Issue
#47085

@k8s-ci-robot k8s-ci-robot requested a review from enisoc October 7, 2024 13:37
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Oct 7, 2024
@k8s-ci-robot k8s-ci-robot requested a review from kow3ns October 7, 2024 13:37
@k8s-ci-robot
Copy link
Contributor

[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 natalisucks for approval. 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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 7, 2024
Copy link

netlify bot commented Oct 7, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 0c91f41
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6703e41c55b0840008786533
😎 Deploy Preview https://deploy-preview-48240--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@niranjandarshann
Copy link
Contributor

Looks Good to me
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2024
@@ -298,8 +298,7 @@ described [above](#deployment-and-scaling-guarantees).
`Parallel` pod management tells the StatefulSet controller to launch or
terminate all Pods in parallel, and to not wait for Pods to become Running
and Ready or completely terminated prior to launching or terminating another
Pod. This option only affects the behavior for scaling operations. Updates are not
affected.
Pod. This option only affects the behavior for scaling operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change actually affects the meaning of the paragraph (it's removing a clarification). If we intended to change the meaning, I think we should do a different rewrite.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed expected change as described in kubernetes/kubernetes#117071 and fixed in kubernetes/kubernetes#117865. With that fix we should be explicit that this operation applies to both scaling and pod creation.

Copy link
Contributor

Choose a reason for hiding this comment

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

There will be followup changes after we push forward the mechanism described in kubernetes/enhancements#961 which currently blocked on monitoring requirements to be able to progress with that effort.

Copy link
Contributor

@niranjandarshann niranjandarshann Oct 12, 2024

Choose a reason for hiding this comment

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

This is needful as it is described in kubernetes/kubernetes#117071 and was fixed in kubernetes/kubernetes#117865 This is expected changes

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference in meaning between the two options (before and after) here?

-This option only affects the behavior for scaling operations. Updates are not affected.
+This option only affects the behavior for scaling operations.

To me they have the same meaning, although the original includes a clarifying statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sftim got your point. Yes we should not ignore the last sentence as well like Updates are not affected.

The sentence is mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestions for improving this sentence

Suggested change
Pod. This option only affects the behavior for scaling operations.
This option only affects the behavior of scaling operations and does not impact updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

I this this sentence will look more better .

Choose a reason for hiding this comment

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

From what I have tested, this option affects updates, too. See #47085. Maybe I am not able to correctly understand what we mean by updates here.

Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

lgtm

@sftim
Copy link
Contributor

sftim commented Oct 12, 2024

Given #48240 (comment)

/lgtm cancel

Let's make sure we're changing meaning here. @ayushpatil2122 if you can set a short PR description that summarizes the change, it will make reviewing easier.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2024
@soltysh
Copy link
Contributor

soltysh commented Oct 14, 2024

Let's make sure we're changing meaning here. @ayushpatil2122 if you can set a short PR description that summarizes the change, it will make reviewing easier.

I agree with Tim here, make it explicit what the current behavior is rather then implying functionality through the removal of the negation.

@tvsfx
Copy link

tvsfx commented Nov 5, 2024

FWIW, as an outsider, I'm still confused about the intended semantics of the Parallel option and the proposed rephrasing.

The source of my confusion is the following: if neither scaling operations nor updates are constrained when Parallel is set, then which operations are still contrained by order? It seems as though none are to me.
I raised a question about this on the kubernetes forums as well.

@sftim
Copy link
Contributor

sftim commented Dec 12, 2024

@ayushpatil2122 what's your intention in terms of this PR?

There is pending feedback; do you agree about revising the proposed changes?

@sftim
Copy link
Contributor

sftim commented Jan 27, 2025

A maintainer may close this PR (due to inactivity). @ayushpatil2122 you would still be welcome to reopen it and continue work, if that had happened.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants