Experimental config file support behind `HAB_FEAT_CONFIG_FILE` feature flag #7360

davidMcneil · 2020-01-23T13:25:03Z

Resolves #7236

There is a lot going on here, and there is still a lot yet to be flushed out. For now, I am keeping the PR as a draft. Everything is behind a feature flag so it should be good to merge if we want to, but I want to get approval from the team on the general approach.

How to use a config file

Config file support is behind the HAB_FEAT_CONFIG_FILE feature flag. This environment variable doubles as the path to the config file. Currently, config file support only applies to the hab sup run subcommand. The config file is in toml format.

So a simple config file might look like:

listen_ctl = "5.6.7.8:8080"
organization = "acme"
bldr_url = "http://test.com"
group = "my_group"
channel = "unstable"

The config file works by overriding/setting cli argument default values. So if you run a command like:

> HAB_FEAT_CONFIG_FILE=<path/to/config/file> \
  hab sup run -h

You should see the default values overridden by the values in the config file.

Note: It would be nice if the cli help said the argument came from a config file, similar to how envvars are handled, instead of simply overriding the default value. This would be possible but would require modifying clap itself so I held off for now.

How does this work

The first step was to use structopt to define the cli (opposed to the clap macro). With structopt you define a struct (SubSupRun in this case) representing all cli arguments. If the command line input is valid it is parsed into the struct.

Note: Currently, we dont actually use the parsed struct. In fact, we dont even create an instance of the struct. structopt is really just acting as a fancy macro to define a clap app, but this can/should change in the future.

Note: There is a bug in structopt that truncates a trailing . in the help messages. For this reason, I wrapped each trailing period with [.]. This is temporary. I am going to file an issue and try and get together a PR fixing the issue. Resolved here

The migration to structopt, in and of itself, has value. The method of defining the cli is cleaner and it makes parsing the values drastically simpler. No more unwrapping parsed values because we know they got past a specific validator.

structopt is widely used and well maintained, but this next part introduces two new crates that still need some development around edge cases, testing, and documentation. partial_derive and configopt are implemented here, but that repo is primarily a proof of concept and contains no real docs or examples. Instead, I will describe the objectives of these two crates below.

`partial_derive`

Partial is a custom derive macro that generates a new struct type that wraps every field of the original struct in an Option. So code like

#[derive(Partial)]
struct Test {
    field1: u64,
    x: String,
}

would a generate something like

struct PartialTest {
    field1: Option<u64>,
    x: Option<String>,
}

It also generates several methods for creating, merging, and patching with "partials". I will have to add documentation.

The purpose of Partial is to let us represent an arbitrary subset of a struct. In our case, the struct is what we derived StructOpt on namely SubSupRun and the generated type PartialSubSupRun. SubSupRun represents a complete configuration and PartialSubSupRun a partial configuration.

We derive serde's Deserialize trait for PartialSubSupRun. This allows us to construct a PartialSubSupRun from a variety of bytes (http request, file on disk...) and in every format serde supports. There are two ways I can see using PartialSubSupRun. The first is to set default values for the cli. The next section will explain how that is accomplished. The second is to "patch" an existing SubSupRun instance. This would allow a user to update the Supervisor's configuration while it is running with a command like hab sup config patch. This second use case is not implemented.

`configopt`

configopt lets us dynamically set the defaults of a clap App with the function:

fn set_defaults(app: &mut App<'_, 'static>, defaults: &impl ConfigOptDefaults)

You will notice the defaults are pulled from a type that implements the ConfigOptDefaults trait. The Partial macro also automatically derives this trait for PartialSubSupRun.

To put it all together.

We create a clap app with SubSupRun::clap() (a function provided by deriving StructOpt)
Create a PartialSubSupRun by parsing a config file
Call set_defaults with the clap app and the parsed PartialSubSupRun

Roadmap

There is a lot of finishing work and cool/nice to have features that could be added. My thoughts on a roadmap would look like this:

Flush out the partial_derive crate (8)
Flush out the configopt crate (8)
~~Fix the structopt trailing . bug (3)~~ Resolved here
Convert all cli arguments to be defined with structopt (8)
Create and use the structopt struct for cli parsing instead of matches.vaue_of("ARG_NAME") (8?)
Create a special cli argument (--config-file) that is parsed before other arguments and used as a config file (3)
Pull config files from a known location such as ~/.hab/config-files (3)
Create a hab sup run cli argument that writes the current configuration to a config file (3). [I could see this being very useful. A user could manually tune how they want the supervisor configured write it to disk and then copy that file to multiple machines changing all of their configurations.]
Make the instance of the structopt stuct a singleton that the Supervisor looks to for making dynamic changes to its configuration (40)
Add a command for patching the configuration of a running Supervisor (3)

Thoughts on the overall approach? Does it feel over-engineered? If it seems like a good path forward I can create an issue for the items in the roadmap. Are there other parts of the app that we would want config file support besides hab sup run? This approach should make it straightforward to add.

jeremymv2 · 2020-01-24T16:32:16Z

Are there other parts of the app that we would want config file support besides hab sup run?

If one of the problems we're seeking to alleviate relates to the complexity and error prone aspects surrounding the bootstrapping and launching of a Habitat supervisor and services, then I think we should also entertain the idea of simplifying the loading of services as part of a holistic solution.

Automating the loading services has always been hindered by the fact that it's a three step process: 1.) launch the Supervisor 2.) wait until it is running and ready for communication 3.) load the services. In the field I've seen this implemented over and over and in many different ways, almost always involving sleep calls. I know that we have the service spec files that could be leveraged in automation, however, we've never advertised these as an official API of sorts. The service spec filesystem path is also only created after the Supervisor initially launches.

I'd like to see the 3 step process above simplified. I'm not sure the single config file is the right solution.. but I think the above service load concerns could potentially be solved elegantly by allowing a user to define all service load aspects in the same config file such that a simple hab sup run will not only launch the Supervisor with its configuration, but then also ensure the services are loaded with the correct load parameters as well.

davidMcneil · 2020-01-24T17:39:31Z

@jeremymv2 Completly agree that automating loading services would be awesome!

Currently, this work is focused on allowing configuration that can be specified at the command line also be specified in a file. What is cool is if we make it so that auto-loading services at startup can be expressed through the cli it will automatically be able to be expressed in a config file. I think that is kind of a big "if" though. It seems like you might want more expressivity than what is possible on the command line. I definitely think it is worth opening up an issue to explore this more.

christophermaier

So far, this looks like it could be a very promising approach. The fact that it unlocks dynamic updating of configuration is really the killer feature for me; there is a lot of stuff in the configuration of the Supervisor (and of individual services, for that matter) that we really should be able to change on-the-fly, without requiring restarts.

Having a better way to deal with how to load services would be nice, and this might also pave the way for that, too. I think that could use some dedicated research of its own, though. (Here's a thought, though... if we captured what's now in a Service spec in a structopt struct, maybe that allows us to both formalize the spec contract, and provide more flexibility in how users get that information to us?)

Nice work!

christophermaier · 2020-01-24T19:51:16Z

components/hab/src/cli.rs

+            usage = "hab sup run [FLAGS] [OPTIONS] [--] [PKG_IDENT_OR_ARTIFACT]"
+        )]
+#[allow(dead_code)]
+struct SubSupRun {


One of the issues I have had with hab sup run is that it has its own options, but also takes the options of hab svc load as well, and sometimes that's confusing to users, but also to developers maintaining those two commands.

Is there a way that we can define a struct that has the purely hab sup run options, and another that has the hab svc load commands and then somehow derive a merged struct from the combination that could be passed to hab sup run?

(Maybe not that, concretely, but some way to more formally separate those two concerns...)

Yep, I think structopt's flattening feature allows exactly that.

Ooooh, that looks like just the thing.

christophermaier · 2020-01-24T19:55:35Z

components/hab/src/cli.rs

+    /// defaults to using `127.0.0.1`[.]
+    #[structopt(name = "SYS_IP_ADDRESS", long = "sys-ip-address")]
+    sys_ip_address: Option<Ipv4Addr>,
+}


I've never been a huge fan of Clap's macro syntax for this project (just because Habitat's CLI is so big, not anything inherent to the macros themselves), I do like defining the CLI using functions that can be composed, since that makes it easier (or rather, would make it easier, if we fully implemented this) to define common CLI options in one place, and then easily reuse them in different commands.

I now realize this is a variation on my hab sup run / hab svc load configuration comment above, but are there ways we could achieve that kind of reuse in this structopt paradigm? Or will we have to continue to manually maintain identical options on a variety of configuration structs?

I think structopt's flattening feature was intended to address this issue exactly. Instead of the unit of composition being functions, it would now be struct definitions.

What is your opinion of structopt? I am kinda of proposing we switch all cli arg definitions to use it. Does that seem sane? Personally, I am a huge fan of it, but would like to make sure others are on board 😃.

Might get weird if we have to have a bunch of one-field structs (at the extreme end of things), but there are probably worse things.

Our CLI definitions have historically been a pain point, so I'm absolutely in favor of anything that helps bring sanity to it. It seems like this could do that, in addition to unlocking some nice functionality elsewhere in the codebase.

I'd love to see where this leads

I think the direction you're headed with structopt is spot on, even more-so with the advent of the flattening feature @davidMcneil!

christophermaier · 2020-01-24T20:01:26Z

components/hab/src/cli.rs

    #[structopt(name = "CACHE_KEY_PATH",
                long = "cache-key-path",
                env = CACHE_KEY_PATH_ENV_VAR,
                default_value = CACHE_KEY_PATH,
                hide_default_value = true)]
-    // TODO (DM): This could probably be a different type for better validation (PathBuf?)
-    cache_key_path: String,
+    cache_key_path: PathBuf,


christophermaier · 2020-01-24T20:08:40Z

components/hab/src/cli.rs

@@ -184,15 +184,16 @@ struct SubSupRun {
                possible_values = &["standalone", "leader"])]
    topology: Option<habitat_sup_protocol::types::Topology>,
    /// The update strategy; [default: none] [values: none, at-once, rolling]
-    // TODO (DM): this should use possible_values = &["none", "at-once", "rolling"]
+    // TODO (DM): this should set a default_value and use possible_values = &["none", "at-once",
+    // "rolling"]


I really wish our update strategy was just at-once or rolling, with the none being handled by an Option. 😢

christophermaier · 2020-01-24T20:12:39Z

components/hab/src/cli.rs

+fn config_file_to_defaults(config_file: &str)
+                           -> Result<PartialSubSupRun, Box<dyn std::error::Error>> {
+    let contents = fs::read_to_string(config_file)?;
+    Ok(toml::from_str(&contents)?)


Nothing to change here (yet?), but I'm sure somebody is going to take issue with TOML as a format (users have done this already in other parts of the code). Since this is all serde under the hood, we could probably pretty easily accept config in TOML, JSON, YAML, or whatever.

We can just wait until somebody asks, though, but the fact that it'd be easy to change is nice 😄

mwrock · 2020-01-25T00:40:22Z

Just to add some historical context for what it is worth. When the spec file was implemented for loading services, one of the key use cases was to be able to have a portable file that you could copy to other supervisors and "bootstrap" it with all the services that supervisor was intended to run. That use case clearly did not take off but I think with some added documentation, it still holds as a viable method.

kagarmoe · 2020-01-31T18:12:49Z

Could we extend this to output all the possible config options to a YAML/TOML/JSON file for docs to consume?

christophermaier · 2020-01-31T22:29:07Z

@kagarmoe I don't think that's something that's built into the code at the moment. Since all the information would be structured, though, it would probably be possible to write some additional code that could be used to generate documentation. Something like that would likely be a separate task, though.

davidMcneil · 2020-02-04T19:09:02Z

@kagarmoe That would be great! I created an issue to track outputting a structured representation of the cli.

davidMcneil · 2020-02-04T19:19:14Z

Unfortunately, structopt applies preprocessing to the help messages. As part of this preprocessing it removes a single period from the first paragraph of help messages. In order to keep the structop version in sync with the raw clap version, I removed these trailing periods from both implementations.

What are reviewers thoughts? Is it ok to remove these periods?

christophermaier · 2020-02-05T16:29:42Z

components/hab/src/cli.rs

+        let mut sub = SubSupRun::clap();
+        if let Ok(config_file) = env::var("HAB_FEAT_CONFIG_FILE") {
+            match config_file_to_defaults(&config_file) {
+                Ok(defaults) => configopt::set_defaults(&mut sub, &defaults),


This flow seems confusing... it seems like you're determining the CLI arguments first, and then grabbing configuration from the config file (if it exists) to... do something with default values underneath that?

I would have expected configuration file values to be read in first, and then overlaid with any CLI and / or environment overrides.

Am I overlooking something here?

Agreed, this is confusing. Actually, what you describe is exactly what is happening, "configuration file values to be read in first, and then overlaid with any CLI". What is confusing is how the overlay happens. The overlay happens by setting/overriding the defaults of the CLI args. So to go line by line:

1316: Creates a clap::App from the SubSupRun definition using structopt

1318: Reads the config file and converts it to a PartialSubSupRun which implements ConfigOptDefaults. ConfigOptDefaults is a trait that can be used to override the default values of a clap::App

1319: Set/override the cli defaults with values from the config file

We are now ready to parse the CLI input taking into account the newly set default values from the config file

The reason we need the song and dance of overriding the default values of the CLI (opposed to merging the CLI and config file after the parsing the CLI input) is to allow the user to not set values on the command line if they are set in the config file. If we did not add a default value parsing the CLI input would fail.

Once we switch to using structopt to parse the CLI opposed to clap's matches.vaue_of("ARG_NAME") this logic will not be necessary.

If it changes out the defaults while still allowing user-specified data to show through (and doesn't replace user-specified data with defaults!), that's cool. The fact that this goes away is also nice.

Adding some comments that clarify this situation would also be nice.

christophermaier · 2020-02-05T16:34:38Z

@davidMcneil What are your plans for your configopt crate? Move it over here? Publish it? I just want to make sure we've got a maintainable path forward for it.

davidMcneil · 2020-02-05T17:09:26Z

@davidMcneil What are your plans for your configopt crate? Move it over here? Publish it? I just want to make sure we've got a maintainable path forward for it.

I have a bit of cleanup/polish I want to do and then I would like to publish it. We could also move the repo over here.

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

bixu · 2020-02-11T21:54:18Z

I can’t tell you how many times I’ve toyed with the idea of volume mounting spec files, but always backed away from the idea, assuming it was an “unstable interface”.

I also second the motion for supporting YAML. Not because I think it’s better than TOML but because it’s familiar to the K8s Kids.

chef-expeditor bot assigned davidMcneil Jan 23, 2020

christophermaier reviewed Jan 24, 2020

View reviewed changes

davidMcneil force-pushed the dmcneil/config-file branch from 66bd87f to 01d7f89 Compare February 4, 2020 18:30

davidMcneil marked this pull request as ready for review February 4, 2020 18:57

davidMcneil requested review from chefsalim and eeyun as code owners February 4, 2020 18:57

christophermaier reviewed Feb 5, 2020

View reviewed changes

davidMcneil added 10 commits February 7, 2020 14:52

Add structopt for sup run

3ead9b3

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

Remove default message for http-disable

5bf4414

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

Remove extra default for listen-ctl

c0eb7b6

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

Add cli todos

ebcf67d

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

Add config file support

532584e

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

Update structopt

a10f428

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

Remove trailing periods in help

359ea43

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

Update dependencies

fa74e8a

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

Update configopt

1523f84

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

Add comments to config file parsing

29d76ec

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

davidMcneil force-pushed the dmcneil/config-file branch from 5a153ab to 29d76ec Compare February 7, 2020 20:30

Pull structopt definitions into seperate file

dce46e8

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

christophermaier approved these changes Feb 10, 2020

View reviewed changes

Refactor to reduce diff

586303a

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

davidMcneil added the X-feature label Feb 10, 2020

davidMcneil changed the title ~~Config file support~~ Experimental config file support behind HAB_FEAT_CONFIG_FILE feature flag Feb 10, 2020

davidMcneil merged commit ce69128 into master Feb 10, 2020

chef-expeditor bot deleted the dmcneil/config-file branch February 10, 2020 17:36

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental config file support behind `HAB_FEAT_CONFIG_FILE` feature flag #7360

Experimental config file support behind `HAB_FEAT_CONFIG_FILE` feature flag #7360

davidMcneil commented Jan 23, 2020 •

edited

jeremymv2 commented Jan 24, 2020 •

edited

davidMcneil commented Jan 24, 2020

christophermaier left a comment

christophermaier Jan 24, 2020

davidMcneil Jan 24, 2020

christophermaier Jan 24, 2020

jeremymv2 Jan 25, 2020

christophermaier Jan 24, 2020

davidMcneil Jan 24, 2020 •

edited

christophermaier Jan 24, 2020

jeremymv2 Jan 25, 2020 •

edited

christophermaier Jan 24, 2020

christophermaier Jan 24, 2020

christophermaier Jan 24, 2020

mwrock commented Jan 25, 2020

kagarmoe commented Jan 31, 2020 •

edited

christophermaier commented Jan 31, 2020

davidMcneil commented Feb 4, 2020

davidMcneil commented Feb 4, 2020

christophermaier Feb 5, 2020

davidMcneil Feb 5, 2020

christophermaier Feb 7, 2020

christophermaier commented Feb 5, 2020

davidMcneil commented Feb 5, 2020

bixu commented Feb 11, 2020

Experimental config file support behind HAB_FEAT_CONFIG_FILE feature flag #7360

Experimental config file support behind HAB_FEAT_CONFIG_FILE feature flag #7360

Conversation

davidMcneil commented Jan 23, 2020 • edited

How to use a config file

How does this work

partial_derive

configopt

Roadmap

jeremymv2 commented Jan 24, 2020 • edited

davidMcneil commented Jan 24, 2020

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

davidMcneil Jan 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymv2 Jan 25, 2020 • edited

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 Jan 25, 2020

kagarmoe commented Jan 31, 2020 • edited

christophermaier commented Jan 31, 2020

davidMcneil commented Feb 4, 2020

davidMcneil commented Feb 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophermaier commented Feb 5, 2020

davidMcneil commented Feb 5, 2020

bixu commented Feb 11, 2020

Experimental config file support behind `HAB_FEAT_CONFIG_FILE` feature flag #7360

Experimental config file support behind `HAB_FEAT_CONFIG_FILE` feature flag #7360

davidMcneil commented Jan 23, 2020 •

edited

`partial_derive`

`configopt`

jeremymv2 commented Jan 24, 2020 •

edited

davidMcneil Jan 24, 2020 •

edited

jeremymv2 Jan 25, 2020 •

edited

kagarmoe commented Jan 31, 2020 •

edited