Improve restart commands by passing changed settings #1480
Conversation
|
This force push addresses the unit test in |
|
^Addressed comments by Matt and refactored some function names to be more clear. |
|
With your changes you addressed this part of #1389:
But @etungsten mentioned:
Should that use case be handled as well in your PR? If so, you need to change |
Ah, that's correct. There needs to be a conditional inside |
Okay, I was going to do it, but after our discussion offline, I thought we are supposed to handle only the |
|
Above force push addresses comments from @webern, @etungsten and @arnaldo2792
Will push the changes for checking if the host-container is enabled in new commit |
|
Above force push shortens the commit message |
I think we should do that in a separate PR. This PR isn't changing behavior that much, just fixing a bug where we impact unrelated host containers. Whether impacted host containers are restarted is a separate problem with different implications and testing requirements. |
|
Addressed @tjkirch's comments |
| if services.is_empty() { | ||
|
|
||
| let service_settings_map = | ||
| service::get_affected_service_setting_map(&args.socket_path, changed_settings) |
There was a problem hiding this comment.
I think my confusion mainly comes from the different ways we're storing settings and services, since some are duplicated, and what "services" actually means in different places. Before, services was a model::Services object, but now we also have maps with just their names so we can find related settings.
Let me throw out an idea, because it might help make my confusion clearer, and at least give us something to start with. What if we get rid of the change on this line, and change get_affected_services to return a new type that has all of that information together. The new type, perhaps just called Services, would effectively be a map where the keys are service names, just like service_settings_map and services are now, and the values would be another new type (Service?) with one field for the relevant setting names and one field for the Service objects used in the model. Then we'd have everything we need, and we can just pass that around instead of needing two different representations of services. It'd be easy to convert for places where we need a model::Services or just the map of services to settings, or we could change those places to understand the new type instead of converting at all.
I think this could simplify quite a bit because we wouldn't have as many conversions between different representations, there'd be fewer maps, fewer naming issues, etc. I can work on a prototype if it'd help clarify what I mean.
There was a problem hiding this comment.
I agree using a type here could help a lot. The type could also have a member function for returning a unique HashSet of whatever it is we need that for.
There was a problem hiding this comment.
Ok, I did a prototype of the Services thing I mentioned. (prototype because it's untested, commits aren't split nicely, etc.) @webern and @sanu11, what do you think, does this make the flow clearer?
There was a problem hiding this comment.
Thanks for putting it up @tjkirch : ) Will go through it
There was a problem hiding this comment.
pub fn get_config_file_names(services: &Services)could be a member function inimpl Services.- creating a member function for
services.0.is_empty()improves readability slightly.
In general in both the original and @sanu11 's PR, I found the propagation of an Option to reflect all services very confusing. I suggested the change where @sanu11 introduced:
service::get_affected_servicesservice::get_all_services
I think hiding this implementation detail helps a bit instead of having mysterious options in async fn run().
There was a problem hiding this comment.
I think there are too many functions and it's too hard to trace. If the Option is what's confusing, maybe we could make our own enum..? get_services(Services::All), get_services(Services::AffectedBySettings(list)), would that help?
|
Addressed @tjkirch's comments except the prototype
PS: I have removed the testcase since the function is removed, would add it later based on the prototype changes |
|
Above push addresses
I have tested the changes for the usecases mentioned in the PR description. I can update the description to contain the new logs once this change is approved/reviewed |
There was a problem hiding this comment.
Looking good to me, though I want to make sure @webern and @etungsten are happy with the ergonomics based on the open comments above.
|
^ Above push addresses @tjkirch's comments
Made changes to the PR description by adding recent logs with changed info statements |
|
Above force push addresses
Changed the PR description for tests according to the changed logs |
There was a problem hiding this comment.
Looks like what's remaining is:
- The member functions suggested in Improve restart commands by passing changed settings #1480 (comment), and possibly a change to
get_affected_services, depending on how @webern is feeling in that thread now - The confusing argument shadowing mentioned in Improve restart commands by passing changed settings #1480 (comment) (sorry)
There was a problem hiding this comment.
My approval is with this one request outstanding (the shadowed variable name). Thanks for all the work on this @sanu11!
There was a problem hiding this comment.
My approval is with this one request outstanding (the shadowed variable name). Thanks for all the work on this @sanu11!
(with actual Approve radio button this time, I'm getting bad at using this correctly)
sure, I will change it. Thanks! 😄 |
If a service is affected by changed settings, the changed settings are passed to the restart commands of that service in an environment variable, separated by spaces. The variable is only set if there are changed settings that affect the service, and only the settings relevant to that service are included. This commit adds a Services wrapper which is struct containing a map of service name(String) to Service where Service has one field for changed_settings and another for model::Service. This was added to remove the complications of different service representations that were necessary for this change.
If no settings have changed, say during startup, handle all host containers. If settings are changed for particular host containers, handle only those host containers. If the settings are not host-container-specific, but affect host containers overall, handle all host containers.
Issue number:
#1389
Description of changes:
Pass a space separated string of changed settings to the restart command
Here, I have added a function to return a map of
service, list of changed settingsso that we get the list of changed settings for a particular service and that list can be passed as a env to the restart command fo that service. Here I have usedenvbecause some commands aresystemctlbased and passing them as arguments to them wouldn't be right.Handle the changed settings for host-containers. Handle only the host-containers whose setting is changed.
Testing done:
settings.host-containersbut affects the host-containers, all the host-containers are handledTerms of contributiaon:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.