Skip to content

tests: run restore etcd from snapshot test using a new subcommand #23080

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
Closed
vrutkovs wants to merge 1 commit into from
Closed

tests: run restore etcd from snapshot test using a new subcommand #23080

vrutkovs wants to merge 1 commit into from

Conversation

@vrutkovs
Copy link
Contributor

@vrutkovs vrutkovs commented Jun 7, 2019 •
edited
Loading

This test is equivalent to bash function in CI's installer template.
It runs in a dedicated subcommand and ensure cluster state is properly
being restored after etcd snapshot restore procedure.

TODO:

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 7, 2019
@vrutkovs vrutkovs force-pushed the disaster-recovery-tests branch from 0e86dec to 71f5c03 Compare June 7, 2019 10:50
@vrutkovs vrutkovs mentioned this pull request Jun 7, 2019
Closed
2 tasks
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jun 7, 2019

/cc @hexfusion @runcom

hexfusion
hexfusion reviewed Jun 7, 2019
View reviewed changes
Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

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

Wow this looks great, just few notes on first pass. I will test a bit and let you know if anything else pops up.

test/e2e/dr/restore-from-snapshot.go
_, err = oc.Run("create").Args("-n", ns, "secret", "generic", "ssh-host-keys", "--from-file", secretKeyArgs).Output()
o.Expect(err).NotTo(o.HaveOccurred())
} else {
o.Expect(err).NotTo(o.HaveOccurred())
Copy link
Contributor

@hexfusion hexfusion Jun 7, 2019

Choose a reason for hiding this comment

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

Regarding the scope of err just making sure you are intended to report on err from

_, err = oc.AdminKubeClient().CoreV1().Secrets(ns).Get("ssh-host-keys", metav1.GetOptions{}) on line 137

Copy link
Contributor Author

@vrutkovs vrutkovs Jun 7, 2019

Choose a reason for hiding this comment

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

Yeah, that's intended. "not found" error would be skipped and the secret would be created, thus two expects in different scopes.

Perhaps in-scope err should be renamed for clarity, wdyt?

test/e2e/dr/restore-from-snapshot.go
}

func constructEtcdConnectionString(masters []string, proxy string) string {
//TODO vrutkovs: replace this nonsense with `etcdctl member list -w json ...`
Copy link
Contributor

@hexfusion hexfusion Jun 7, 2019

Choose a reason for hiding this comment

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

+1 :)

Copy link
Contributor

@hexfusion hexfusion Jun 7, 2019

Choose a reason for hiding this comment

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

I will hack on this.

@vrutkovs vrutkovs changed the title tests: run restore etcd from snapshot test using a new subcommand WIP tests: run restore etcd from snapshot test using a new subcommand Jun 7, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 7, 2019
tests: run restore etcd from snapshot test using a new subcommand
65bf6f1 
This test is equivalent to bash function in CI's installer template.
It runs in a dedicated subcommand and ensure cluster state is properly
being restored after etcd snapshot restore procedure.
@vrutkovs vrutkovs force-pushed the disaster-recovery-tests branch from 7b66bdf to 65bf6f1 Compare June 10, 2019 07:14
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 10, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vrutkovs
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vrutkovs vrutkovs mentioned this pull request Jun 10, 2019
Closed
@vrutkovs vrutkovs changed the title WIP tests: run restore etcd from snapshot test using a new subcommand tests: run restore etcd from snapshot test using a new subcommand Jun 10, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2019
runcom
runcom reviewed Jun 13, 2019
View reviewed changes
cmd/openshift-tests/disaster-recovery.go
}

func initDRSnapshotRestore(value string) error {
if len(value) == 0 {
Copy link
Member

@runcom runcom Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

nit if value == ""

Copy link
Contributor

@deads2k deads2k Jun 18, 2019

Choose a reason for hiding this comment

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

nit if value == ""

project standard is to use len. If you have instances of the == "" please update to match the kube/openshift standard.

@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jun 13, 2019

/cc @smarterclayton

Rewrote DR restore from snapshot scenario test, PTAL

vrutkovs
vrutkovs commented Jun 18, 2019
View reviewed changes
test/e2e/dr/restore-from-snapshot.go
"sudo -i install -o core -g core /root/assets/backup/snapshot.db /tmp/snapshot.db")
setMachineConfig("rollback-B.yaml", oc, mcps)

scpFileToHost(os.Getenv("KUBE_SSH_KEY_PATH"), proxy, "/home/core/.ssh/id_rsa", firstMaster)
Copy link
Contributor Author

@vrutkovs vrutkovs Jun 18, 2019

Choose a reason for hiding this comment

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

Discussed this with @deads2k - the code in lines L78-95 is running when the cluster is considered to be in a bad state already.
This should be moved to a bash script, as it would be a great example for the customers of how DR is being tested in CI.

@vrutkovs vrutkovs mentioned this pull request Jun 19, 2019
Merged
10 tasks
@vrutkovs
Copy link
Contributor Author

vrutkovs commented Jun 20, 2019

See #23208

@vrutkovs vrutkovs closed this Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deads2k deads2k deads2k left review comments

@bparees bparees Awaiting requested review from bparees

@gabemontero gabemontero Awaiting requested review from gabemontero

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

+2 more reviewers

@runcom runcom runcom left review comments

@hexfusion hexfusion hexfusion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vrutkovs @openshift-ci-robot @runcom @hexfusion @deads2k