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

Statefulset PreDeploy flow checks for pod readiness if labels were patched #841

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Statefulset PreDeploy flow checks for pod readiness if labels were patched #841

merged 1 commit into from
Jul 24, 2024

Conversation

shreyas-s-rao
Copy link
Contributor

How to categorize this PR?

/area quality
/kind bug

What this PR does / why we need it:
Statefulset PreDeploy flow checks for pod readiness if labels were patched. This ensures there is no transient quorum loss due to back-to-back double rolling of sts pods during downgrade of etcd-druid from master/v0.23.0+ version to v0.22.x version.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
/cc @unmarshall @seshachalam-yv @ishan16696

Release note:

NONE

@shreyas-s-rao shreyas-s-rao requested a review from a team as a code owner July 23, 2024 12:13
@gardener-robot gardener-robot added area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/bug Bug needs/review Needs review labels Jul 23, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 23, 2024
@gardener-robot gardener-robot added the size/s Size of pull request is small (see gardener-robot robot/bots/size.py) label Jul 23, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 23, 2024
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Jul 23, 2024
@shreyas-s-rao
Copy link
Contributor Author

/retest

1 similar comment
@shreyas-s-rao
Copy link
Contributor Author

/retest

seshachalam-yv
seshachalam-yv approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@seshachalam-yv seshachalam-yv left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Jul 23, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 23, 2024
@ishan16696
Copy link
Member

ishan16696 commented Jul 24, 2024 •
edited

Hi @shreyas-s-rao ,
why can't we use this https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#minimum-ready-seconds to give some extra buffer to the pods to become Ready ?

@shreyas-s-rao
Copy link
Contributor Author

@ishan16696 this is a good suggestion in general, but atleast as of today druid does not set minReadySeconds on the sts, and druid is the only actor who creates and manages the sts. So there's no need to consider this field here.

ishan16696
ishan16696 approved these changes Jul 24, 2024
View reviewed changes
Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

LGTM!!

@ishan16696
Copy link
Member

@ishan16696 this is a good suggestion in general, but atleast as of today druid does not set minReadySeconds on the sts, and druid is the only actor who creates and manages the sts. So there's no need to consider this field here.

yeah, let's discuss this later . As of now, we can go ahead with this PR.

@ishan16696 ishan16696 merged commit b7dcd31 into gardener:hotfix-v0.22 Jul 24, 2024
11 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jul 24, 2024
@shreyas-s-rao shreyas-s-rao deleted the fix/predeploy-ready-check branch July 24, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishan16696 ishan16696 ishan16696 approved these changes

@seshachalam-yv seshachalam-yv seshachalam-yv approved these changes

Assignees
No one assigned
Labels
area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@shreyas-s-rao @ishan16696 @seshachalam-yv @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot