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

Do not recover db snapshot if index is 0 #9481

Closed
wants to merge 1 commit into from

Conversation

jlhawn
Copy link

@jlhawn jlhawn commented Mar 22, 2018

... as that would indicate that the backend is a newly initialized
store rather than an outdated snapshot.

Fixes #9480

cc @gyuho

... as that would indicate that the backend is newly initialized
store rather than an outdated snapshot.

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
@gyuho gyuho self-assigned this Mar 22, 2018
@jlhawn
Copy link
Author

jlhawn commented Mar 22, 2018

@gyuho I've confirmed that this fixes the issue I was seeing but I'm not 100% certain that it's safe.

@jlhawn
Copy link
Author

jlhawn commented Mar 22, 2018

This code block suggests to me that it may be safe (it logs a warn message instead of returning an error). There's also that TODO comment about removing the check when it no longer supports upgrades from pre-3.0 releases.

@gyuho
Copy link
Contributor

gyuho commented Mar 23, 2018

This is unsafe. v3 should still panic on accidental db file deletion. I will add an optional flag to allow boot from empty db file, in case of upgrades from v2.

@gyuho
Copy link
Contributor

gyuho commented Mar 23, 2018

when it no longer supports upgrades from pre-3.0 releases.

We still support upgrades from pre-3.0. We might panic in that TODO in v4 (where v2 support is completely dropped).

@gyuho
Copy link
Contributor

gyuho commented Mar 23, 2018

This code block suggests to me that it may be safe

That code block only applies when db file initially existed.

@jlhawn
Copy link
Author

jlhawn commented Mar 23, 2018

That code block only applies when db file initially existed.

The member/snap/db file does already exist. In my repro in #9480 (comment) it exists from the time you start etcd v3.0 but you don't hit the crash until v3.2 starts up.

@gyuho
Copy link
Contributor

gyuho commented Mar 23, 2018

member/snap/db file does already exist

Then, that file should have been created from v3.0 migrate. Since we only support v3.2 and v3.3, we will provide a way to override that in v3.2. It would be like:

  • manually delete db file (given that no v3 data is ever written)
  • upgrade to v3.0
  • manually delete db file (given that no v3 data is ever written)
  • upgrade to v3.1
  • manually delete db file (given that no v3 data is ever written)
  • upgrade to v3.2 with --unsafe-overwrite-db

@jlhawn
Copy link
Author

jlhawn commented Mar 23, 2018

@gyuho if that is the suggested upgrade process then I wonder if it would just be okay to upgrade directly from v2.3 to v3.2 (with the understanding that this would not be a zero-downtime upgrade) using the --unsafe-overwrite-db flag? That way we could avoid the awkward steps of deleting the db file on those other versions?

Also, what would be the recommended next step? I would guess that you would want to start the server using --unsafe-overwrite-db and then restart the sever ASAP without that flag?

@gyuho
Copy link
Contributor

gyuho commented Mar 23, 2018

@jlhawn Yeah... just upgrade directly to v3.2 should be easier (as long as there will be no v3 data). Sorry for confusing steps. I will make sure to document this process.

@gyuho
Copy link
Contributor

gyuho commented Mar 23, 2018

Also, what would be the recommended next step? I would guess that you would want to start the server using --unsafe-overwrite-db and then restart the sever ASAP without that flag?

I will document the steps in #9484.

@jlhawn
Copy link
Author

jlhawn commented Mar 23, 2018

I will document the steps in #9484.

Thanks! I'll ask more questions there if I have any.

@jlhawn jlhawn closed this Mar 23, 2018
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.

None yet

2 participants