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.

Sign up for GitHub

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

Jump to bottom

Fix rolling update for StatefulSet integration #3684

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix rolling update for StatefulSet integration #3684

wants to merge 8 commits into from

Conversation

mbobrovskyi
Copy link
Contributor

@mbobrovskyi mbobrovskyi commented Nov 28, 2024 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fix rolling update for StatefulSet integration

Which issue(s) this PR fixes:

Fixes #3690

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed rolling update for StatefulSet integration

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 28, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 28, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 28, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 45c310b
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6749d131a1ee3100083ab115

@mbobrovskyi
Copy link
Contributor Author

/hold for #3686

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2024
@mbobrovskyi mbobrovskyi marked this pull request as ready for review November 28, 2024 19:45
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 28, 2024
@mbobrovskyi mbobrovskyi force-pushed the fix/sts-rolling-update branch 2 times, most recently from 7f3d79d to 3625407 Compare November 29, 2024 07:19
@mbobrovskyi mbobrovskyi changed the title Allow to update image on StatefulSet. Allow to update pod spec on StatefulSet. Nov 29, 2024
@mbobrovskyi mbobrovskyi changed the title Allow to update pod spec on StatefulSet. Fix rolling update for StatefulSet integration Nov 29, 2024
@mbobrovskyi mbobrovskyi mentioned this pull request Nov 29, 2024
Closed
@mbobrovskyi
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2024
@mbobrovskyi mbobrovskyi force-pushed the fix/sts-rolling-update branch 2 times, most recently from 80a0cb9 to 58d7730 Compare November 29, 2024 14:03
mimowo
mimowo reviewed Nov 29, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, just nits from me. I think we could consider a dedicated KEP for serving workloads, where we could mention the new annotation (and the fast-admission annotation added before), but I wouldn't like to block the PR on it before 0.10, as this is an important fix.

cc @tenzen-y

test/e2e/singlecluster/statefulset_test.go Outdated Show resolved Hide resolved
test/e2e/singlecluster/statefulset_test.go Outdated Show resolved Hide resolved
test/e2e/singlecluster/statefulset_test.go Outdated Show resolved Hide resolved
test/e2e/singlecluster/statefulset_test.go Show resolved Hide resolved
test/integration/controller/jobs/pod/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controller/jobs/pod/pod_controller_test.go Outdated Show resolved Hide resolved
@mimowo
Copy link
Contributor

mimowo commented Nov 29, 2024

LGTM, just nits from me. I think we could consider a dedicated KEP for serving workloads, where we could mention the new annotation (and the fast-admission annotation added before), but I wouldn't like to block the PR on it before 0.10, as this is an important fix.

cc @tenzen-y

I synced with @mbobrovskyi and he will create a KEP for serving workloads - starting from the StatefulSet support. We will then extend the KEP with LeaderWorkerSet when we have a prototype. But I think it would be great to merge this for 0.10 anyway.

mimowo
mimowo reviewed Nov 29, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold
For another look from @tenzen-y
Please also add the KEP in a separate PR with the two annotations we introduced for the support: "fast-admission" and "serving".

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 29, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 31d622888584bf77d17c73d3cb3d6166eecccc38

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbobrovskyi, mimowo

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2024
@mimowo
Copy link
Contributor

mimowo commented Nov 29, 2024

Alternatively I'm also good with a section in the PodGroup KEP entitles serving workload support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mimowo mimowo mimowo left review comments

@trasc trasc Awaiting requested review from trasc

Assignees

@mimowo mimowo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rolling update does not work for StatefulSet integration
3 participants
@mbobrovskyi @k8s-ci-robot @mimowo