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

clickhouse-admin: Reject old configurations #7137

Open
andrewjstone opened this issue Nov 22, 2024 · 6 comments · May be fixed by #7347
Open

clickhouse-admin: Reject old configurations #7137

andrewjstone opened this issue Nov 22, 2024 · 6 comments · May be fixed by #7347
Assignees

Comments

@andrewjstone
Copy link
Contributor

Currently, everytime the reconfigurator executor pushes a configuration to clickhouse-admin-keeper or clickhouse-admin-server,
the XML configuration file is generated and reloaded by the keeper or server process respectively. This is somewhat aesthetically displeasing if we consistently rewrite the same configuration file, but we decided when closing #6909 that it's the more robust way to handle reconfiguration failures than any alternative.

However, for correctness purposes, we still want to reject older configurations from overwriting newer ones. This can wreak havoc on the clusters. While it's very unlikely to see this currently due to how we manage changes, it is still possible with N independent nexuses pushing down their own latest configuration.

To fix this, we should save the current configuration (with its generation number) in an auxiliary file, and keep this generation number in memory at the admin-server. We can then ignore any attempts to use an old configuration.

@karencfv
Copy link
Contributor

I think I'm overlooking something, but why do we need to save the entire current configuration and not just the generation number? Generation numbers are always assigned sequentially no?

@andrewjstone
Copy link
Contributor Author

I think I'm overlooking something, but why do we need to save the entire current configuration and not just the generation number? Generation numbers are always assigned sequentially no?

It's not strictly necessary. It just may be useful for debugging. Overhead is minimal.

@karencfv
Copy link
Contributor

Gotcha, yeah that makes sense

@karencfv
Copy link
Contributor

@andrewjstone I doubt you're working on this, but just as an FYI, I'm going to pick this issue up and work on it next :)

@andrewjstone
Copy link
Contributor Author

@andrewjstone I doubt you're working on this, but just as an FYI, I'm going to pick this issue up and work on it next :)

Awesome! Thanks @karencfv.

One thing, after thinking more about your note on saving the entire configuration, I think that it might cause more problems than it's worth. I remember coming up with some issues a month ago, but didn't write them down unfortunately.

I would stick with your suggestion of just saving the generation number :)

@karencfv
Copy link
Contributor

ah! thanks for the heads up @andrewjstone

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

Successfully merging a pull request may close this issue.

2 participants