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.

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

etcdctl: restore should create a snapshot #6370

Merged
merged 1 commit into from
Sep 8, 2016

Conversation

xiang90
Copy link
Contributor

@xiang90 xiang90 commented Sep 7, 2016

Restore should create a snapshot. So the new db file
can be sent to newly joined member.

/cc @hongchaodeng Can you please give this a try?

Restore should create a snasphot. So the new db file
can be sent to newly joined member.
@hongchaodeng
Copy link
Contributor

Fixes #6361

@xiang90
Copy link
Contributor Author

xiang90 commented Sep 7, 2016

@hongchaodeng I will write a few tests around this today + tomorrow.

@hongchaodeng
Copy link
Contributor

Not working..

@xiang90
Copy link
Contributor Author

xiang90 commented Sep 7, 2016

@hongchaodeng It works for me. At least for the example you provided in that issue. One thing I noticed is that you use etcdctl instead of ./etcdctl in your example. Probably you did not update etcdctl but still used old etcdctl in your GOPATH or BIN PATH?

@hongchaodeng
Copy link
Contributor

Correct. Works now.

@hongchaodeng
Copy link
Contributor

@xiang90
I will try to test it on etcd controller tomorrow.
Need to figure out docker image :)

@xiang90
Copy link
Contributor Author

xiang90 commented Sep 7, 2016

@gyuho it seems that there is no easy way to test snapshot restore with today's e2e testing infra. Can you find a way to test what #6361 does?

@gyuho
Copy link
Contributor

gyuho commented Sep 7, 2016

@xiang90 Sure, I will add e2e test to address #6361

@xiang90
Copy link
Contributor Author

xiang90 commented Sep 7, 2016

@gyuho It seems to me that we have a bad abstraction in e2e testing around etcd process. etcdProcess should understand all its configuration. However, now we construct all args in the cluster conf. And then we pass the agrs into etcd process. So now it is very hard for us to just start on etcd process instead of creating a cluster. We should make etcd process be able to construct the args.

@gyuho
Copy link
Contributor

gyuho commented Sep 7, 2016

@xiang90 args you mean flags to etcd binary, or args + flags to etcdctl?

@xiang90
Copy link
Contributor Author

xiang90 commented Sep 7, 2016

@gyuho

Now we create args here https://github.com/coreos/etcd/blob/master/e2e/etcd_test.go#L243-L304 in etcdProcessClusterConfig.

The created arg is here: https://github.com/coreos/etcd/blob/master/e2e/etcd_test.go#L130 which is totally obscure to etcd process.

However, etcd process SHOULD understand all flags given to it. It should not be obscure. Or we cannot easily create an etcd process by giving it config.

What we should do is to make etcd process understands ALL configuration. We should covert these configuration fields into args in etcd process, not etcdClusterConfiguration.

@xiang90
Copy link
Contributor Author

xiang90 commented Sep 7, 2016

If you try to test #6361, you will probably see the same issue.

@gyuho
Copy link
Contributor

gyuho commented Sep 7, 2016

Ok, I will see if we can make it more configurable.

@xiang90
Copy link
Contributor Author

xiang90 commented Sep 8, 2016

I manually tested this in a few cases. Will add test cases to e2e later on, since we need quite a few changes to e2e to make testing easier. I want to unblock @hongchaodeng's work on the controller.

@xiang90 xiang90 merged commit 28d80ad into etcd-io:master Sep 8, 2016
@xiang90 xiang90 deleted the fix_restore branch September 8, 2016 19:25
@gyuho gyuho mentioned this pull request Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants