Implement etcd-backup-restore on OSS of Alicloud #108

minchaow · 2019-02-01T10:21:10Z

What this PR does / why we need it:
Implement function of backing up and restoring etcd on OSS of Alicloud

Special notes for your reviewer:

When running "make verify", there was one failure testing case as follow:

• Failure [2.036 seconds]
Defrag Defragmentation [It] should defragment and reduce size of DB within time
/Workspace/go/src/github.com/gardener/etcd-backup-restore/pkg/etcdutil/defrag_test.go:53

Expected
: 258048
to be <
: 258048

/Workspace/go/src/github.com/gardener/etcd-backup-restore/pkg/etcdutil/defrag_test.go:69

Full Stack Trace
/Workspace/go/src/github.com/gardener/etcd-backup-restore/vendor/github.com/onsi/gomega/internal/assertion/assertion.go:75 +0x305
github.com/gardener/etcd-backup-restore/vendor/github.com/onsi/gomega/internal/assertion.(*Assertion).Should(0xc000938400, 0x1801ea0, 0xc0006fa570, 0x0, 0x0, 0x0, 0x1)
/Workspace/go/src/github.com/gardener/etcd-backup-restore/vendor/github.com/onsi/gomega/internal/assertion/assertion.go:28 +0x11f
github.com/gardener/etcd-backup-restore/pkg/etcdutil.glob..func1.1.2()
/Workspace/go/src/github.com/gardener/etcd-backup-restore/pkg/etcdutil/defrag_test.go:69 +0xc72
github.com/gardener/etcd-backup-restore/vendor/github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync(0xc0002f0060, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
/Workspace/go/src/github.com/gardener/etcd-backup-restore/pkg/etcdutil/etcdutil_suite_test.go:42 +0xf7
testing.tRunner(0xc0001f4500, 0x164df60)
/usr/local/go/src/testing/testing.go:827 +0x163
created by testing.(*T).Run
/usr/local/go/src/testing/testing.go:878 +0x65a

However, when I ran on the latest master branch without my change, the same error was still happened. So it is confused me if it is my code causes this error.

The implement needs 3rd part SDK, so I appended the dependency into [[constraint]] in the Gopkg.toml. When running "make revendor", not only aliyun SDK was added, but also some other dependencies were updated, which cause one problem as follow when to run "make verify":

panic: codecgen version mismatch: current: 8, need 10. Re-generate file: /Workspace/go/src/github.com/gardener/etcd-backup-restore/vendor/github.com/coreos/etcd/client/keys.generated.go

goroutine 1 [running]:
github.com/gardener/etcd-backup-restore/vendor/github.com/coreos/etcd/client.init.0()
/Workspace/go/src/github.com/gardener/etcd-backup-restore/vendor/github.com/coreos/etcd/client/keys.generated.go:45 +0x144

Ginkgo ran 1 suite in 4.835755187s
Test Suite Failed
Makefile:61: recipe for target 'test' failed
make: *** [test] Error 1

The same error was also happened when running on the latest master branch.

Release note:

Add new cloud provider OSS (Alibaba Object Storage Service) support for etcd-backup-restore

swapnilgm · 2019-02-01T13:20:53Z

@minchaow Thank you for the PR. We will review it soon.

Regarding test cases:
All tests for master and recent PR are passing in CI/CD pipeline and locally on my machine as well. So there shouldn't be any issue.

Anyways, looking at

Defrag Defragmentation [It] should defragment and reduce size of DB within time
/Workspace/go/src/github.com/gardener/etcd-backup-restore/pkg/etcdutil/defrag_test.go:53``

It looks like DB defragmentation ran on already defragmented etcd. By any chance, any other process calling defragmentation on etcd? I'm not able to reproduce it on master.
Just for safer side, try to run the test in docker container. You can use something like below:

docker run --rm -it \
    -v ${PWD}:/master-version \
   -e GOPATH="/go" \
   -w /master-version golang bash -c "make verify"

The implement needs 3rd part SDK, so I appended the dependency into [[constraint]] in the Gopkg.toml. When running "make revendor", not only aliyun SDK was added, but also some other dependencies were updated, which cause one problem as follow when to run "make verify":

This issue is similar to etcd-io/etcd#8715 . Might have been triggered due to later release of go-codec library 1.1.2.
Till upstream fixes this, do the following.
Please add following line to Gopkg.toml at the end and run make revendor again. Make sure that Gopkg.lock reflects the codec version in use as 1.1.1.

[[override]]
name = "github.com/ugorji/go"
revision = "b4c50a2b199d93b13dc15e78929cfb23bfdf21ab"

swapnilgm

Just skimmed thought it, will review it again. As suggested in previous comment please do the revendoring. Also, add the license details for SDK in https://github.com/gardener/etcd-backup-restore/blob/master/LICENSE.md#subcomponents.

pkg/snapstore/oss_snapstore.go

pkg/snapstore/oss_snapstore_test.go

minchaow · 2019-02-01T14:51:42Z

@minchaow Thank you for the PR. We will review it soon.

Regarding test cases:
All tests for master and recent PR are passing in CI/CD pipeline and locally on my machine as well. So there shouldn't be any issue.

Anyways, looking at

Defrag Defragmentation [It] should defragment and reduce size of DB within time
/Workspace/go/src/github.com/gardener/etcd-backup-restore/pkg/etcdutil/defrag_test.go:53``

It looks like DB defragmentation ran on already defragmented etcd. By any chance, any other process calling defragmentation on etcd? I'm not able to reproduce it on master.
Just for safer side, try to run the test in docker container. You can use something like below:
docker run --rm -it \
    -v ${PWD}:/master-version \
   -e GOPATH="/go" \
   -w /master-version golang bash -c "make verify"
The implement needs 3rd part SDK, so I appended the dependency into [[constraint]] in the Gopkg.toml. When running "make revendor", not only aliyun SDK was added, but also some other dependencies were updated, which cause one problem as follow when to run "make verify":

This issue is similar to etcd-io/etcd#8715 . Might have been triggered due to later release of go-codec library 1.1.2.
Till upstream fixes this, do the following.
Please add following line to Gopkg.toml at the end and run make revendor again. Make sure that Gopkg.lock reflects the codec version in use as 1.1.1.
[[override]]
name = "github.com/ugorji/go"
revision = "b4c50a2b199d93b13dc15e78929cfb23bfdf21ab"

@swapnilgm Thank you very much. I will modify my code asap and run the UT cases again according to your suggestion.

Best regards!

minchaow · 2019-02-11T09:55:17Z

@swapnilgm , thanks for your suggestions very much. UT has passed when running inside the container, and the workaround for my second issue is workable as well.
Also, I have changed my code according to your review comments and added license information.

minchaow · 2019-02-12T12:19:25Z

@swapnilgm, this PR is important for enabling gardener on alicloud, so could you please help review the code, the sooner the better, many thanks for you help! :)

swapnilgm · 2019-02-12T12:39:18Z

@minchaow Thanks for addressing the changes. I can understand the importance of the PR. We will review it soon.

swapnilgm · 2019-02-14T05:26:53Z

@shreyas-s-rao Please review the PR.
@minchaow We don't have access to Alicloud, so we can just theoretically review the PR and can't test it with actual OSS. If required, Shreyas might get back to you over slack or mail to check the demo with actual deployment.

swapnilgm · 2019-02-14T05:32:08Z

Sorry my bad. Just received the access to AliCloud.

minchaow · 2019-02-14T10:53:32Z

Hi @shreyas-s-rao, code has changed according to suggestions, please help review again. Thanks a lot.

shreyas-s-rao · 2019-02-14T11:09:53Z

@minchaow After running make revendor, push the code along with all the new files that are added to the vendor directory, so that the OSS SDK gets added in-tree.

shreyas-s-rao

@minchaow Thank you very much for the well-written PR! I have reviewed your PR, and have suggested a few changes in your code. Please address them. Other than those, there are few changes you will have to make in the documentation and code comments to include information about "OSS" as a new snapstore provider:

README.md: line 118
README.md: Under Cloud Provider Credentials, add a line about the env variable details for OSS
.github/ISSUE_TEMPLATE/bug_report.md: line 26
pkg/snapstore/types.go: line 23
chart/values.yaml: line 26

Also, once you run make revendor, commit the changes along with the vendored files. It's fine if other files are added/modified in the vendor directory, as Godep has a method to resolve dependencies, and a few vendor files will get changed here and there. Not an issue.
Run make revendor in your normal dev env, so that github.com/gardener doesn't get added as a vendor package.

pkg/snapstore/oss_snapstore.go

pkg/snapstore/oss_snapstore_test.go

pkg/snapstore/snapstore_test.go

shreyas-s-rao

@minchaow Just a few more suggestions for changes. Kindly address them. Thanks.

pkg/snapstore/oss_snapstore_test.go

minchaow · 2019-02-15T12:14:36Z

@swapnilgm, @shreyas-s-rao, comments addressed, and changes pushed. Please help review. Thanks.

shreyas-s-rao

@minchaow Thank you for addressing my review comments and making the changes! I have just a couple of comments on the new code you added. Please address them. Thanks.

pkg/snapstore/oss_snapstore_test.go

minchaow · 2019-02-15T15:25:16Z

@shreyas-s-rao, comments addressed, and the commit pushed.

shreyas-s-rao

@minchaow Thanks for addressing my comments. I have just one last change. I caught it only now when running the tests multiple times, so I wasn't able to suggest this change in the previous reviews. Sorry for the trouble. 😅

pkg/snapstore/oss_snapstore_test.go

minchaow · 2019-02-15T16:01:09Z

@shreyas-s-rao, comments addressed. Thanks for your so careful review! 👍 😄

shreyas-s-rao

@minchaow Thanks so much for addressing all the comments. LGTM! 👍🏼

minchaow requested review from georgekuruvillak, shreyas-s-rao and swapnilgm as code owners February 1, 2019 10:21

swapnilgm suggested changes Feb 1, 2019

View reviewed changes

pkg/snapstore/oss_snapstore.go Outdated Show resolved Hide resolved

pkg/snapstore/oss_snapstore_test.go Outdated Show resolved Hide resolved

minchaow force-pushed the alicloud branch from 29d9e7c to 426b7e1 Compare February 11, 2019 09:46

swapnilgm removed the needs/changes Needs (more) changes label Feb 12, 2019

minchaow force-pushed the alicloud branch from 426b7e1 to 52ab51f Compare February 14, 2019 10:51

shreyas-s-rao requested changes Feb 15, 2019

View reviewed changes

pkg/snapstore/oss_snapstore.go Show resolved Hide resolved

pkg/snapstore/oss_snapstore_test.go Show resolved Hide resolved

pkg/snapstore/oss_snapstore_test.go Outdated Show resolved Hide resolved

pkg/snapstore/snapstore_test.go Outdated Show resolved Hide resolved

shreyas-s-rao requested changes Feb 15, 2019

View reviewed changes

shreyas-s-rao added needs/rebase Needs git rebase needs/changes Needs (more) changes and removed needs/review Needs review labels Feb 15, 2019

swapnilgm added this to the 0.5.0 milestone Feb 15, 2019

minchaow force-pushed the alicloud branch from 52ab51f to 74d9d08 Compare February 15, 2019 12:03

minchaow force-pushed the alicloud branch from 74d9d08 to 4cc2501 Compare February 15, 2019 12:11

shreyas-s-rao requested changes Feb 15, 2019

View reviewed changes

pkg/snapstore/oss_snapstore_test.go Outdated Show resolved Hide resolved

pkg/snapstore/oss_snapstore_test.go Outdated Show resolved Hide resolved

pkg/snapstore/oss_snapstore_test.go Outdated Show resolved Hide resolved

minchaow force-pushed the alicloud branch 2 times, most recently from 863bad7 to d0dfa85 Compare February 15, 2019 15:22

shreyas-s-rao requested changes Feb 15, 2019

View reviewed changes

pkg/snapstore/oss_snapstore_test.go Show resolved Hide resolved

Implement etcd-backup-restore on OSS of Alicloud

9bcd4e0

minchaow force-pushed the alicloud branch from d0dfa85 to 9bcd4e0 Compare February 15, 2019 15:57

shreyas-s-rao approved these changes Feb 15, 2019

View reviewed changes

gardener-robot-ci-1 added 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 Feb 15, 2019

shreyas-s-rao 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 needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 15, 2019

gardener-robot-ci-1 added 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 Feb 16, 2019

shreyas-s-rao merged commit 030a6c9 into gardener:master Feb 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement etcd-backup-restore on OSS of Alicloud #108

Implement etcd-backup-restore on OSS of Alicloud #108

minchaow commented Feb 1, 2019 •

edited by shreyas-s-rao

Loading

swapnilgm commented Feb 1, 2019

swapnilgm left a comment

minchaow commented Feb 1, 2019

minchaow commented Feb 11, 2019

minchaow commented Feb 12, 2019

swapnilgm commented Feb 12, 2019

swapnilgm commented Feb 14, 2019

swapnilgm commented Feb 14, 2019

minchaow commented Feb 14, 2019

shreyas-s-rao commented Feb 14, 2019

shreyas-s-rao left a comment

shreyas-s-rao left a comment

minchaow commented Feb 15, 2019

shreyas-s-rao left a comment

minchaow commented Feb 15, 2019

shreyas-s-rao left a comment

minchaow commented Feb 15, 2019

shreyas-s-rao left a comment

Implement etcd-backup-restore on OSS of Alicloud #108

Implement etcd-backup-restore on OSS of Alicloud #108

Conversation

minchaow commented Feb 1, 2019 • edited by shreyas-s-rao Loading

swapnilgm commented Feb 1, 2019

swapnilgm left a comment

Choose a reason for hiding this comment

minchaow commented Feb 1, 2019

minchaow commented Feb 11, 2019

minchaow commented Feb 12, 2019

swapnilgm commented Feb 12, 2019

swapnilgm commented Feb 14, 2019

swapnilgm commented Feb 14, 2019

minchaow commented Feb 14, 2019

shreyas-s-rao commented Feb 14, 2019

shreyas-s-rao left a comment

Choose a reason for hiding this comment

shreyas-s-rao left a comment

Choose a reason for hiding this comment

minchaow commented Feb 15, 2019

shreyas-s-rao left a comment

Choose a reason for hiding this comment

minchaow commented Feb 15, 2019

shreyas-s-rao left a comment

Choose a reason for hiding this comment

minchaow commented Feb 15, 2019

shreyas-s-rao left a comment

Choose a reason for hiding this comment

minchaow commented Feb 1, 2019 •

edited by shreyas-s-rao

Loading