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

Support events in restore script #35784

Merged
merged 1 commit into from
Nov 2, 2016
Merged

Support events in restore script #35784

merged 1 commit into from
Nov 2, 2016

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Oct 28, 2016 •
edited by k8s-oncall
Loading

Ref #20504

This change is Reviewable

@wojtek-t wojtek-t added retest-not-required release-note-none Denotes a PR that doesn't merit a release note. labels Oct 28, 2016
@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 28, 2016
roberthbailey
roberthbailey reviewed Oct 28, 2016
View reviewed changes
cluster/restore-from-backup.sh
# Make it possible to overwrite version file (or default version)
# with VERSION_CONTENTS env var.
if [ ! -z "${VERSION_CONTENTS:-}" ]; then
echo "${VERSION_CONTENTS}" > "${VERSION_FILE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

will this fail if version.txt already exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Should it?

cluster/restore-from-backup.sh
@@ -205,6 +212,13 @@ mv /var/etcd/data "${MNT_DISK}/var/etcd-corrupted"
# Replace the corrupted data dir with the resotred data.
mv "${BACKUP_DIR}" /var/etcd/data

if [ "${RESTORE_EVENT_ETCD:-}" == "true" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

since this doesn't restore, it just resets, maybe we could name this RESET_EVENT_ETCD or something that is more closely aligned with the effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

mml
mml suggested changes Oct 28, 2016
View reviewed changes
cluster/restore-from-backup.sh

# Make it possible to overwrite version file (or default version)
# with VERSION_CONTENTS env var.
if [ ! -z "${VERSION_CONTENTS:-}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Might just be me, but I find if [ -n "$blah" ]; then more idiomatic.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

cluster/restore-from-backup.sh
# with VERSION_CONTENTS env var.
if [ ! -z "${VERSION_CONTENTS:-}" ]; then
echo "${VERSION_CONTENTS}" > "${VERSION_FILE}"
echo "TARGET_STORAGE variable unset - skipping migration"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this message? We haven't even checked TARGET_STORAGE.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy-paster error.

cluster/restore-from-backup.sh
@@ -205,6 +212,13 @@ mv /var/etcd/data "${MNT_DISK}/var/etcd-corrupted"
# Replace the corrupted data dir with the resotred data.
mv "${BACKUP_DIR}" /var/etcd/data

if [ "${RESTORE_EVENT_ETCD:-}" == "true" ]; then
# Save the corrupted data (clean directory if it is already non-empty).
rm -rf "${MNT_DISK}/var/etcd-events-corrupted"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put this directory name in a variable, since we repeat it three times.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

wojtek-t
wojtek-t commented Oct 31, 2016
View reviewed changes
Copy link
Member Author

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

PTAL

cluster/restore-from-backup.sh

# Make it possible to overwrite version file (or default version)
# with VERSION_CONTENTS env var.
if [ ! -z "${VERSION_CONTENTS:-}" ]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

done

cluster/restore-from-backup.sh
# with VERSION_CONTENTS env var.
if [ ! -z "${VERSION_CONTENTS:-}" ]; then
echo "${VERSION_CONTENTS}" > "${VERSION_FILE}"
echo "TARGET_STORAGE variable unset - skipping migration"
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy-paster error.

cluster/restore-from-backup.sh
@@ -205,6 +212,13 @@ mv /var/etcd/data "${MNT_DISK}/var/etcd-corrupted"
# Replace the corrupted data dir with the resotred data.
mv "${BACKUP_DIR}" /var/etcd/data

if [ "${RESTORE_EVENT_ETCD:-}" == "true" ]; then
# Save the corrupted data (clean directory if it is already non-empty).
rm -rf "${MNT_DISK}/var/etcd-events-corrupted"
Copy link
Member Author

Choose a reason for hiding this comment

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

done

cluster/restore-from-backup.sh
@@ -205,6 +212,13 @@ mv /var/etcd/data "${MNT_DISK}/var/etcd-corrupted"
# Replace the corrupted data dir with the resotred data.
mv "${BACKUP_DIR}" /var/etcd/data

if [ "${RESTORE_EVENT_ETCD:-}" == "true" ]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 7fe1e06. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@mml
Copy link
Contributor

mml commented Oct 31, 2016

@k8s-bot gci gke e2e test this ISSUE: #35913

@mml
Copy link
Contributor

mml commented Oct 31, 2016

LGTM, but it looks like @roberthbailey still has one outstanding question.

@wojtek-t
Copy link
Member Author

wojtek-t commented Nov 2, 2016

Talked with @roberthbailey offline and discussed his comment. He agreed that what is now is ok.

So applying lgtm label.

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2016
@roberthbailey
Copy link
Contributor

lgtm

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 94a6538 into kubernetes:master Nov 2, 2016
@wojtek-t wojtek-t deleted the tweak_restore_script branch November 18, 2016 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberthbailey roberthbailey roberthbailey left review comments

@mml mml mml approved these changes

Assignees

@mml mml

@roberthbailey roberthbailey

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wojtek-t @k8s-ci-robot @mml @roberthbailey @k8s-github-robot @googlebot