extract supervisor templating and execute an install hook when installing packages #5866

mwrock · 2018-11-21T00:18:00Z

This PR includes the following key parts:

Extracting code related to compiling templates and hooks moved to core. For a more complete description of that work see moving some common functionality from habitat core#90
Adding a hab pkg compile command that will compile all templates in a package in an ad hoc manner
Added behavior to execute a new install hook upon loading a package and also adding a InstallHookMode argument to hab pkg install that takes one of 3 values: default, ignore, and force.
The docker exporter ignores install hooks when assembling the build root locally and then forces the hook when executing the docker file.

The InstallHookMode values have the following corresponding behavior:
default: Runs the install hook when initially loading/installing a package
ignore: Ignores any install hooks when loading a package (this will be key for exporter scenarios and debugging) force: will execute the install` hook of package and any deps regardless of whether they have been previously installed (also key for exporters and debugging)

Note: I considered adding arguments to the compile sub command for saving the rendered templates in an alternate directory. I'm not strictly opposed to adding that in but the current use cases will need them to be in the hab/svc directory and adding the ability to save them elsewhere just adds more refactoring that can be added in a separate PR.

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

thesentinels · 2018-11-21T00:18:02Z

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-26T19:55:29Z

components/hab/src/command/pkg/compile.rs

@@ -12,16 +12,17 @@
 // See the License for the specific language governing permissions and


It's weird that git thinks this is a rename of components/sup/src/sys/windows/abilities.rs because it certainly is not.

Maybe just GitHub is confused? My local git doesn't see this as a rename:

➤ git diff --name-status 2af19c74cd7653ded563f7ef1f5997d18a53216e M Cargo.lock M components/common/src/command/package/install.rs M components/pkg-export-docker/defaults/Dockerfile.hbs M components/pkg-export-docker/defaults/Dockerfile_win.hbs M components/pkg-export-docker/src/build.rs M components/pkg-export-docker/src/docker.rs

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

related: #5161

mwrock · 2018-11-26T22:21:35Z

Note I plan to add to the docs before this is merged but would like some review first in case that changes the direction of the docs.

baumanj · 2018-11-27T23:15:37Z

Review in progress. Should finish tomorrow.

baumanj

Mostly optional readability feedback, but I do feel pretty strongly about renaming InstallHookMode and it's variants to improve clarity.

baumanj · 2018-11-27T20:59:36Z

components/common/src/command/package/install.rs

@@ -197,6 +200,32 @@ impl Default for InstallMode {
    }
 }

+#[derive(Debug, Eq, PartialEq)]
+pub enum InstallHookMode {
+    Default,


We might want to give this a more illustrative name like RunOnce or something. Plus, Default is easy to get confused with the trait name Default.

Yeah, that's a great point re: "Default"

components/common/src/command/package/install.rs

components/hab/src/cli.rs

baumanj · 2018-11-28T14:48:02Z

components/hab/src/command/pkg/compile.rs

@@ -12,16 +12,17 @@
 // See the License for the specific language governing permissions and


Maybe just GitHub is confused? My local git doesn't see this as a rename:

➤ git diff --name-status 2af19c74cd7653ded563f7ef1f5997d18a53216e M Cargo.lock M components/common/src/command/package/install.rs M components/pkg-export-docker/defaults/Dockerfile.hbs M components/pkg-export-docker/defaults/Dockerfile_win.hbs M components/pkg-export-docker/src/build.rs M components/pkg-export-docker/src/docker.rs

components/hab/src/main.rs

mwrock · 2018-12-10T22:31:49Z

Most recent commits bring back the supervisor specific hooks and HookTable. Also, loading a package and triggering an install hook now only compiles configuration files that reside in a config_install directory. This allows us to declare exactly which templates will support census context and also prevents changed install configuration files from causing a supervisor reconfigure.

mwrock · 2018-12-10T22:39:04Z

There are 2 key areas where I deviated from some changes @christophermaier and I discussed:

I'm not using strict mode compiling the install based config files since it is not supported in the handlebars version we are using.
I left out the feature flag. Thinking about the implications of configuration files in an install context, it seems having them in their own directory makes alot of sense. The main thing up for debate in my mind is the actual name of the directory. This targets config_install.

mwrock · 2018-12-29T00:43:49Z

This is ready for review again. After using/testing the feature more extensively and some discussions with @christophermaier I have made the following changes:

Most of the templating extraction into core has been relocated to common since nothing outside of habitat uses that.
We needed to persist the state/result of install_hooks so that in the event that they fail, we know and can retry them. We now create a file called INSTALL_STATUS in the pkg path that stores the exit code of the install hook. If an install hook exists and this file either does not exist of has a non 0 value, the install hook is retried whenever the package is loaded.
Some scenarios may not call common::command::package::install::start when first loading a package. The 2 scenarios where this is the case are packages built in a studio and packages loaded from a spec file by the supervisor. There is now a public check_install_hooks function that can be called to run all install hooks of a package and its TDEPS.
If a install hook fails, proceed no further in either loading the package into a supervisor or installing the parent package in the failure occurred in a dependent package.
Add integration tests to the supervisor to cover many of the above cases
With the above logic around capturing install hook success/failure and rerunning failed or unrun install hooks, there is not really a need for the Always InstallHookMode. This enum now only includes Ignore which will not run any install hooks and run (the default). Now pkg install includes just an optional flag --ignore-install-hook to set the mode to Ignore.
plan-build should specify --ignore-install-hook when installing runtime deps
all install hook behavior is behind the HAB_FEAT_INSTALL_HOOK flag. If this is not set, install hooks will never be rendered or run
docker exporter now takes optional --memory and --user-toml args. --memory is simply passed to docker build and --user-toml take a path to a user.toml file that , if provided, is copied to /hab/user/{svc name}/config/user.toml. Both of these args facilitate scenarios where install hooks are run during docker build. SQLServer is a great example that needs at least 2GB during a install and needs its svc_account config to be SYSTEM in a container.

baumanj

Mostly small things, but there are some correctness issues I think we should address.

baumanj · 2019-01-03T17:21:11Z

components/hab/src/main.rs

@@ -670,6 +672,13 @@ fn sub_pkg_install(ui: &mut UI, m: &ArgMatches) -> Result<()> {
        LocalPackageUsage::default()
    };

+    let install_hook_mode =
+        if !feat::is_enabled(feat::InstallHook) || m.is_present("IGNORE_INSTALL_HOOK") {


Suggested change

if !feat::is_enabled(feat::InstallHook) || m.is_present("IGNORE_INSTALL_HOOK") {

if m.is_present("IGNORE_INSTALL_HOOK") {

The argument is only available if the feature is enabled, so there's no way is_present could return true otherwise. If you try to add it without the feature being enabled, you get an error before ever reaching this code:

➤ ./target/debug/hab pkg install --ignore-install-hook core/redis error: Found argument '--ignore-install-hook' which wasn't expected, or isn't valid in this context

baumanj · 2019-01-03T17:55:00Z

components/sup/src/manager/mod.rs

+            if let Ok(package) =
+                PackageInstall::load(&service.pkg.ident, Some(Path::new(&*FS_ROOT_PATH)))
+            {
+                if let Err(err) = common::command::package::install::check_install_hooks(


This is based on your comment here, correct?

Some scenarios may not call common::command::package::install::start when first loading a package. The 2 scenarios where this is the case are packages built in a studio and packages loaded from a spec file by the supervisor. There is now a public check_install_hooks function that can be called to run all install hooks of a package and its TDEPS.

Since add_service is called to synchronize the supervisor state with the spec files, what happens if a service is loaded with --ignore-install-hook, then the supervisor is restarted and it runs this code based on the spec file? Since there's no INSTALL_HOOK_STATUS_FILE it seems like run_install_hook_when_failed would run the install hook, but that seems contrary to the user's intent.

Relatedly, run_install_hook_when_failed seems to be a bit of a misnomer. It's more like run_install_hook_unless_already_successful.

--ignore-install-hook is intentionally only available on hab pkg install and not on hab svc load. In a supervisor context any install hook should be run if provided otherwise It is safe to assume the service would fail.

Definitely agree on the function name. I never felt great about it and I like your suggested name.

Ok, that makes sense. I think a comment to that effect would be useful for posterity.

baumanj · 2019-01-03T18:10:46Z

components/plan-build/bin/hab-plan-build.sh

@@ -1970,6 +1974,27 @@ do_default_build_config() {
    done
    chmod 755 "$pkg_prefix"/config
  fi
+  if [[ "${HAB_FEAT_INSTALL_HOOK:-}" = "true" && -d "$PLAN_CONTEXT/config_install" ]]; then


Suggested change

if [[ "${HAB_FEAT_INSTALL_HOOK:-}" = "true" && -d "$PLAN_CONTEXT/config_install" ]]; then

if [[ -n "${HAB_FEAT_INSTALL_HOOK:-}" && -d "$PLAN_CONTEXT/config_install" ]]; then

The features environment variables consider anything non-null and non-empty to be true

components/plan-build/bin/hab-plan-build.sh

baumanj · 2019-01-03T19:08:27Z

components/plan-build/bin/hab-plan-build.sh

+    find "$PLAN_CONTEXT/config_install" "$find_exclusions" | while read -r FILE
+    do
+      if [[ -d "$FILE" ]]; then
+        mkdir -p "$pkg_prefix${FILE#$PLAN_CONTEXT}"


I consider myself fairly familiar with bash and yet still had to look it up to understand what ${parameter#word} does. Especially since it's used twice, I'd suggest an intermediate variable to make things a bit clearer:

local plan_context_relative_path="$pkg_prefix${FILE#$PLAN_CONTEXT}"

baumanj · 2019-01-03T19:24:09Z

components/pkg-export-docker/src/build.rs

+                .as_ref()
+                .join("hab")
+                .join("user")
+                .join(ctx.primary_svc_ident().clone().name)


There's no need to clone here

Suggested change

.join(ctx.primary_svc_ident().clone().name)

.join(&ctx.primary_svc_ident().name)

Or, you could avoid the many joins and just use one path:

let dst = rootfs.as_ref().join(format!( "hab/user/{}/config/user.toml", ctx.primary_svc_ident().name ));

According to the Path docs, this should work on Windows

baumanj · 2019-01-03T19:45:19Z

components/plan-build/bin/hab-plan-build.sh

@@ -745,10 +745,10 @@ _install_dependency() {
              "${HAB_FEAT_IGNORE_LOCAL:-}" = "TRUE" ]]; then
        IGNORE_LOCAL="--ignore-local"
    fi
-    $HAB_BIN install -u $HAB_BLDR_URL --channel $HAB_BLDR_CHANNEL ${IGNORE_LOCAL:-} "$dep" || {
+    $HAB_BIN install -u $HAB_BLDR_URL --channel $HAB_BLDR_CHANNEL ${IGNORE_LOCAL:-} "$dep" "$2" || {


I can see this is to support possibly passing --ignore-install-hook, but that's not very clear when viewed without the additional context of this PR. I'd suggest making this more general and nixing the local dep on L738:

Suggested change

$HAB_BIN install -u $HAB_BLDR_URL --channel $HAB_BLDR_CHANNEL ${IGNORE_LOCAL:-} "$dep" "$2" || {

$HAB_BIN install -u $HAB_BLDR_URL --channel $HAB_BLDR_CHANNEL ${IGNORE_LOCAL:-} "$@" || {

Ditto for L751.

baumanj · 2019-01-03T19:46:07Z

components/plan-build/bin/hab-plan-build.sh

@@ -1083,7 +1083,11 @@ _resolve_run_dependencies() {

  # Append to `${pkg_deps_resolved[@]}` all resolved direct run dependencies.
  for dep in "${pkg_deps[@]}"; do
-    _install_dependency "$dep"
+    if [[ "${HAB_FEAT_INSTALL_HOOK:-}" = "true" ]]; then


Suggested change

if [[ "${HAB_FEAT_INSTALL_HOOK:-}" = "true" ]]; then

if [[ -n "${HAB_FEAT_INSTALL_HOOK:-}" ]]; then

components/pkg-export-docker/src/build.rs

Obvious fix; these changes are the result of automation not creative thinking.

Some code cleanups after #5866

With #5866, we inadvertently broke the output of the HTTP gateway when retrieving health check hook information. The overall health of a service was still returned, but neither the standard output nor the standard error were included. Because the addition of `install` (and later, `unintsall`) hooks introduced a situation in which hooks could be executed without having a service group present (that is, outside of a running Supervisor process), the common logic for finding the path to the log output of hooks was changed to accept a `&str` rather than a `&ServiceGroup`. The value that is ultimately used should have only been the "service name" portion (e.g., "redis" and not "redis.default"). Unfortunately, because `ServiceGroup` implements `Deref<Target=String>`, the calls to `stdout_log_path` and `stderr_log_path` in the `http_gateway` module could continue to compile. Since `Deref` is automatically invoked, the compiler was able to coerce the `&ServiceGroup` reference all the way to a `&str`, thus satisfying the new code. However, this `&str` was the _full_ name of the `ServiceGroup` (e.g., "redis.default"), which ended up silently breaking this HTTP gateway functionality. Rather than looking for the files at `/hab/svc/$SERVICE/logs`, we were instead looking at `hab/svc/$SERVICE.$GROUP/logs`. Now, we explicitly call `service_group.service()` at this callsite, thus restoring the prior functionality. A "proper" fix would involve more extensive refactoring to ensure that we have stricter typing to ensure the right "kind" of string is making its way down to `stdout_log_path` and `stderr_log_path`, and possibly removing the `Deref<Target=String>` implementation for `ServiceGroup`, since there is clearly more than one kind of string-like thing we could conceivably want out of a `ServiceGroup`. Such an approach appears to get us rather far afield into the workings and usage of our `outputln!` macros, which is an entirely different can of worms that is best left unopened here (see #6584 for more on that). Signed-off-by: Christopher Maier <cmaier@chef.io>

mwrock requested review from baumanj, chefsalim, christophermaier, eeyun, elliott-davis, fnichol, raskchanky and reset as code owners November 21, 2018 00:18

mwrock commented Nov 26, 2018

View reviewed changes

mwrock force-pushed the install_hook branch 2 times, most recently from 2d72ece to e57b63e Compare November 26, 2018 21:15

mwrock mentioned this pull request Nov 26, 2018

Allow packages to expose a runnable hook #5161

Closed

mwrock force-pushed the install_hook branch from 36bf561 to 893c6ce Compare November 28, 2018 02:33

baumanj suggested changes Nov 28, 2018

View reviewed changes

mwrock force-pushed the install_hook branch from 7e9ecf9 to 2ed5e84 Compare November 29, 2018 19:14

mwrock force-pushed the install_hook branch 3 times, most recently from 0ca73b4 to 6dc1860 Compare December 10, 2018 22:05

baumanj approved these changes Dec 11, 2018

View reviewed changes

mwrock force-pushed the install_hook branch from fb2cecc to 1902e1e Compare December 27, 2018 00:51

mwrock requested a review from scotthain as a code owner December 29, 2018 00:06

mwrock force-pushed the install_hook branch from 472cec4 to bd5ac68 Compare January 3, 2019 19:54

baumanj suggested changes Jan 3, 2019

View reviewed changes

mwrock deleted the install_hook branch January 18, 2019 22:01

chef-ci added a commit that referenced this pull request Jan 18, 2019

Update CHANGELOG.md with details from pull request #5866

ecf5107

Obvious fix; these changes are the result of automation not creative thinking.

This was referenced Jan 18, 2019

Managing "run-less" services #5815

Closed

Allow docker exporter to override default base image #4948

Closed

jamesc mentioned this pull request Jan 23, 2019

Some code cleanups after #5866 #6077

Merged

jamesc pushed a commit that referenced this pull request Jan 24, 2019

Merge pull request #6077 from jamesc/jc/cleanups

7d68527

Some code cleanups after #5866

mwrock added A-supervisor labels Jan 29, 2019

mwrock mentioned this pull request Jan 29, 2019

Release 0.74.0. #6091

Merged

christophermaier mentioned this pull request Nov 17, 2020

[FIX] Ensure that health check output is available via HTTP gateway #7991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extract supervisor templating and execute an install hook when installing packages #5866

extract supervisor templating and execute an install hook when installing packages #5866

mwrock commented Nov 21, 2018 •

edited

Loading

thesentinels commented Nov 21, 2018

mwrock Nov 26, 2018

baumanj Nov 28, 2018

mwrock commented Nov 26, 2018

mwrock commented Nov 26, 2018

baumanj commented Nov 27, 2018

baumanj left a comment

baumanj Nov 27, 2018

christophermaier Jan 16, 2019

baumanj Nov 28, 2018

mwrock commented Dec 10, 2018

mwrock commented Dec 10, 2018

mwrock commented Dec 29, 2018

baumanj left a comment

baumanj Jan 3, 2019

baumanj Jan 3, 2019

mwrock Jan 3, 2019

baumanj Jan 4, 2019

baumanj Jan 3, 2019

baumanj Jan 3, 2019

baumanj Jan 3, 2019

baumanj Jan 3, 2019

baumanj Jan 3, 2019

baumanj Jan 3, 2019

		@@ -12,16 +12,17 @@
		// See the License for the specific language governing permissions and

	if !feat::is_enabled(feat::InstallHook) \|\| m.is_present("IGNORE_INSTALL_HOOK") {
	if m.is_present("IGNORE_INSTALL_HOOK") {

	if [[ "${HAB_FEAT_INSTALL_HOOK:-}" = "true" && -d "$PLAN_CONTEXT/config_install" ]]; then
	if [[ -n "${HAB_FEAT_INSTALL_HOOK:-}" && -d "$PLAN_CONTEXT/config_install" ]]; then

	.join(ctx.primary_svc_ident().clone().name)
	.join(&ctx.primary_svc_ident().name)

	$HAB_BIN install -u $HAB_BLDR_URL --channel $HAB_BLDR_CHANNEL ${IGNORE_LOCAL:-} "$dep" "$2" \|\| {
	$HAB_BIN install -u $HAB_BLDR_URL --channel $HAB_BLDR_CHANNEL ${IGNORE_LOCAL:-} "$@" \|\| {

	if [[ "${HAB_FEAT_INSTALL_HOOK:-}" = "true" ]]; then
	if [[ -n "${HAB_FEAT_INSTALL_HOOK:-}" ]]; then

extract supervisor templating and execute an install hook when installing packages #5866

extract supervisor templating and execute an install hook when installing packages #5866

Conversation

mwrock commented Nov 21, 2018 • edited Loading

thesentinels commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwrock commented Nov 26, 2018

mwrock commented Nov 26, 2018

baumanj commented Nov 27, 2018

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

mwrock commented Dec 10, 2018

mwrock commented Dec 10, 2018

mwrock commented Dec 29, 2018

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

mwrock commented Nov 21, 2018 •

edited

Loading