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

Stopped reconciliation if quorumloss annotation is set by operator. #446

Merged
merged 4 commits into from
Nov 4, 2022

Conversation

abdasgupta
Copy link
Contributor

@abdasgupta abdasgupta commented Oct 10, 2022

How to categorize this PR?

/area TODO
/kind TODO

What this PR does / why we need it:
This PR enables ETCD controller to check for an annotation named druid.gardener.cloud/ignore-reconciliation in ETCD CR. The annotation is applied by human operator who wants to recover a cluster from permanent quorum loss. The ETCD controller does not reconcile until the annotation is removed.

Which issue(s) this PR fixes:
Fixes #
#437

Special notes for your reviewer:

Release note:

Applying the annotation `druid.gardener.cloud/ignore-reconciliation` on the ETCD CR will stop etcd-druid from reconciling it.
This is helpful for operators to apply any manual fixes to the ETCD components, such as manually fixing permanent quorum loss as per the [playbook](https://github.com/gardener/etcd-druid/blob/master/docs/operation/Recover_From_Etcd_Permanent_Quorum_Loss.md).

@abdasgupta abdasgupta requested a review from a team as a code owner October 10, 2022 06:01
@gardener-robot
Copy link

@abdasgupta Labels area/todo, kind/todo do not exist.

@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Oct 10, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 Oct 10, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has 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 Oct 10, 2022
@gardener-robot
Copy link

@abdasgupta Labels area/todo, kind/todo do not exist.

@abdasgupta abdasgupta added this to the v0.14.0 milestone Oct 10, 2022
@aaronfern
Copy link
Contributor

/assign

Copy link
Contributor

@aaronfern aaronfern left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @abdasgupta! Looks good in general

One suggestion, consider using the annotation druid.gardener.cloud/ignore instead as it's more semantically accurate

@abdasgupta
Copy link
Contributor Author

Thanks for the PR @abdasgupta! Looks good in general

One suggestion, consider using the annotation druid.gardener.cloud/ignore instead as it's more semantically accurate

Done

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has 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 Oct 11, 2022
Copy link
Contributor

@aaronfern aaronfern left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!
/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Oct 12, 2022
@ishan16696
Copy link
Member

ishan16696 commented Oct 13, 2022

Please add an appropriate release note.

Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@abdasgupta thanks for the PR. Can you please add a test for this?
Also, please update the PR note and add a release note too.

@abdasgupta
Copy link
Contributor Author

@abdasgupta thanks for the PR. Can you please add a test for this?
Also, please update the PR note and add a release note too.

@shreyas-s-rao Any idea how to add a test case for this? This PR is just not reconciling on addition of the annotation

@gardener-robot
Copy link

@abdasgupta Labels area/todo, kind/todo do not exist.

1 similar comment
@gardener-robot
Copy link

@abdasgupta Labels area/todo, kind/todo do not exist.

@abdasgupta
Copy link
Contributor Author

Updated the release note

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has 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 Oct 21, 2022
Copy link
Contributor

@aaronfern aaronfern left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @abdasgupta!

One nit and one suggestion from me

docs/operation/Recover_From_Etcd_Permanent_Quorum_Loss.md Outdated Show resolved Hide resolved
docs/operation/Recover_From_Etcd_Permanent_Quorum_Loss.md Outdated Show resolved Hide resolved
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has 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 Oct 26, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has 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 Oct 27, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has 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 Oct 31, 2022
@ishan16696
Copy link
Member

Please don't do a force-push it makes review difficult.
As we decided please address review as separate commit and at the end we will do merge-squash.

Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@abdasgupta Thanks for making the changes. Final few comments from my side.

Please also update the release note to add the full link to the document - https://github.com/gardener/etcd-druid/blob/master/docs/operation/Recover_From_Etcd_Permanent_Quorum_Loss.md

docs/operation/Recover_From_Etcd_Permanent_Quorum_Loss.md Outdated Show resolved Hide resolved
docs/operation/Recover_From_Etcd_Permanent_Quorum_Loss.md Outdated Show resolved Hide resolved
@gardener-robot
Copy link

@abdasgupta Labels area/todo, kind/todo do not exist.

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has 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 Nov 3, 2022
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@abdasgupta Thanks for making the requested changes.
/lgtm

Copy link
Contributor

@aaronfern aaronfern left a comment

Choose a reason for hiding this comment

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

/lgtm

@abdasgupta abdasgupta merged commit fd1b423 into gardener:master Nov 4, 2022
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Nov 4, 2022
@abdasgupta abdasgupta deleted the playbook branch November 4, 2022 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/high-availability High availability related kind/enhancement Enhancement, improvement, extension 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 size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants