User-config Watcher #3898

asymmetric · 2017-10-25T14:41:52Z

The User-config Watcher uses the FileWatcher to keep track of the user.toml file.

When a service is started, the UCW starts a worker thread that watches the file for changes, removal or creation. When either of those events happen, the worker notifies the Watcher via a channel.

The Manager can signal the Worker to stop running by using another channel.

Closes #2805.

thesentinels · 2017-10-25T14:42:00Z

Thanks for the pull request! Here is what will happen next:

Your PR will be reviewed by the maintainers
If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

asymmetric · 2017-10-25T15:38:43Z

Oh, seems like we have to fix a couple failing tests. On it.

krnowak · 2017-11-03T21:41:05Z

Updated, but is based on #3814, so this PR needs to wait until #3814 is merged.

asymmetric · 2017-11-06T10:27:23Z

components/sup/src/manager/user_config_watcher.rs

-    fn path(&self) -> &Path {
-        &self.pkg.user_config_path
+    fn path(&self) -> UserConfigPath {
+        self.cfg.user_config_path.clone()


Why clone here? Wouldn't it be better to let the caller do it, if they need to?

christophermaier · 2017-11-21T21:39:34Z

components/sup/src/manager/file_watcher.rs

+    ///
+    /// This will create an instance of `W` and start watching the
+    /// paths. When looping the file watcher, it will not emit any
+    /// initial "file appeared" event even the watched file existed


christophermaier · 2017-11-21T21:48:24Z

components/sup/src/manager/mod.rs

+        {
+            let mut services = self.services.write().expect("Services lock is poisoned!");
+            mem::swap(services.deref_mut(), &mut svcs);
+        }


How did this work before? I'm not quite following the rationale here.

(Thanks for correcting the spelling error with "poisoned", though 🎉 )

remove_service took &self before, rather than &mut self.

Now it takes &mut self because it calls self.user_config_watcher.remove(service), which stops the watcher thread for that service and removes it from the states HashMap. Therefore, we have a clash between the above, immutable borrow, and the mutable one below. Hence the block.

christophermaier · 2017-11-21T21:52:56Z

components/sup/src/manager/mod.rs

+            };
+
+            service = services.remove(services_idx);
+        }


I'm also not quite seeing the reason for these changes either... I assume it's somehow related to the other scope change above?

The explanation is basically the same as above. Obtaining the services variable involves taking a mutable ref on self. Also, remove_service requires mutable ref on self. So we needed to limit the scope of the services variable for mutable ref on self to go away before we could acquire another one by running the remove_service function.

christophermaier · 2017-11-21T22:00:43Z

components/sup/src/manager/service/mod.rs

+
+            if cfg_updated_from_rumors || census_ring.changed() {
+                let config_changed = cfg_updated_from_rumors || self.user_config_updated;
+                let needs_reload = self.compile_hooks(&ctx) || self.user_config_updated;


I think compiling the hooks is all you need here... if a change to a user config file resulted in the compiled output of the templates, then that should be the only thing you care about, right?

Couldn't a reload be necessary if e.g. a port was changed in the configuration of a DB?

That should be taken care of when we generate a render context and compile the hooks. If the change to the user config resulted in different configs being rendered to disk, then that would be accounted for with the earlier code.

Do you mean that the reconfigure hook would be run whenever the configuration in a user.toml has changed?
What if that hook isn't defined?

In general, I'm not sure I understand the connection between a change in the user.toml and hooks.
Couldn't there be an update to a user.toml file that doesn't produce any change in hooks?

user.toml is one layer of a service's configuration (

habitat/components/sup/src/manager/service/config.rs

Lines 79 to 80 in e2a946f

/// User level configuration loaded by a Service's `user.toml`

pub user: Option<toml::Value>,

)

