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

Added a safety check before add a learner in etcd cluster. #605

Merged
merged 4 commits into from Apr 14, 2023
Merged

Added a safety check before add a learner in etcd cluster. #605

merged 4 commits into from Apr 14, 2023

Conversation

ishan16696
Copy link
Member

@ishan16696 ishan16696 commented Mar 18, 2023 •
edited

What this PR does / why we need it:
This PR adds a safety check of whether data-dir exist or not before adding new member as a learner(non-voting member) in etcd cluster.

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

Special notes for your reviewer:

Release note:

Added a safety check before adding a learner(non-voting) member in etcd cluster.

@ishan16696 ishan16696 requested a review from a team as a code owner March 18, 2023 04:32
@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 Mar 18, 2023
@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) 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 Mar 18, 2023
aaronfern
aaronfern requested changes Mar 18, 2023
View reviewed changes
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!
Can you please add a test case to test this functionality?

pkg/member/member_control.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Mar 18, 2023
@ishan16696
Copy link
Member Author

Can you please add a test case to test this functionality?

A positive test case is already there, and for -ve tests I can't mock function like os.Stat or os.RemoveAll as they are not defined by interface.

@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 Mar 18, 2023
@ishan16696 ishan16696 added the merge/squash Should be merged via 'Squash and merge' label Mar 20, 2023
@aaronfern aaronfern added the reviewed/lgtm Has approval for merging label Mar 20, 2023
@gardener-robot gardener-robot removed needs/changes Needs (more) changes needs/review Needs review labels Mar 20, 2023
@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 Mar 20, 2023
@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Mar 21, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 removed 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 Mar 21, 2023
aaronfern
aaronfern reviewed Mar 22, 2023
View reviewed changes
pkg/errors/error.go Outdated Show resolved Hide resolved
@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 Mar 22, 2023
unmarshall
unmarshall reviewed Mar 27, 2023
View reviewed changes
pkg/errors/error.go Show resolved Hide resolved
pkg/initializer/initializer.go Outdated Show resolved Hide resolved
pkg/initializer/initializer.go
@@ -252,7 +258,15 @@ func (e *EtcdInitializer) restoreInMultiNode(ctx context.Context) error {
return fmt.Errorf("unable to remove the data-dir %v", err)
}

if err := retry.OnError(retry.DefaultBackoff, errors.AnyError, func() error {
if err := retry.OnError(retry.DefaultBackoff, errors.IsErrNotNil, func() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a code repetition. Can we extract this into a reusable function?

abdasgupta
abdasgupta approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@abdasgupta abdasgupta 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. Looks good to me. /lgtm

@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 Apr 12, 2023
@ishan16696
Copy link
Member Author

ishan16696 commented Apr 12, 2023 •
edited

Resolved the merge conflicts. PTAL

@ishan16696 ishan16696 merged commit 96e810f into gardener:master Apr 14, 2023
9 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 14, 2023
@ishan16696 ishan16696 deleted the addLearner/safetyCheck branch April 14, 2023 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unmarshall unmarshall unmarshall left review comments

@abdasgupta abdasgupta abdasgupta approved these changes

@aaronfern aaronfern aaronfern approved these changes

Assignees
No one assigned
Labels
merge/squash Should be merged via 'Squash and merge' 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/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.

[Feature] Check data-dir exist or not before adding new member as a learner in etcd cluster
8 participants
@ishan16696 @unmarshall @abdasgupta @aaronfern @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot