moving some common functionality from habitat #90

mwrock · 2018-11-17T01:53:34Z

This PR accompanies habitat-sh/habitat#5866 which moves most of the hook and templating code from the supervisor into the common crate of the habitat repo. There was some functionality that also needed to be used commonly but seemed better suited here:

sup/src/fs.rs -> folded into /core/src/fs.rs
sup/src/sys/abilities -> folded into /core/src/os/users
sup/src/sys/user -> folded into /core/src/os/users
sup/src/manager/service/dir.rs -> folded into /core/src/fs.rs

Signed-off-by: mwrock matt@mattwrock.com

thesentinels · 2018-11-17T01:53:35Z

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!

mwrock · 2018-11-26T22:10:34Z

related: habitat-sh/habitat#5161

baumanj · 2018-11-27T17:32:25Z

Starting to review this now

baumanj · 2018-11-27T17:40:59Z

components/core/src/error.rs

@@ -168,11 +179,17 @@ pub enum Error {
    WrongActivePackageTarget(package::PackageTarget, package::PackageTarget),
 }

+unsafe impl Send for Error {}
+unsafe impl Sync for Error {}


I'm curious why these are necessary/appropriate. According to the docs, Send and Sync are "automatically implemented when the compiler determines it's appropriate".

It was the only way I could get of a compile error compiling the supervisor against this. Something releted to not implementing send. I don't remember it well. I'll dig it up.

heh...go figure...I can't repro that error now. I have a hunch that is due to some changes I made yesterday in the supervisor.

The issue here is the addition of handlebars::error::RenderError to the core Error enum. The version we're at fails to require Sync in one of its fields. See sunng87/handlebars-rust#194.

baumanj

Just a couple small things. Overall, I'm really happy to see this made more modular.

baumanj · 2018-11-27T18:08:00Z

components/core/src/templating/hooks.rs

+            config_from
+                .and_then(|p| Some(p.as_path()))
+                .unwrap_or(&package.installed_path)
+                .join("hooks"),


I think this can be simplified to:

config_from.unwrap_or(&package.installed_path).join("hooks"),

Also, the only caller I see for this function passes None for config_from, so it seems like we could eliminate that argument entirely and just pass

package.installed_path.join("hooks"),

to Self::load. If that argument is needed, I'd rather eliminate the Option and make the callers do:

from_package_install(package, &package.installed_path) // vs from_package_install(package, &some_other_path)

Since I think that's more evocative to read at the call-site than

from_package_install(package, None) // vs from_package_install(package, &some_other_path)

The config_from is explicitly passed by the supervisor. You just don't see it since that is in a separate repo.

Yeah, I see here and here.

I think I get it now. This is an Option because ServiceSpec's field is an option. That makes more sense.

I think we may be better served by removing the optionality from ServiceSpec; I'm not convinced it's necessary. But that's a change for another day.

christophermaier · 2018-11-27T20:07:50Z

components/core/Cargo.toml

@@ -10,11 +10,13 @@ base64 = "*"
 gcc = "0.3"

 [dependencies]
+actix-web = { version = "*", default-features = false }


Why do we need Actix in core now?

HealthCheck uses its StatusCode but based on your point below, we probably don't need that here after all.

Its used by HealthCheck. I looked at moving that back to the supervisor as you suggested below but its pretty well coupled with the hooks (see my comment below).

christophermaier · 2018-11-27T20:08:35Z

components/core/src/error.rs

@@ -37,6 +37,7 @@ pub enum Error {
    /// Occurs when a `habitat_core::package::PackageArchive` is being read.
    ArchiveError(libarchive::error::ArchiveError),
    BadBindingMode(String),
+    BadEnvConfig(String),


It'd be nice to have a doc comment on this, similar to the others.

Not sure why github is not showing this as "outdated" its been fixed

christophermaier · 2018-11-27T20:08:49Z

components/core/src/error.rs

    PermissionFailed(String),
    /// Error parsing the contents of a plan file were incomplete or malformed.
    PlanMalformed,
    // When CreateProcessAsUserW does not have the correct privileges
    PrivilegeNotHeld,
    /// When an error occurs parsing or compiling a regular expression.
    RegexParse(regex::Error),
+    /// Whwn an error occurs serializing rendering context


"Whwn" -> "When"

christophermaier · 2018-11-27T20:31:54Z

components/core/src/templating/health.rs

+use actix_web::http::StatusCode;
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize)]
+pub enum HealthCheck {


Does this really need to be over here? What would anything outside the Supervisor need this for?

hmm. Keeping this in the supervisor actually looks a bit messy. It is referenced several times in the HealthCheckHook. Given how the HookTable works, all hooks need to be defined in core.

christophermaier · 2018-11-27T20:41:12Z

components/core/src/templating/mod.rs

+    let cfg_renderer = config::CfgRenderer::new(pkg.path.join("config"))?;
+    cfg_renderer.compile(&pkg.name, &pkg, &ctx)?;
+
+    hooks::HookTable::from_package_install(package).compile(&pkg.name, &ctx);


What happens if you try to compile a hook that depends on things that aren't present in this limited rendering context (like census information)?

Seems like we should throw an error.

I'm assuming this comment is also targetted to config templates also? Currently it just renders nothing if there is nothing in the context. Thinking of the following example scenario, I'd prefer to raise a warning instead of an error:

There may be multiple config templates and some have, for example, binds and others do not. When the install hook fires at install time, it will need to compile all templates since it cannot assume which templates it will use. We wouldn't want the templates with the binds to error during installation.

Also for debug purposes, one might use hab pkg compile and there would be no bind context. So a warning informing the cli user that there is missing context would be nice but they could still render the templates.

One extra point. I'm not certain yet how much we can control for that via the serde API.

I also thought we were going to have a specific install hook, or am I misremembering?

That might end up being cleaner, since we wouldn't necessarily need to bring over all the hooks, just the handlebars and context stuff.

Yeah there is an InstallHook added to hooks.rs. Maybe it makes better sense for the HookTable not to include that hook and then just keep HookTable and the non-install hook implementations with the supervisor.

The downside of keeping the non-install hooks with the supervisor is that the hab pkg compile cli command will not be able to compile them. Also, future work is going to need to be able to compile the health-check via the cli.

christophermaier · 2018-11-27T20:46:28Z

components/core/src/templating/test_helpers.rs

@@ -0,0 +1,106 @@
+// Copyright (c) 2017 Chef Software Inc. and/or applicable contributors


These also got moved to https://github.com/habitat-sh/habitat/blob/master/components/sup/src/test_helpers.rs... if we're moving them over here, the other tests in the supervisor that use them should pull them from here.

ends up that the only files using these functions were moved over

baumanj

The Sync issue on the Error enum will break habitat, so we can't merge this until we come up with a solution to that.

mwrock · 2018-11-29T02:52:42Z

@christophermaier @baumanj Still not great but what do you think of f320dff for dealing with the RenderError issues?

baumanj · 2018-11-29T16:46:57Z

@christophermaier @baumanj Still not great but what do you think of f320dff for dealing with the RenderError issues?

I think that's good! It's clean and small and I don't think should negatively impact the utility of the error messages at all. We might want to add a note to change it back when we upgrade the handlebars crate (maybe an issue, linked from our comments at the pinning in Cargo.toml), but otherwise I'm happy with it.

mwrock · 2018-12-10T22:25:03Z

Note that this now keeps the supervisor specific hooks along with HookTable with the supervisor code. Only the Hook trait, supporting structs and the InstallHook remains here.

Also it is now possible to specify the directory where config files should be rendered to. They used to always render to svc_config_path. Configuration files to be referenced in an install hook will reside in a config_install directory and render to svc_config_install_path.

christophermaier

Looks good... I had a few suggestions around OS-specific code, which could very well be left for another PR.

For good record-keeping, though, this PR's title and body should be rewritten to reflect what's now going on (there isn't really any templating-specific code in here anymore, for instance).

christophermaier · 2019-01-14T19:08:29Z

components/core/src/os/users/linux.rs

+#[cfg(target_os = "macos")]
+pub fn can_run_services_as_svc_user() -> bool {
+    true
+}


We should move this out of this module, since it doesn't pertain to Linux.

I'm not sure we really even need this implementation; I don't think any macOS code would make it down to anyplace that this function would be needed.

In the same spirit, if we're only pulling this module in on Linux, then all the #[cfg(target_os = "linux")] annotations end up being noise.

yup. I just double checked and don't see any scenario where code would reach here on a mac until it runs a supervisor and we support darwin package installs.

christophermaier · 2019-01-14T19:12:04Z

components/core/src/os/users/windows.rs

        env::remove_var("USERNAME");
-        assert_eq!(get_current_username(), None)
+        assert_eq!(get_current_username(), None);
+        env::set_var("USERNAME", orig_user);


Any chance these two tests end up stepping on each other? They're both manipulating global state.

TIL: cargo check runs tests in parallel. So yes.

I'd prefer to tackle this separately by refactoring the windows implementation of get_current_username to call the GetUserNameW API rather than checking the environment variable. Will file an issue.

Using an API call is probably a better way forward. And yeah, it sounds worth a separate PR 👍

If you do end up sticking with the environment variable approach, though, you'll want to look at using our new-ish locked env var tooling (which would also mean pulling it over here to the core crate).

Signed-off-by: mwrock <matt@mattwrock.com>

mwrock requested review from baumanj, chefsalim, christophermaier, fnichol, raskchanky and reset as code owners November 17, 2018 01:53

mwrock added A-supervisor A-cli C-feature E-less-easy L-rust T-DO-NOT-MERGE X-feature labels Nov 17, 2018

mwrock force-pushed the templating branch 2 times, most recently from 6347676 to a0bfd44 Compare November 20, 2018 17:34

This was referenced Nov 26, 2018

extract supervisor templating and execute an install hook when installing packages habitat-sh/habitat#5866

Merged

Allow packages to expose a runnable hook habitat-sh/habitat#5161

Closed

baumanj reviewed Nov 27, 2018

View reviewed changes

baumanj suggested changes Nov 27, 2018

View reviewed changes

baumanj approved these changes Nov 27, 2018

View reviewed changes

christophermaier suggested changes Nov 27, 2018

View reviewed changes

baumanj suggested changes Nov 28, 2018

View reviewed changes

mwrock force-pushed the templating branch from f320dff to c1f090f Compare November 29, 2018 18:36

christophermaier mentioned this pull request Nov 30, 2018

Add a handlebars linter to check for undefined variables in hooks habitat-sh/habitat#1779

Open

mwrock force-pushed the templating branch from c1f090f to 8f32292 Compare December 7, 2018 02:03

mwrock force-pushed the templating branch from 0b04576 to 0eb8473 Compare December 8, 2018 00:24

baumanj approved these changes Dec 11, 2018

View reviewed changes

mwrock force-pushed the templating branch from d3439b6 to 67bbd24 Compare December 20, 2018 01:29

mwrock force-pushed the templating branch 4 times, most recently from 739da6d to e7f7b90 Compare January 11, 2019 18:53

christophermaier approved these changes Jan 14, 2019

View reviewed changes

mwrock force-pushed the templating branch from e7f7b90 to 73285e0 Compare January 14, 2019 20:18

mwrock changed the title ~~extract supervisor templating into core~~ moving some common functionality from habitat Jan 14, 2019

moving some common functionality from habitat

5abf5e2

Signed-off-by: mwrock <matt@mattwrock.com>

mwrock force-pushed the templating branch from 73285e0 to 5abf5e2 Compare January 14, 2019 22:23

mwrock removed the T-DO-NOT-MERGE label Jan 18, 2019

mwrock merged commit 534e730 into master Jan 18, 2019

mwrock deleted the templating branch January 18, 2019 20:50

		@@ -0,0 +1,106 @@
		// Copyright (c) 2017 Chef Software Inc. and/or applicable contributors

moving some common functionality from habitat #90

moving some common functionality from habitat #90

Conversation

mwrock commented Nov 17, 2018 • edited Loading

thesentinels commented Nov 17, 2018

mwrock commented Nov 26, 2018

baumanj commented Nov 27, 2018

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

baumanj 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

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

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

baumanj left a comment

Choose a reason for hiding this comment

mwrock commented Nov 29, 2018

baumanj commented Nov 29, 2018

mwrock commented Dec 10, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwrock commented Nov 17, 2018 •

edited

Loading