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

sozuctl: deserialize the configuration correctly during upgrades #364

Closed
BlackYoup opened this issue Feb 23, 2018 · 5 comments
Closed

sozuctl: deserialize the configuration correctly during upgrades #364

BlackYoup opened this issue Feb 23, 2018 · 5 comments

Comments

@BlackYoup
Copy link
Collaborator

Before we start to upgrade the running sozu, sozuctl should check if the configuration file has the right format / options. It should use what has been done in #363

@BlackYoup
Copy link
Collaborator Author

For more context, I had troubles when upgrading sozu after the proxy protocol PR was merged.

To reproduce:

  • checkout commit aef54af
  • build sozu
  • start it with the default bin/config.toml file
  • checkout master again
  • build sozu and ctl
  • upgrade using bin/config.toml

The master should panic:

2018-03-04T23:50:18Z 67025359724785 2426 MASTER INFO    launching new master
2018-03-04T23:50:18Z 67025359862395 2426 MASTER INFO    master launched: 8019
thread 'main' panicked at 'could not parse upgrade data: ErrorImpl { code: Message("missing field `proxy_protocol`"), line: 1, column: 6392 }', /checkout/src/libcore/result.rs:916:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

It shouldn't have happened and sozuctl should have warned me about this by not upgrading and printing a nice message :)

@Geal
Copy link
Member

Geal commented Mar 5, 2018

this error is linked to the UpgradeData, a struct that's transmitted between the old master and the new. We should accept missing fields and replace them with some default values, but the issue here is that it comes from a field of UpgradeData which probably already has a deserialization routine used elsewhere.
Right now I think the most correct way would be to have UpgradeData fields be copies of those structures with optional fields that can be filled when deserializing from an older version.

@Geal
Copy link
Member

Geal commented Mar 9, 2018

For a bit more context, this is linked to #104
Unfortunately, versioning will need to be applied to the deserialization of everything: configuration messages, configuration file, state

@Geal
Copy link
Member

Geal commented Mar 14, 2018

so, I took a closer look. When some fields are removed, they will be silently ignored on upgrade. When some fields are added, deserialization of the upgrade data will fail in the new master process (because it does not see a field it expects).
For now, we can fix it easily by specifying default values for the new fields we add, as documented here: https://serde.rs/attr-default.html

I expect another upgrade issue between 0.5 and master, since we renamed instances to backends. But for the rest of the configuration, default fields should work properly

@Geal Geal changed the title sozuctl: check the configuration file before upgrading sozuctl: deserialize the configuration crrectly during upgrades Mar 14, 2018
@Geal Geal changed the title sozuctl: deserialize the configuration crrectly during upgrades sozuctl: deserialize the configuration correctly during upgrades Mar 14, 2018
@Geal
Copy link
Member

Geal commented Mar 16, 2018

The first example of using default values for deserialization was done in b07fd1f. I can confirm it works

@Geal Geal closed this as completed Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants