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

agent/consul: make Client/Server config reloading more obvious #8696

Merged
merged 2 commits into from
Jan 14, 2021

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Sep 16, 2020

Previously it was not clear which fields were being reloaded. They were hidden in multiple function calls and large structs. With this change the list of reloadable fields is explicit.

I believe this commit also fixes a bug. Previously RPCMaxConnsPerClient was not being re-read from the RuntimeConfig, so passing it to Server.ReloadConfig was never changing the value.

Also improve the test runtime by not doing a lot of unnecessary work, and fixed the naming of one of the config fields so that it is consistent across structs.

@dnephin dnephin added theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization theme/config Relating to Consul Agent configuration, including reloading backport/1.8 labels Sep 16, 2020
@dnephin dnephin mentioned this pull request Sep 16, 2020
15 tasks
@dnephin dnephin requested a review from a team September 18, 2020 17:59
@dnephin dnephin force-pushed the dnephin/fix-load-limits branch 2 times, most recently from 8c1189d to d2afe6d Compare September 28, 2020 19:20
Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I believe this commit also fixes a bug. Previously RPCMaxConnsPerClient was not being re-read from the RuntimeConfig, so passing it to Server.ReloadConfig was never changing the value.

Also improve the test runtime by not doing a lot of unnecessary work.
so that the field name is consistent across config structs.
@dnephin
Copy link
Contributor Author

dnephin commented Jan 14, 2021

Added a changelog entry for the bug fix, and confirmed the test failures are flakes that pass locally.

@dnephin dnephin merged commit cf4fa18 into master Jan 14, 2021
@dnephin dnephin deleted the dnephin/fix-load-limits branch January 14, 2021 22:40
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/310796.

@hashicorp-ci
Copy link
Contributor

🍒❌ Cherry pick of commit cf4fa18 onto release/1.9.x failed! Build Log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/config Relating to Consul Agent configuration, including reloading theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants