Store configurable shutdown parameters #6539

davidMcneil · 2019-05-09T22:35:53Z

This is the initial work to finish the configurable shutdown feature started here #6450. There are three levels of precedence for the configurable shutdown timeout:
1 svc stop or svc unload
2. svc load
3. plan.sh.

The configurable shutdown signal can only be set in the plan.sh.

TODO

Add pkg_shutdown_timeout and pkg_shutdown_signal to the default template. Waiting for Streamline plan initialization #6495 to merge
Add windows build support to hab-plan-build.ps1
Test windows support
Resolve stop issue discussed here

Resolves #6451

chef-expeditor · 2019-05-09T22:36:11Z

Hello davidMcneil! 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!

christophermaier

@davidMcneil Looks great so far! I think there are some other cleanups we can squeeze out of this, but I'm really liking where it's going!

christophermaier · 2019-05-10T15:10:41Z

components/common/src/templating/package.rs

+                                  .clone()
+                                  .expect("No package release in PackageInstall"),
+                  shutdown_signal: package.shutdown_signal()?.unwrap_or_else(Default::default),
+                  shutdown_timeout: package.shutdown_timeout()?.unwrap_or_else(Default::default) };


These could use unwrap_or_default() instead of unwrap_or_else(Default::default)

It strikes me as very awkward that PackageInstall::shutdown_signal and ::shutdown_timeout can fail. Delegating the error handling of read_metafile to the callers of the individual accessors seems like the wrong pattern here. Returning Option<ShutdownSignal> is fine, but passing around a PackageInstall which may fail in read_metafile seems like forcing the error handling later than we want. I'd expect the failures to occur when PackageInstall is created. I understand the impetus to only call read_metafile lazily, but I don't think it's necessary for performance and makes the code more awkward and harder to reason about.

I know this predates your change and would require some reworking to address, but I think it's worthwhile to avoid perpetuating a bad pattern.

I completely agree about pushing error handling down into PackageInstall, but I think that is going to be a piece of work by itself, and may have far-reaching impact. I don't think that should be a part of this PR.

Can we get an issue filed and do it as a direct follow up? If not, I think the likelihood of this getting fixed is low.

@baumanj We can file an issue, but how direct a follow-up it would be will need some research. I started to look into this a while ago, but I think it had a much bigger footprint that you might otherwise think.

Created issue #6572.

christophermaier · 2019-05-10T15:16:12Z

components/hab/src/cli.rs

-        app
-    }
+/// a customized timeout.
+fn add_configurable_shutdown_options(mut app: App<'static, 'static>) -> App<'static, 'static> {


Since this function is just dealing with a single option now, we might want to change the name of this function to something like add_shutdown_timeout_option.

christophermaier · 2019-05-10T15:19:07Z

components/hab/src/cli.rs

+                                                           a shutdown signal to wait before \
+                                                           killing a service process")
+                                                    .long("shutdown-timeout")
+                                                    .takes_value(true));


Also, since we're just adding a single option, you can get rid of the mutability here:

fn add_configurable_shutdown_options(app: App<'static, 'static>) -> App<'static, 'static> { app.arg(Arg::with_name("SHUTDOWN_TIMEOUT").help("The number of seconds after sending a \ shutdown signal to wait before killing a \ service process") .long("shutdown-timeout") .takes_value(true)) }

christophermaier · 2019-05-10T15:30:50Z

components/hab/src/main.rs

@@ -1051,6 +1050,8 @@ fn sub_svc_load(m: &ArgMatches<'_>) -> Result<()> {
    update_svc_load_from_input(m, &mut msg)?;


This reminds me... we'll also need to add this option to the run command, as well.

(hab sup run takes options to configure the running of the Supervisor, but it also can take the same options that hab svc load does. Normally, you would just start up a Supervisor using hab sup run without any of the service-specific options. However, for scenarios like containers it is convenient to be able to start the container and configure the (single) service it is going to run in a single invocation.)

Looking at update_svc_load_from_input, it always takes an unmodified default SvcLoad struct as input, so we could just create that struct inside that function (and remove the update_ prefix from its name). We'd also want to add the shutdown timeout parsing call to that function.

christophermaier · 2019-05-10T15:41:26Z

components/hab/src/main.rs

+     .map(|s| s.parse().map_err(Into::into))
+     //  Convert from Option<Result<_>> to Result<Option<_>>
+     .map_or(Ok(None), |o| o.map(Some))
+}


This is pretty slick 👍

This is very slick indeed.

From a usability and code simplicity perspective, I think we're better off putting the validation into CLAP (with validator, see example) so that the user has a consistent experience and then we don't need to deal with the potential failures at this level.

Man, Github's UI is being very weird today...

I have a thought: #6539 (comment)

christophermaier · 2019-05-10T15:46:38Z

components/sup-protocol/protocols/ctl.proto

 }

 // Request to unload a loaded service.
 message SvcUnload {
  optional sup.types.PackageIdent ident = 1;
-  // Name of the signal to send the service to shut it down (e.g.,
-  // "TERM" and not "SIGTERM"). Only applies to Unix platforms.
-  optional string signal = 2;
  // Timeout in before killing the service
  optional uint32 timeout_in_seconds = 3;
 }


When removing a field from a protobuf message, it's important to prevent both the field name and field number from being reused in the future, which can break compatibility guarantees.

Check out the docs for more.

christophermaier · 2019-05-10T15:46:50Z

components/sup-protocol/protocols/ctl.proto

@@ -119,9 +118,6 @@ message SvcStart {
 // Request to stop a loaded and started service.
 message SvcStop {
  optional sup.types.PackageIdent ident = 1;
-  // Name of the signal to send the service to shut it down (e.g.,
-  // "TERM" and not "SIGTERM"). Only applies to Unix platforms.
-  optional string signal = 2;


Same as above.

christophermaier · 2019-05-10T16:31:44Z

components/core/src/os/process/mod.rs

@@ -86,7 +88,7 @@ impl From<ShutdownTimeout> for Duration {
 // but we are making it available on Windows as well for situations
 // where a Windows CLI is communicating with a Linux Supervisor.
 #[allow(non_snake_case)]
-#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Copy, Hash)]


Similar logic applies here as for the ShutdownSignal above, but with a twist.

The reason you need to have Serialize and Deserialize here at all is that ServiceStatus has derived Deserialize.

habitat/components/sup/src/manager/commands.rs

Lines 408 to 414 in 14a6dcd

#[derive(Deserialize)]

struct ServiceStatus {

pkg: Pkg,

process: ProcessStatus,

service_group: ServiceGroup,

desired_state: DesiredState,

}

This struct also has a pkg field, even though it ultimately only really needs the ident field from Pkg.

Because of all this, Pkg needs to implement Deserialize, which then cascades out to ShutdownSignal and ShutdownTimeout. If we could rework ServiceStatus to not need Pkg, then that cuts off the chain, meaning we don't need to implement either Serialize or Deserialize for the shutdown types.

(Actually, Service and Pkg don't really even need to implement Serialize at all anymore, so we can go ahead and get rid of a bunch of Serialize implementations right now!)

So, in the end, I think we can remove those Serialize impls, and temporarily derive Deserialize for the ShutdownTimeout. Once we can rework ServiceStatus, we can then remove that, too.

(To be clear, ShutdownTimeout will need both Serialize and Deserialize because the spec file logic depends on it.)

(The reason that Service and Pkg implement Serialize is actually a leftover of history, which you can catch up on in #4823. The TL;DR of it is that the data that actually gets serialized for templating is not Pkg anymore, but rather a proxy object that allows us to separate the internal implementation details from the interface we present to users. There was a similar change that was made for Service later on, in #5689.

I don't think there's currently a really compelling reason to add the signal and shutdown data to that PkgProxy right now (I'm not quite sure what you'd meaningfully want to do with them in a template). If people ask for it, we can easily add it later.

christophermaier · 2019-05-10T16:38:34Z

components/core/src/os/process/mod.rs

@@ -51,7 +53,7 @@ use time::Duration;
 /// throughout our code, which can be confusing, we can just pass this
 /// around, and turn it into a `time::Duration` at the last possible
 /// moment.)
-#[derive(Debug, Clone)]
+#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Hash)]
 pub struct ShutdownTimeout(u32);


This reminded me of #6469.

Though deriving Serialize and Deserialize currently does the "right thing" here (we'll end up getting the same thing we'd get if we were to go through the FromStr and Display traits), if we were to end up changing the underlying implementation details of Signal, we could potentially introduce incompatibilities elsewhere, particularly in spec file serialization.

I don't foresee us changing the underlying u32 here, but Duration is also a logical choice, and it serializes very differently. Locking down the concrete behavior we want here is the safe choice to make.

baumanj

Functionality all looks good to me!

I think we can simplify some things, but all these comments are yours to take or leave as you see fit.

I would like to see the one TODO addressed before merging, but I think @christophermaier has more context there.

baumanj · 2019-05-13T20:49:46Z

components/common/src/templating/package.rs

+                                  .clone()
+                                  .expect("No package release in PackageInstall"),
+                  shutdown_signal: package.shutdown_signal()?.unwrap_or_else(Default::default),
+                  shutdown_timeout: package.shutdown_timeout()?.unwrap_or_else(Default::default) };


It strikes me as very awkward that PackageInstall::shutdown_signal and ::shutdown_timeout can fail. Delegating the error handling of read_metafile to the callers of the individual accessors seems like the wrong pattern here. Returning Option<ShutdownSignal> is fine, but passing around a PackageInstall which may fail in read_metafile seems like forcing the error handling later than we want. I'd expect the failures to occur when PackageInstall is created. I understand the impetus to only call read_metafile lazily, but I don't think it's necessary for performance and makes the code more awkward and harder to reason about.

I know this predates your change and would require some reworking to address, but I think it's worthwhile to avoid perpetuating a bad pattern.

baumanj · 2019-05-13T21:04:00Z

components/hab/src/main.rs

+     .map(|s| s.parse().map_err(Into::into))
+     //  Convert from Option<Result<_>> to Result<Option<_>>
+     .map_or(Ok(None), |o| o.map(Some))
+}


This is very slick indeed.

From a usability and code simplicity perspective, I think we're better off putting the validation into CLAP (with validator, see example) so that the user has a consistent experience and then we don't need to deal with the potential failures at this level.

baumanj · 2019-05-13T21:30:50Z

components/plan-build/bin/shared.bash

@@ -244,6 +244,22 @@ _render_metadata_SVC_USER() {
  echo "$pkg_svc_user" > "$pkg_prefix"/SVC_USER
 }

+_render_metadata_SHUTDOWN_SIGNAL() {
+  if [[ -n "$pkg_shutdown_signal" ]]; then


Since this file gets sourced by others, so we don't know whether or not set -u, which aborts on unbound variable expansion will be in effect. In that case, it's best to program defensively and handle the unset situation, by replacing it with the empty string (see the Parameter Expansion docs; definitely worth bookmarking):

Suggested change

if [[ -n "$pkg_shutdown_signal" ]]; then

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

Similarly elsewhere

Also, there's a fair bit of redundancy here. We could add

_render_metadata() { local key=${1?required argument: metadata key} local value=${2?required argument: metadata value} local required=${3:-false} if "$required" || [[ -n "${value:-}" ]]; then debug "Rendering $key metadata file" # shellcheck disable=2154 echo "$value" > "$pkg_prefix"/"$key" fi }

and then the callers change from

_render_metadata_SVC_GROUP _render_metadata_SVC_USER _render_metadata_SHUTDOWN_SIGNAL _render_metadata_SHUTDOWN_TIMEOUT

to

_render_metadata SVC_GROUP "$pkg_svc_group" true _render_metadata SVC_USER "$pkg_svc_user" true _render_metadata SHUTDOWN_SIGNAL "$pkg_shutdown_signal" _render_metadata SHUTDOWN_TIMEOUT "$pkg_shutdown_timeout"

or if we add a helper:

_render_required_metadata() { _render_metadata "$@" true }

_render_required_metadata SVC_GROUP "$pkg_svc_group" _render_required_metadata SVC_USER "$pkg_svc_user" _render_metadata SHUTDOWN_SIGNAL "$pkg_shutdown_signal" _render_metadata SHUTDOWN_TIMEOUT "$pkg_shutdown_timeout"

I would prefer to do the redundancy cleanup as part of another PR. I opened up ticket #6571.

baumanj · 2019-05-13T21:53:53Z

components/sup/src/manager/action.rs

 #[derive(Clone, Debug, Default)]
-pub struct ShutdownSpec {
+pub struct ShutdownInput {


Since this just takes a u32 wrapper and puts it in an option, I think it should be Copy. For now this simplifies consuming code. If this struct grows large enough later that it doesn't make sense, the compiler will guide us towards the appropriate changes elsewhere.

baumanj · 2019-05-13T22:06:29Z

components/core/src/os/process/mod.rs

@@ -51,7 +53,7 @@ use time::Duration;
 /// throughout our code, which can be confusing, we can just pass this
 /// around, and turn it into a `time::Duration` at the last possible
 /// moment.)
-#[derive(Debug, Clone)]
+#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Hash)]


Suggested change

#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Hash)]

#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Copy, Hash)]

Structs which wrap Copy typesshould themselves beCopy` since they're representationally identical to the underlying type and it simplifies consuming code.

baumanj · 2019-05-13T22:27:35Z

components/sup/src/manager/action.rs

-        ShutdownSpec { signal, timeout }
+impl Into<ShutdownInput> for habitat_sup_protocol::ctl::SvcUnload {
+    fn into(self) -> ShutdownInput {
+        ShutdownInput { timeout: self.timeout_in_seconds.map(Into::into), }


In instances where we know the type, and not just the trait bound, I prefer to use it since I think it makes the code clearer. I'd prefer:

Suggested change

ShutdownInput { timeout: self.timeout_in_seconds.map(Into::into), }

ShutdownInput { timeout: self.timeout_in_seconds.map(ShutdownTimeout::from), }

but this is also preferable to Into::into in my opinion:

Suggested change

ShutdownInput { timeout: self.timeout_in_seconds.map(Into::into), }

ShutdownInput { timeout: self.timeout_in_seconds.map(u32::into), }

Similarly elsewhere

I noticed this previous comment. I do not have a particularly strong preferences either way, but I would like to standardize on a common method.

I agree that a standard is best; no matter what it is. Let's continue the discussion over at that aforementioned comment.

baumanj · 2019-05-13T22:30:43Z

components/core/src/os/process/mod.rs

@@ -151,7 +153,7 @@ impl fmt::Display for Signal {
 /// Encapsulates logic for defining the default shutdown signal we
 /// send services, and handles translation from external types at the
 /// edges of our system.
-#[derive(Debug, Clone)]
+#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Hash)]


Suggested change

#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Hash)]

#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Copy, Hash)]

baumanj · 2019-05-13T22:45:21Z

components/sup/src/manager/mod.rs

+pub struct ShutdownConfig {
+    pub signal:  ShutdownSignal,
+    pub timeout: ShutdownTimeout,
+}


I think these can be merged into one structure declaration which makes the difference clearer and eliminates some redundancy

pub struct ShutdownConfig { #[cfg(unix)] pub signal: ShutdownSignal, pub timeout: ShutdownTimeout, }

baumanj · 2019-05-13T22:49:59Z

components/sup/src/manager/mod.rs

+                                                                    })
+                                                    });
+        let signal = pkg.shutdown_signal.clone();
+        Self { signal, timeout }


Like the declaration, instead of fully separate unix and windows impl blocks, I think this puts a focus on the differences:

#[cfg(unix)] let shutdown_config = Self { signal: pkg.shutdown_signal.clone(), timeout }; #[cfg(windows)] let shutdown_config = Self { timeout }; shutdown_config

baumanj · 2019-05-13T22:51:08Z

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

@@ -149,6 +150,7 @@ impl IntoServiceSpec for habitat_sup_protocol::ctl::SvcLoad {
        if let Some(ref interval) = self.health_check_interval {
            spec.health_check_interval = interval.seconds.into()
        }
+        spec.shutdown_timeout = self.shutdown_timeout.map(Into::into);


Same comment as previously about Into::into

christophermaier · 2019-05-15T16:34:18Z

components/core/src/os/process/mod.rs

-#[derive(Debug, Clone)]
-pub struct ShutdownSignal(Signal);
+#[derive(Deserialize, Debug, Clone, Copy)]
+pub struct ShutdownSignal(#[serde(with = "serde_string")] Signal);


This is cool, but after doing some digging, I see that we could also do something like this, using the from attribute:

#[derive(Deserialize, Debug, Clone, Copy)] #[serde(from = "Signal")] pub struct ShutdownSignal(Signal);

Using serde_string is good, too, so I bring it up mainly as a way of sharing something nifty I just uncovered 😄 As we get a bit more principled with how we're using Serde, though, I think it would be good to dig further into the crate to try and uncover ways like this to leverage existing code.

(I went on a search for other alternatives, because I find it a little amazing that there doesn't seem to be support built into Serde for automatically taking advantage of existing Display and FromStr implementations on a type to do serialization and deserialization.)

I really do like how you refactored the serde_string stuff to make it more ergonomic to use, though; that's a nice touch!

I had not seen "from" that is cool. I found this open issue about adding documentation for the method I used. Which makes me think that maybe it is their preferred method? There serialize and deserialize implementations are simpler than ours so we could maybe clean it up more.

Good find! Yeah, that implementation looks nice (especially with the right trait bounds).

I like it 👍

christophermaier · 2019-05-15T16:41:02Z

components/hab/src/cli.rs

@@ -1095,15 +1095,25 @@ pub fn sub_svc_status() -> App<'static, 'static> {
    )
 }

-fn sub_svc_stop(feature_flags: FeatureFlag) -> App<'static, 'static> {
+pub fn parse_optional_arg<T: FromStr, E>(name: &str, m: &ArgMatches) -> Result<Option<T>, E>


I think we will only be using this in the case that we've successfully gotten past the validation stage.

In that case, we could tighten up the return type to be simply Option<T>, since presumably any input that couldn't be successfully parsed would have been weeded out before now. We could tack an expect() to s.parse(), and call it a day.

(I've been writing some code elsewhere that deals with these conversions and it's been both clarifying and simplifying to write functions with the knowledge that the input will have been validated beforehand.)

christophermaier · 2019-05-15T16:59:17Z

@davidMcneil This all looks really good. I'm gonna have a think about how best to tackle that outstanding TODO.

christophermaier

@davidMcneil I tried this out and it works like a charm. Nice work!

I do have a small suggestion, though... let me know what you think.

christophermaier · 2019-05-28T21:27:36Z

components/plan-build/bin/shared.bash

+}
+
+_render_metadata_SHUTDOWN_TIMEOUT() {
+  if [[ -n "${pkg_shutdown_timeout:-}" ]]; then


What do you think about having the variable be named pkg_shutdown_timeout_sec, to convey the units?

christophermaier · 2019-05-28T21:44:53Z

@davidMcneil Actually, we'll need some updated documentation in https://github.com/habitat-sh/habitat/blob/master/www/source/partials/docs/_reference-plan-settings.html.md.erb and https://github.com/habitat-sh/habitat/blob/master/www/source/partials/docs/_reference-basic-settings.html.md.erb, though I'm not quite sure why we have two files that seem to be largely the same 🤔

christophermaier · 2019-05-31T17:36:57Z

@davidMcneil Looks great; feel free to merge whenever you like!

baumanj

Address these small issues in www and then I think we should be good to go. Since @raskchanky is the only www code owner, you'll need to do an admin merge.

baumanj · 2019-06-11T18:26:03Z

www/source/partials/docs/_reference-basic-settings.html.md.erb

+: Optional. The signal to send the service to shutdown. The default is `TERM`.
+
+```bash
+pkg_shutdown_signal=$pkg_shutdown_signal


This example could be more helpful. Why not something like

Suggested change

pkg_shutdown_signal=$pkg_shutdown_signal

pkg_shutdown_signal=HUP

?

baumanj · 2019-06-11T18:27:32Z

www/source/partials/docs/_reference-plan-settings.html.md.erb

+**Optional**. The signal to send the service to shutdown. The default is `TERM`.
+
+```bash
+pkg_shutdown_signal=$pkg_shutdown_signal


Same as above

There are three places that a configurable shutdown timeout may be set: 1 `svc stop` or `svc unload` 2. `svc load` 3. `plan.sh`. The configurable shutdown signal can only be set in the `plan.sh`. Signed-off-by: David McNeil <dmcneil@chef.io>

Signed-off-by: David McNeil <dmcneil@chef.io>

Remove unnecessary deriving from Serialize Use string methods for serialization where it makes sense Signed-off-by: David McNeil <dmcneil@chef.io>

Signed-off-by: David McNeil <dmcneil@chef.io>

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Signed-off-by: David McNeil <dmcneil@chef.io>

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

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

davidMcneil requested review from baumanj, chefsalim, christophermaier, eeyun, raskchanky and scotthain as code owners May 9, 2019 22:35

christophermaier assigned davidMcneil May 10, 2019

christophermaier added the X-feature label May 10, 2019

christophermaier suggested changes May 10, 2019

View reviewed changes

baumanj approved these changes May 13, 2019

View reviewed changes

davidMcneil force-pushed the dmcneil/configurable-shutdown branch from 8fc1460 to 5ccd81a Compare May 15, 2019 15:56

christophermaier reviewed May 15, 2019

View reviewed changes

davidMcneil force-pushed the dmcneil/configurable-shutdown branch from 5ccd81a to 395aa27 Compare May 16, 2019 16:39

christophermaier mentioned this pull request May 16, 2019

allow dots in package names #6569

Closed

davidMcneil force-pushed the dmcneil/configurable-shutdown branch from cb7322f to 4f162f0 Compare May 17, 2019 15:34

This was referenced May 17, 2019

Reduce redundencies in shared.bash #6571

Closed

Remove failable methods from PackageInstall #6572

Closed

davidMcneil force-pushed the dmcneil/configurable-shutdown branch 2 times, most recently from c57c431 to e541178 Compare May 28, 2019 16:12

christophermaier approved these changes May 28, 2019

View reviewed changes

davidMcneil force-pushed the dmcneil/configurable-shutdown branch 2 times, most recently from 00debf3 to e5b4bbc Compare June 6, 2019 18:14

baumanj reviewed Jun 11, 2019

View reviewed changes

davidMcneil force-pushed the dmcneil/configurable-shutdown branch from ec04702 to 5ce2da5 Compare June 11, 2019 18:53

davidMcneil added 2 commits June 11, 2019 21:03

Store configurable shutdown parameters

8de11fe

There are three places that a configurable shutdown timeout may be set: 1 `svc stop` or `svc unload` 2. `svc load` 3. `plan.sh`. The configurable shutdown signal can only be set in the `plan.sh`. Signed-off-by: David McNeil <dmcneil@chef.io>

Use unwrap_or_default

12645b0

Signed-off-by: David McNeil <dmcneil@chef.io>

davidMcneil added 19 commits June 11, 2019 21:03

Rename to add_shutdown_timeout_option

f8a17a1

Signed-off-by: David McNeil <dmcneil@chef.io>

Add --shutdown-timeout to hab sup run

bc44f78

Signed-off-by: David McNeil <dmcneil@chef.io>

Cleanup serialization

dbf606d

Remove unnecessary deriving from Serialize Use string methods for serialization where it makes sense Signed-off-by: David McNeil <dmcneil@chef.io>

Add copy to shutdown timeout and signal

c2c889c

Signed-off-by: David McNeil <dmcneil@chef.io>

Use stored shutdown config on noninteractive shutdown

29d8853

Signed-off-by: David McNeil <dmcneil@chef.io>

Add shutdown signal Serialize to fix status

5a190fd

Signed-off-by: David McNeil <dmcneil@chef.io>

Reduce duplication of conditional compilation

7ca0b51

Signed-off-by: David McNeil <dmcneil@chef.io>

Use validator for --shutdown-timeout

ed3666c

Signed-off-by: David McNeil <dmcneil@chef.io>

Add shutdown config to plan.sh template

6c48785

Signed-off-by: David McNeil <dmcneil@chef.io>

Convert Into::into into from

da9f872

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add powershell support for shutdown timeout in plan

57b240c

Signed-off-by: David McNeil <dmcneil@chef.io>

Add documentation for --shutdown-timeout

4498ad5

Signed-off-by: David McNeil <dmcneil@chef.io>

Fix build on windows

a1a6b21

Signed-off-by: David McNeil <dmcneil@chef.io>

Remove copyright

121921e

Signed-off-by: David McNeil <dmcneil@chef.io>

Rename pkg_shutdown_timeout to pkg_shutdown_timeout_sec

aacfba8

Signed-off-by: David McNeil <dmcneil@chef.io>

Add shutdown ducumentation

5cc45c6

Signed-off-by: David McNeil <dmcneil@chef.io>

Fix testcase

45bf56c

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Fix shutdown_timeout serialization

a4e47d6

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add example signal to docs

0abbeb0

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

davidMcneil force-pushed the dmcneil/configurable-shutdown branch from 5ce2da5 to 0abbeb0 Compare June 12, 2019 01:04

davidMcneil merged commit b5716da into master Jun 12, 2019

davidMcneil deleted the dmcneil/configurable-shutdown branch June 12, 2019 19:06

chef-ci added a commit that referenced this pull request Jun 12, 2019

Update CHANGELOG.md with details from pull request #6539

40d3fcd

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

christophermaier added Type:Feature PRs that add a new feature and removed X-feature labels Jul 24, 2020

christophermaier mentioned this pull request Nov 6, 2020

Smma/worker scheduler 1523 habitat-sh/builder#1539

Merged

		@@ -1051,6 +1050,8 @@ fn sub_svc_load(m: &ArgMatches<'_>) -> Result<()> {
		update_svc_load_from_input(m, &mut msg)?;

	#[derive(Deserialize)]
	struct ServiceStatus {
	pkg: Pkg,
	process: ProcessStatus,
	service_group: ServiceGroup,
	desired_state: DesiredState,
	}

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

	#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Hash)]
	#[derive(Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Copy, Hash)]

	ShutdownInput { timeout: self.timeout_in_seconds.map(Into::into), }
	ShutdownInput { timeout: self.timeout_in_seconds.map(ShutdownTimeout::from), }

	pkg_shutdown_signal=$pkg_shutdown_signal
	pkg_shutdown_signal=HUP

Store configurable shutdown parameters #6539

Store configurable shutdown parameters #6539

Conversation

davidMcneil commented May 9, 2019 • edited Loading

chef-expeditor bot commented May 9, 2019

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

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

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

christophermaier commented May 15, 2019

christophermaier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophermaier commented May 28, 2019

christophermaier commented May 31, 2019

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

davidMcneil commented May 9, 2019 •

edited

Loading