Add an alternative path for user.toml #3814

krnowak · 2017-10-18T08:43:04Z

This commit makes the supervisor to look for user configuration in two
locations: /hab/svc/<pkgname>/user.toml and
/hab/user/<pkgname>/config/user.toml. If the first path is missing, it
will try to load the configuration from the second one.

The reason is to work around an issue in Kubernetes/Docker/Linux
kernel.

In the Habitat operator we use the Kubernetes Secret feature for
putting user configuration as an initial configuration and as a source
of configuration updates. Initially it worked fine, until we
discovered that Kubernetes actually mounts a directory at
/hab/svc/<pkgname> to put user.toml there, so all the previous
contents there were effectively hidden.

We worked it around with a SubPath feature of Kubernetes, which
basically instead of mounting a directory at /hab/svc/<pkgname>,
bind-mounts some file on the host to the /hab/svc/<pkgname>/user.toml
path inside the Kubernetes pod.

This unfortunately breaks updating the configuration via the
Kubernetes Secret. This is because Kubernetes tries hard to make the
change of the Secret atomic. The atomicity of the change is achieved
with renaming. Doing the rename changes the inode of the target file
and since the bind mounts are based on inodes, the change is not
reflected in the pod.

It may take some time before we come up with a solution for this
problem and the solution to be released, so for now we would like to
add the second path for the user configuration. The new
/hab/user/<pkgname>/config directory can only contain user.toml file,
so it is safe to mount a directory here with our own copy of the file,
and we can stop using SubPath feature, so the updates should work
again.

Signed-off-by: Krzesimir Nowak krzesimir@kinvolk.io

Also, there seem to be an agreement that this file should be moved out of the /hab/svc directory, so users do not need to mess with the service specific files there.

thesentinels · 2017-10-18T08:43:06Z

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!

krnowak · 2017-10-18T08:47:13Z

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

@@ -101,6 +101,8 @@ pub struct Pkg {
    pub svc_run: PathBuf,
    pub svc_user: String,
    pub svc_group: String,
+    pub user_path: PathBuf,


I'm not sure if adding new fields here create some serialization problem. Like during the update of supervisor - we serialize this struct, store it on the disk, update the supervisor, deserialize the struct from the disk and we get empty fields for user_path and user_config_path. Is this something to be defensive about (by detecting if the fields are empty, for instance)?

Hrm... that's a good question. I wonder if these really need to be public members of the Pkg struct at all; maybe they should just stay function calls (which you use with your PackageForConfig trait). You could just use the fs::user_path(&package.ident.name), etc. implementations directly in your trait implementation for Pkg, get the exact same behavior, and avoid any serialization issues.

krnowak · 2017-10-18T08:49:02Z

CC: @fnichol, @christophermaier, @lilic, @asymmetric

We talked about it on the last meeting.

krnowak · 2017-10-23T13:08:50Z

Some food for thought:

Should we prefer the new path over the old one?
- If so, then it means that if both /hab/svc/<pkgname>/user.toml and /hab/user/<pkgname>/config/user.toml files exist, we read the latter, not the former, like the patch currently does.
Should the path preference be persistent over the service's lifetime?
- This is to clarify whether Supervisor should read the less preferred path after the more preferred path was removed sometime during the service's lifetime (so the service already got some config from the file in the more preferred location) or it should just think that there is no user configuration at all.
Should we deprecate the /hab/svc/<pkgname>/user.toml path?
- If so, we probably should print a warning about the deprecated path, when reading config from there.
When implementing the user.toml watcher, should we watch for both old and new paths?
- Doing this may be a bit complicated.
- Since this new path and the user.toml watcher are new features, maybe only watching the new path would be a good option.

asymmetric · 2017-10-23T13:35:20Z

I personally think that the user-config watcher should only watch the new path (/hab/user/foo/config/user.toml).

The reason is that the user-config watcher is a new feature, so this won't break anybody's workflow.

I also think the old location should be deprecated with a warning, and removed in future releases. The user.toml (or its location) hasn't been widely documented yet, so I think now is the right time.

christophermaier · 2017-11-01T17:44:11Z

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

+        }
+    }
+
+    fn steal_path(self) -> PathBuf {


What is the intention behind "stealing" the path? Other Rust APIs conventionally use "take" to denote removal of the data from its current owning context (with replacement by some default, e.g., None for an Option type).

Perhaps implementing From<UserConfigPath> for PathBuf would be more clear?

Ah, the From<UserConfigPath> looks like the way to go, thanks.

christophermaier · 2017-11-02T01:03:04Z

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

+    }
+}
+
+pub trait PackageForConfig {


I'm not sure the name of this trait accurately captures the intention behind the trait; what does PackageForConfig mean?

I'll try to come up with some better name for it. And some docs. Probably will take me more time than addressing the rest of the issues here. ;)

This trait was meant to be basically a subset of the Pkg struct, so I could mock it in tests.

christophermaier · 2017-11-02T01:07:19Z

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

+                            "user configuration location",
+                            ucp.get_path().display(),
+                            recommended_path.display(),
+                        );


This format string has three placeholders, but only two values are supplied.

It has three values, first is the "user configuration location" string. I did it that way, otherwise the line was over 100 chars long and rustfmt was bailing out… I'll split it out to a separate function so I'll have less indentation and more space for the string.

Ah, you're right... I totally missed that.

You can also split long strings up, like we do here:

habitat/components/sup/src/main.rs

Lines 160 to 162 in cc079b6

(about: "Load a service to be started and supervised by Habitat from a package or \

artifact. Services started in this manner will persist through Supervisor \

restarts.")

Ah, nice. That's exactly what I need. Thanks!

christophermaier · 2017-11-02T01:28:17Z

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

-        cfg.load_user(&package)?;
-        cfg.load_environment(&package)?;
+        cfg.load_user(package)?;
+        cfg.load_environment(package)?;


Ah, thanks for cleaning up those unnecessary &s 👍

christophermaier · 2017-11-02T01:36:54Z

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

+                }
+            }
+        }
+        // No user config found at all. Next time try loading only the


When is "next time"? As far as I can see, this is only ever called when creating a new Cfg struct.

I presume this is laying the groundwork for refreshing the user.toml content based on file watch events... in that case, is the intention to never change which user.toml path is being looked at? If this starts up and a user.toml in the new location is not present, but one at the old path is present, won't this always look at the old path, even if a new one shows up later? If that's the case, it may simplify this code and make it easier to reason about if we split this function into two functions: one that does the initial determination of which path we're going to look at, and the other that does the actual consumption of content. Actually, in that case, we could probably just use the original code after we figured out which path to consult.

Yeah, we basically want to stick to a single path, which is determined at the time when we load the config for the first time.

I'll have a look at splitting the functions. I also had an idea of cleaning up the load_{default,user,environment} functions, so they don't take the self parameter. This would be to avoid using Cfg::default, which forces the user_config_parameter to be an Option, because it's value is set sometime later in the new function.

christophermaier · 2017-11-02T01:57:59Z

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

@@ -101,6 +101,8 @@ pub struct Pkg {
    pub svc_run: PathBuf,
    pub svc_user: String,
    pub svc_group: String,
+    pub user_path: PathBuf,


Hrm... that's a good question. I wonder if these really need to be public members of the Pkg struct at all; maybe they should just stay function calls (which you use with your PackageForConfig trait). You could just use the fs::user_path(&package.ident.name), etc. implementations directly in your trait implementation for Pkg, get the exact same behavior, and avoid any serialization issues.

krnowak · 2017-11-03T13:17:08Z

Updated the PR. I dropped the user config path persistence for now, I'll make it a part of the follow up PR with the user config watching functionality. But I have refactored Cfg a bit, so it does not use Cfg::default() in the constructor.

christophermaier

Looks good 👍

I left some feedback on ways to simplify some of the test code, but beyond that I like the changes you've made. We should be able to merge this very soon.

christophermaier · 2017-11-03T13:43:50Z

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

+            gossip: None,
+            environment: environment,
+            gossip_incarnation: 0,
+        });


