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 the revision consitency check error handling #200

Merged
merged 4 commits into from
Sep 11, 2019
Merged

Fix the revision consitency check error handling #200

merged 4 commits into from
Sep 11, 2019

Conversation

swapnilgm
Copy link
Contributor

Signed-off-by: Swapnil Mhamane [email protected]

What this PR does / why we need it:

  • Because of incorrect error handling at revision consistency check, the etcd-backup-restore used to fail the validation and trigger restore for valid data directory. This was noticed when network was down at validation time but available at restore time. This PR fixes the issue.
  • Additionally uses etcd API for etcd snap directory validation. We don't have to maintain code here anymore.
  • Improves the test coverage for validator package.
    Which issue(s) this PR fixes:
    Fixes #

Special notes for your reviewer:

Release note:

Fix the error handling in revision consistency check leading to restoration on valid etcd data directory.

@swapnilgm swapnilgm added kind/bug Bug priority/critical Needs to be resolved soon, because it impacts users negatively needs/review Needs review component/etcd-backup-restore ETCD Backup & Restore platform/all needs/lgtm Needs approval for merging area/backup Backup related needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 9, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 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 Sep 9, 2019
@swapnilgm swapnilgm force-pushed the fix/revision-consistency-check branch from 481c09c to 8631d8c Compare September 9, 2019 10:12
@gardener-robot-ci-1 gardener-robot-ci-1 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 Sep 9, 2019
@swapnilgm swapnilgm force-pushed the fix/revision-consistency-check branch from 8631d8c to d674902 Compare September 9, 2019 10:31
@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 Sep 9, 2019
Update the documentation
f6b94fc 
Signed-off-by: Swapnil Mhamane <[email protected]>
@swapnilgm swapnilgm force-pushed the fix/revision-consistency-check branch from d674902 to f6b94fc Compare September 9, 2019 10:33
@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 Sep 9, 2019
Fix watch cleanup handling
9649ae4 
Signed-off-by: Swapnil Mhamane <[email protected]>
@swapnilgm swapnilgm force-pushed the fix/revision-consistency-check branch from 4c60f60 to 9649ae4 Compare September 10, 2019 09:56
@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 Sep 10, 2019
shreyas-s-rao
shreyas-s-rao requested changes Sep 10, 2019
View reviewed changes
Copy link
Collaborator

@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.

@swapnilgm Thanks a lot for the well-written PR! Overall LGTM. Just a few minor changes/questions. Please address them. Thanks.

pkg/initializer/validator/types.go Outdated Show resolved Hide resolved
doc/development/new_cp_support.md Outdated Show resolved Hide resolved
doc/development/new_cp_support.md Outdated Show resolved Hide resolved
doc/development/new_cp_support.md Outdated Show resolved Hide resolved
doc/development/new_cp_support.md Outdated Show resolved Hide resolved
doc/development/new_cp_support.md Outdated Show resolved Hide resolved
pkg/snapshot/restorer/restorer_test.go Show resolved Hide resolved
pkg/initializer/validator/datavalidator_test.go Outdated Show resolved Hide resolved
pkg/initializer/validator/datavalidator_test.go Outdated Show resolved Hide resolved
pkg/initializer/validator/datavalidator_test.go 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 Sep 11, 2019
@shreyas-s-rao
Apply suggestions from code review
9ed7ca1 
Co-Authored-By: Shreyas Rao <[email protected]>
Signed-off-by: Swapnil Mhamane <[email protected]>
@swapnilgm swapnilgm force-pushed the fix/revision-consistency-check branch from 7c36a48 to 9ed7ca1 Compare September 11, 2019 05:25
@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 Sep 11, 2019
@swapnilgm
Copy link
Contributor Author

Thank you for the review. I have updated the PR. PTAL.

shreyas-s-rao
shreyas-s-rao approved these changes Sep 11, 2019
View reviewed changes
Copy link
Collaborator

@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.

@swapnilgm Thanks for making the changes. LGTM.

@shreyas-s-rao shreyas-s-rao merged commit f645685 into gardener:master Sep 11, 2019
@swapnilgm swapnilgm deleted the fix/revision-consistency-check branch September 11, 2019 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreyas-s-rao shreyas-s-rao shreyas-s-rao approved these changes

@georgekuruvillak georgekuruvillak Awaiting requested review from georgekuruvillak

Assignees
No one assigned
Labels
area/backup Backup related component/etcd-backup-restore ETCD Backup & Restore kind/bug Bug needs/lgtm Needs approval for merging needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review platform/all priority/critical Needs to be resolved soon, because it impacts users negatively
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@swapnilgm @shreyas-s-rao @gardener-robot-ci-1