This, in turn, is part of the RenderContext (

habitat/components/sup/src/templating/context.rs

Lines 56 to 63 in e2a946f

#[derive(Clone, Debug, Serialize)]

pub struct RenderContext<'a> {

pub sys: &'a Sys,

pub pkg: &'a Pkg,

pub cfg: &'a Cfg,

pub svc: Svc<'a>,

pub bind: Binds<'a>,

}

) that is created for templating both hooks and configuration.

When hooks and configuration are rendered according to the new data present in the RenderContext, we return a boolean that reflects whether any content of the templated files changed. If they do, then we reconfigure or reload as needed.

If data changed, but the output in the templated files remained the same, then there is no reason to restart. In the extreme case, imagine adding comments to your user.toml file. The toml file certainly changed, but it will have no effect on the hooks or configuration of the service, by definition.

christophermaier · 2017-11-21T22:03:29Z

components/sup/src/manager/service/mod.rs

+            } else if self.user_config_updated {
+                (true, true, self.compile_configuration(&ctx))
+            } else {
+                (false, self.needs_reload, self.needs_reconfiguration)


I believe that needs_reload and needs_reconfiguration are only set inside this function, so I think we can collapse this branch of the if/else, too.

I'm not sure I understand, but I'll bite - needs_reload and needs_reconfiguration are set to false also in reload and reconfigure functions, which are also gated on needs_reload and needs_reconfiguration being true. The (false, self.needs_reload, self.needs_reconfiguration) tuple here is to try to avoid changing value any of the members, so we don't accidentally set them from true to false because some config we don't care about has changed, but the reloading/reconfiguring didn't yet happen.

I haven't investigated if this can be the case, though.

See reply above... I don't think additional if/else comparisons should be needed if new user config is accounted for when we generate our template rendering context.

christophermaier · 2017-11-21T22:04:53Z

components/sup/src/manager/user_config_watcher.rs

+// requires a lot of ceremony, so we work around this with this trait.
+pub trait Serviceable {
+    fn name(&self) -> &str;
+    fn path(&self) -> &UserConfigPath;


Might be good to call this user_config_path instead of just path for clarity.

asymmetric · 2017-12-11T15:49:40Z

@cm we've tried to address your comments, could you PTALA?

asymmetric · 2017-12-20T14:59:32Z

@christophermaier I followed your advice and removed the redundant checks, which also results in a cleaner implementation. Let me know what you think.

christophermaier · 2017-12-21T15:41:29Z

components/sup/src/manager/mod.rs

+        if let Err(_) = self.user_config_watcher.remove(service) {
+            debug!(
+                "Error stopping user-config watcher thread for service {}",
+                service


logging the error would be nice

christophermaier · 2017-12-21T15:44:52Z

components/sup/src/manager/mod.rs

+
+        for service in services.iter_mut() {
+            if self.user_config_watcher.have_events_for(service) {
+                outputln!("Reloading service {}", &service.spec_ident);


This log line is a bit misleading, in that we're not reloading the service here; we're merely recognizing that the user config has changed and marking the service for possible restart (just because the config file changed doesn't necessarily mean that the change will have an effect on templated content).

Replaced the message with "User configuration of the service {} changed".

christophermaier · 2017-12-21T15:49:31Z

components/sup/src/manager/service/config.rs

+            &UserConfigPath::Deprecated(ref p) => p,
+        }
+    }
+}


Minor point, but it would be more idiomatic to implement AsRef<Path> here instead, particularly since that's how Cfg::load_user is implemented.

Done. Ampersands strike back, though. ;)

let user = Self::load_user(self.user_config_path.get_path())?;

is now

let user = Self::load_user(&self.user_config_path)?;

I was wondering if I also should implement the Deref trait, so we can get the &Path too for scenarios like user_config_path().join("foo"). Or is doing user_config_path().as_ref().join("foo") fine enough? For now I went with the second option (in user config watcher tests).

I think using as_ref() for now is fine; we can revisit later if necessary.

christophermaier · 2017-12-21T16:03:41Z

components/sup/src/manager/user_config_watcher.rs

+            // happened.
+            let (events_tx, events_rx) = sync_channel(1);
+            let (running_tx, running_rx) = channel();
+            let (watching_tx, watching_rx) = sync_channel(1);


Out of curiosity, why did you choose to go with channels with a buffer size of 1 to act as a boolean, rather than using an AtomicBool?

Uh, I guess that it has some Golang background, but I'll leave this question to @asymmetric - he wrote that code. ;)

@asymmetric any input on this one? ☝️

Sorry for the delay, I was on holiday.

Mostly the reason is that I find it easier to reason about concurrency problems using message passing, rather than shared data.

If I remember correctly, there were also issues around the fact that the AtomicBools (or the channels in the final implementation) are fields of the WorkerState struct, and borrowing one field for reading locked the whole struct, making it hard to modify the other field in the same context.

I think there are techniques that we could use to deal with the borrowing, and ultimately, I think something like an AtomicBool may be simpler and easier to reason about.

That being said, we can probably tackle that later. I don't think we need to hold this feature up any more.

krnowak · 2018-01-02T16:27:31Z

Review issues addressed, rebased to current master and resolved the conflict.

christophermaier · 2018-01-12T11:02:16Z

@krnowak In the future, I think it would be easier to review if you could just add new commits when addressing feedback, instead of folding the changes into the commits that are already there. Otherwise, a reviewer almost has to go over the entire PR again to look for what's different. When everything is good, you can go back and squash things as needed.

Other than that, I just had the remaining question about channel vs. AtomicBool... after that, I think we can get this merged in.

asymmetric · 2018-01-16T13:01:16Z

@cm addressed your comment, let me know if there's anything else.

christophermaier · 2018-01-17T14:15:21Z

@asymmetric @krnowak If you all could rebase this on current master, we can go ahead and get it merged in.

Now that there are multiple watchers in the Manager struct, it makes sense to make the SpecWatcher field more descriptive. Also, there will be yet another watcher coming soon to the struct, so group them together. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

It's used in several places, so a constant makes sense - it will be helpful in avoiding possible typos in strings that wouldn't be caught by a compiler. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

We will need to reload the user config everytime we notice that it has changed. To be able to do it, we need to remember which path for the user configuration was used when loading it for the first time. Also, we start differentiating the deprecated and recommended paths in the API, so user config watcher can easily tell if the path was deprecated or not. The deprecated paths will not be watched. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

Sometimes the initial event is problematic, because the initial existence of the file (or lack of it) was already handled. Getting the initial event could result in useless work being done again. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io> Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

The delay will be used by tests to make sure that they wait long enough for the watcher to be able to send events. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

The UserConfigWatcher watches each service's user.toml file. It reacts on file creation, deletion and change, triggering a service reload and configuration. If a service has hooks defined, those are executed, otherwise the default actions are performed instead. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

In this commit the UserConfigWatcher is hooked into the Manager. The Manager can now handle events from the watcher and trigger a reload of services it is supervising on configuration change. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

There is a new logging key for the user config watcher. And some semantics of the reconfigure hook have changed. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Some functions were undocumented, some of them had wrong docs or typos, some of them needed a small update in docs after the reload-on-configuration-change work. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

asymmetric · 2018-01-17T16:28:11Z

@cm done!

christophermaier · 2018-01-17T16:30:33Z

@thesentinels approve

thesentinels · 2018-01-17T16:30:34Z

🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests.

thesentinels · 2018-01-17T16:31:08Z

Travis CI has started testing this PR.

thesentinels · 2018-01-17T16:55:40Z

💖 Travis CI reports this PR passed.

It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now.

I just want you and the contributor to answer me one question:

asymmetric requested review from reset, krnowak and fnichol October 25, 2017 14:41

asymmetric requested review from christophermaier, cnunciato and mgamini as code owners October 25, 2017 14:41

asymmetric mentioned this pull request Oct 25, 2017

User config watcher kinvolk-archives/habitat#3

Closed

krnowak force-pushed the asymmetric/user-toml-watcher branch from a416de0 to 35cad45 Compare November 3, 2017 21:36

asymmetric commented Nov 6, 2017

View reviewed changes

krnowak force-pushed the asymmetric/user-toml-watcher branch 2 times, most recently from 2311fdf to 6c5b5e5 Compare November 7, 2017 15:16

christophermaier reviewed Nov 21, 2017

View reviewed changes

asymmetric mentioned this pull request Nov 27, 2017

Running a Service Group in Kubernetes #2804

Closed

5 tasks

asymmetric force-pushed the asymmetric/user-toml-watcher branch from 6c5b5e5 to c651102 Compare December 11, 2017 15:48

asymmetric force-pushed the asymmetric/user-toml-watcher branch 3 times, most recently from ff74690 to bad8973 Compare December 11, 2017 18:57

asymmetric force-pushed the asymmetric/user-toml-watcher branch from bad8973 to 23a68ee Compare December 20, 2017 14:46

christophermaier reviewed Dec 21, 2017

View reviewed changes

krnowak force-pushed the asymmetric/user-toml-watcher branch from 23a68ee to 09ac22b Compare January 2, 2018 16:26

krnowak force-pushed the asymmetric/user-toml-watcher branch from 09ac22b to f5c3856 Compare January 3, 2018 12:17

Lorenzo Manacorda and others added 9 commits January 17, 2018 16:22

Use a constant for user.toml

4bf55e0

It's used in several places, so a constant makes sense - it will be helpful in avoiding possible typos in strings that wouldn't be caught by a compiler. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Expose the watch delay FileWatcher is using.

86fa371

The delay will be used by tests to make sure that they wait long enough for the watcher to be able to send events. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

Use UserConfigWatcher in Manager

a02e3a0

In this commit the UserConfigWatcher is hooked into the Manager. The Manager can now handle events from the watcher and trigger a reload of services it is supervising on configuration change. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Update website documentation to reflect changes in this PR

b4a81f9

There is a new logging key for the user config watcher. And some semantics of the reconfigure hook have changed. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Add or update some API docs

fcdec28

Some functions were undocumented, some of them had wrong docs or typos, some of them needed a small update in docs after the reload-on-configuration-change work. Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

asymmetric force-pushed the asymmetric/user-toml-watcher branch from f5c3856 to fcdec28 Compare January 17, 2018 15:23

thesentinels merged commit e7aa623 into habitat-sh:master Jan 17, 2018

	/// User level configuration loaded by a Service's `user.toml`
	pub user: Option<toml::Value>,

	#[derive(Clone, Debug, Serialize)]
	pub struct RenderContext<'a> {
	pub sys: &'a Sys,
	pub pkg: &'a Pkg,
	pub cfg: &'a Cfg,
	pub svc: Svc<'a>,
	pub bind: Binds<'a>,
	}

User-config Watcher #3898

User-config Watcher #3898

Conversation

asymmetric commented Oct 25, 2017 • edited Loading

thesentinels commented Oct 25, 2017

asymmetric commented Oct 25, 2017

krnowak commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asymmetric Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asymmetric commented Dec 11, 2017

asymmetric commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asymmetric Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak commented Jan 2, 2018

christophermaier commented Jan 12, 2018

asymmetric commented Jan 16, 2018

christophermaier commented Jan 17, 2018

asymmetric commented Jan 17, 2018

christophermaier commented Jan 17, 2018

thesentinels commented Jan 17, 2018

thesentinels commented Jan 17, 2018

thesentinels commented Jan 17, 2018

asymmetric commented Oct 25, 2017 •

edited

Loading

asymmetric Nov 29, 2017 •

edited

Loading

asymmetric Jan 16, 2018 •

edited

Loading