I like how this ended up!

christophermaier · 2017-11-03T14:03:03Z

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

+            file.read_to_string(&mut buf).expect(
+                "read value from toml file",
+            );
+            assert_eq!(buf, self.text);


I think this implementation is more complex than it needs to be for these tests; you probably just need a write_all and a flush. The assert_eq! is basically testing that Rust's I/O code is working.

Beyond that, though, I think there's a larger opportunity to simplify this test code. There's a lot of repetition in the test cases themselves around opening a file, creating a representation of the desired TOML, and then writing it. In the end, you just want one function that takes a path and some content, and writes that data to the right place. That one function could hide all the details of file opening, formatting, and writing, which are really incidental to the tests themselves. When you're reading the tests, you just want one line that says "OK, I'm creating this config file at this location"; you don't want to worry about the details of how that's done.

I added this assert_eq, because earlier I had a reset function that cleared a file and wrote new contents there. So I truncated it and then wrote new contents only to find out that I got a bunch of zeros at the beginning of the file, because truncating is not seeking.

Anyway, I'll have a look how to make it simpler.

In some follow up commit I'd like to add another field, that does not have an obvious default value. Either I could work it around by putting it inside Option (and then unwrap() the field with assumption that after Cfg::new() finishes, it will never be None) or just stop using Cfg::default(). I went with the latter. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

This commit makes the supervisor to look for user configuration in two locations: /hab/svc/<pkgname>/user.toml and /hab/user/<pkgname>/config/user.toml. If the first path is missing, it will try to load the configuration from the second one. The reason is to work around an issue in Kubernetes/Docker/Linux kernel. In the Habitat operator we use the Kubernetes Secret feature for putting user configuration as an initial configuration and as a source of configuration updates. Initially it worked fine, until we discovered that Kubernetes actually mounts a directory at /hab/svc/<pkgname> to put user.toml there, so all the previous contents there were effectively hidden. We worked it around with a SubPath feature of Kubernetes, which basically instead of mounting a directory at /hab/svc/<pkgname>, bind-mounts some file on the host to the /hab/svc/<pkgname>/user.toml path inside the Kubernetes pod. This unfortunately breaks updating the configuration via the Kubernetes Secret. This is because Kubernetes tries hard to make the change of the Secret atomic. The atomicity of the change is achieved with renaming. Doing the rename changes the inode of the target file and since the bind mounts are based on inodes, the change is not reflected in the pod. It may take some time before we come up with a solution for this problem and the solution to be released, so for now we would like to add the second path for the user configuration. The new /hab/user/<pkgname>/config directory can only contain user.toml file, so it is safe to mount a directory here with our own copy of the file, and we can stop using SubPath feature, so the updates should work again. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

Introduce the PackageConfigPaths trait, which will allow us to override the paths where the user configuration should be put for testing purposes. Before that, the paths were basically hardcoded to something under `/hab`. There is an environment variable (`TESTING_FS_ROOT`) for changing the filesystem root, where `hab` directory is being placed, but it is not really usable from unit tests. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

krnowak · 2017-11-03T20:08:48Z

Updated. The tests are now indeed simpler.

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

Setting up the Package struct in this test was necessary just to have things to compile, otherwise nothing from the struct was used at all. With the TestPkg struct in place, we don't need to do the setup anymore. Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

christophermaier · 2017-11-06T18:19:01Z

@thesentinels approve

thesentinels · 2017-11-06T18:19:03Z

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

thesentinels · 2017-11-06T18:26:11Z

Travis CI has started testing this PR.

thesentinels · 2017-11-06T18:54:27Z

💖 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:

The path was changed in habitat-sh/habitat#3814 Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

krnowak requested review from christophermaier, fnichol and reset as code owners October 18, 2017 08:43

krnowak commented Oct 18, 2017

View reviewed changes

krnowak force-pushed the krnowak/custom-user-toml-path branch 2 times, most recently from 18e2baf to 0d48872 Compare November 1, 2017 10:55

christophermaier reviewed Nov 2, 2017

View reviewed changes

krnowak force-pushed the krnowak/custom-user-toml-path branch from 0d48872 to b41f281 Compare November 3, 2017 13:14

christophermaier reviewed Nov 3, 2017

View reviewed changes

krnowak added 3 commits November 3, 2017 21:00

krnowak force-pushed the krnowak/custom-user-toml-path branch from b41f281 to 2e914c9 Compare November 3, 2017 20:07

krnowak added 2 commits November 3, 2017 22:09

Add tests for loading user configuration

bb7446a

Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>

krnowak force-pushed the krnowak/custom-user-toml-path branch from 2e914c9 to 50f3aad Compare November 3, 2017 21:10

krnowak mentioned this pull request Nov 3, 2017

User-config Watcher #3898

Merged

thesentinels merged commit e315621 into habitat-sh:master Nov 6, 2017

asymmetric pushed a commit to habitat-sh/habitat-operator that referenced this pull request Jan 30, 2018

Update mount path for user.toml

a288230

The path was changed in habitat-sh/habitat#3814 Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

asymmetric pushed a commit to habitat-sh/habitat-operator that referenced this pull request Jan 30, 2018

Update mount path for user.toml

70cc1bf

The path was changed in habitat-sh/habitat#3814 Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

asymmetric mentioned this pull request Jan 30, 2018

Update user.toml path and example images habitat-sh/habitat-operator#172

Merged

asymmetric pushed a commit to habitat-sh/habitat-operator that referenced this pull request Jan 31, 2018

Update mount path for user.toml

8023360

The path was changed in habitat-sh/habitat#3814 Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

asymmetric pushed a commit to habitat-sh/habitat-operator that referenced this pull request Jan 31, 2018

Update mount path for user.toml

9d2293e

The path was changed in habitat-sh/habitat#3814 Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

krnowak mentioned this pull request Feb 13, 2018

Release 0.5.0 habitat-sh/habitat-operator#179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an alternative path for user.toml #3814

Add an alternative path for user.toml #3814

krnowak commented Oct 18, 2017 •

edited

Loading

thesentinels commented Oct 18, 2017

krnowak Oct 18, 2017

christophermaier Nov 2, 2017

krnowak commented Oct 18, 2017

krnowak commented Oct 23, 2017

asymmetric commented Oct 23, 2017

christophermaier Nov 1, 2017

krnowak Nov 2, 2017

christophermaier Nov 2, 2017

krnowak Nov 2, 2017

christophermaier Nov 2, 2017

krnowak Nov 2, 2017

christophermaier Nov 2, 2017

krnowak Nov 2, 2017

christophermaier Nov 2, 2017

christophermaier Nov 2, 2017

krnowak Nov 2, 2017

christophermaier Nov 2, 2017

krnowak commented Nov 3, 2017

christophermaier left a comment

christophermaier Nov 3, 2017

christophermaier Nov 3, 2017

krnowak Nov 3, 2017

krnowak commented Nov 3, 2017

christophermaier commented Nov 6, 2017

thesentinels commented Nov 6, 2017

thesentinels commented Nov 6, 2017

thesentinels commented Nov 6, 2017

	(about: "Load a service to be started and supervised by Habitat from a package or \
	artifact. Services started in this manner will persist through Supervisor \
	restarts.")

Add an alternative path for user.toml #3814

Add an alternative path for user.toml #3814

Conversation

krnowak commented Oct 18, 2017 • edited Loading

thesentinels commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak commented Oct 18, 2017

krnowak commented Oct 23, 2017

asymmetric commented Oct 23, 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

Choose a reason for hiding this comment

krnowak commented Nov 3, 2017

christophermaier left a 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

krnowak commented Nov 3, 2017

christophermaier commented Nov 6, 2017

thesentinels commented Nov 6, 2017

thesentinels commented Nov 6, 2017

thesentinels commented Nov 6, 2017

krnowak commented Oct 18, 2017 •

edited

Loading