Refactor ServiceConfig into RenderContext in supervisor #2226

reset · 2017-05-04T17:01:38Z

Refactor ServiceConfig into RenderContext in Habitat Supervisor

The spirit of this commit is to improve reliability when compiling templated
plan artifacts such as configuration files and hooks. We accomplish this by
adding a RenderContext module which is used in the templating module to
bind data to templates. This should improve overall readability of our code
and also ensure that we have a typed structure when deserializing data from
handlebars in the templating::helpers module. We leverage a zero-copy
deserialization implementation in RenderContext to ensure that we don't
duplicate data for each helper in a template.

Add templating::context module and RenderContext struct. This is what
is sent to our templating engine to be bound to all templates.
Add manager::sys module and Sys struct. This represents the system as
a whole and is eventually serialized in the RenderContext as sys
- Move http_listen to Sys struct from Manager struct
- Move gossip_listen to Sys struct Manager struct
Add Arc<Sys> to Service struct. This holds a read only reference to
the Sys struct of the Manager which loaded it. This ensures that we
only allocate one Sys struct which doesn't change unless the Supervisor
is restarted
Rewrote manager::service::config module and added Cfg struct to it.
This contains all configuration logic for the service and is passed
to the RenderContext in the same way as the Sys struct
Add manager::service::package module and Pkg struct to it. This is
similar to both the Sys and Cfg modules in it's relation with
RenderContext. It contains all information about the package the service
is running
Moved supervisor module to manager::service::supervisor
Fix serialization/deserialization issues with SysInfo struct
Fix API for crypto::hash - only return a Result if there is actually
an error case
Move command execution functions from the util module to
manager::service::exec. The create_command function from that module has
been renamed to run_cmd and now accepts a reference to a Pkg struct
which will ensure that all commands are executed within the context of the
service's package
Remove gossip.toml and config.toml cache files from service directory.
We no longer need to write these out since we now persist and reload our
entire rumor store on supervisor boot.
Improve error when loading a bad package from a package install
Upgrade completely to serde 1.0+
- Temporarily switch to git source for libsodium dependency
Replace "Butterfly" in supervisor output with "Supervisor" or another more
appropriate description which hides our implementation details
Add function and module documentation

thesentinels · 2017-05-04T17:01:39Z

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!

bodymindarts

Excelent work! Some minor things.

bodymindarts · 2017-05-05T08:42:30Z

components/sup/src/lib.rs

@@ -289,7 +289,6 @@ pub mod fs;
 pub mod http_gateway;
 pub mod manager;
 pub mod output;
-pub mod supervisor;


I like this a lot!

bodymindarts · 2017-05-05T08:43:36Z

components/sup/src/manager/mod.rs

@@ -48,16 +49,16 @@ use serde;
 use serde_json;
 use time::{self, Timespec, Duration as TimeDuration};

-pub use manager::service::{Service, ServiceConfig, ServiceSpec, UpdateStrategy, Topology};
-use self::service::{DesiredState, StartStyle};
+pub use self::service::{Service, ServiceSpec, UpdateStrategy, Topology};


should we organize the pub use imports together, perhaps at the bottom of the use declarations. This would improve visibility into what is actually being exported like we put all the pub fields at the top of a struct.

The way I've organized these are

Imports from std

Public imports from extern

Imports from extern

Public imports from our own crate

Imports from our own crate

I chose this since the rest of the code is formatted (or should be... we really need to commit some rules to the wiki!)

Imports

Public Const

Private Const

Public Static

Private Static

Public Trait

Private Trait

Public Struct

Private Struct

Public Function

Private Function

We've got a few other small things in there like recently organizing members of the struct into public/private sections and organization of functions within the struct itself.

If you think we can come up with something more readable then let's go ahead and do it. Maybe it should be:

Imports from std

Public imports from extern

Public Imports from our own crate

Imports from extern

Imports from our own crate

Okay this isn't documented nor adhered to consistently. I'm fine leaving it as is but we need to start a style guide to communicate this.

Yeah we really do, I'll put that on my list of things to do for Friday

I guess this was part of our unspoken conventions that we're taking from Aaron Turon's Rust Guidelines book.

bodymindarts · 2017-05-05T08:53:45Z

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

@@ -144,7 +141,7 @@ pub trait Hook: fmt::Debug + Sized {

    fn path(&self) -> &Path;

-    fn template(&self) -> &Template;
+    fn registry(&self) -> &Registry;


Not really licking the name Registry when I read this code (the whole file). Registry is a very generic name that (for me) doesn't really give me any useful information about what it actually does.
I guess its a kind of container but perhaps we could be more specific (HookContainer), but then it also has behaviour (later on I see registry.render() so perhaps its a kind of HookRenderer. I don't know but currently it doesn't read nicely to me.

Yeah I was a 2/10 on this name but it was a hell of a lot better than Template which is what is actually being registered to this thing. I was also thinking Renderer so maybe Template was a bad name but TemplateRenderer is spot on. I'll make that change

bodymindarts · 2017-05-05T08:54:34Z

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

 use std::fmt;
 use std::fs::File;
 use std::io::BufWriter;
 use std::io::prelude::*;
 use std::path::{Path, PathBuf};
 use std::result;
 use std::str::FromStr;
-use std::sync::{Arc, RwLock, RwLockReadGuard};
+use std::sync::Arc;


Cool that this is simplified.

bodymindarts · 2017-05-05T09:04:40Z

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

    pub depot_url: String,
    pub spec_file: PathBuf,
    pub spec_ident: PackageIdent,
    pub start_style: StartStyle,
    pub topology: Topology,
    pub update_strategy: UpdateStrategy,
+    pub cfg: Cfg,
+    pub pkg: Pkg,
+    pub sys: Arc<Sys>,


Why is Sys behind an Arc? Shouldn't it be immutable after start up? Or is this just to save memory instead of cloning?

Ya this is just to save on memory instead of cloning. An Arc is basically a read-only thread safe pointer. This is totally immutable ;)

bodymindarts · 2017-05-05T09:10:09Z

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

                    ElectionStatus::None => {
-                        if self.last_election_status != *current_election_status {
+                        if self.last_election_status != census_group.topology_election_status {


nice improvement

bodymindarts · 2017-05-05T09:11:50Z

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

-        if let Some(err) = self.copy_run().err() {
-            outputln!(preamble self.service_group, "Failed to copy run hook: {}", err);
+        {
+            let ctx = self.render_context(&census_ring);


If this can be avoided and run in the update_configuration method I would find that much better.
Perhaps there could be code like:

if !self.initialized { ... always render everything return }

in the update_configuration method.

Yeah you're right, this actually can completely be avoided. It's already being called in the update_templates function that I've created from refactored bits of update_configuration. Gonna make those changes and push them up!

bodymindarts · 2017-05-05T09:12:30Z

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

        if !self.initialized {
-            self.initialize();
+            self.initialize(census_ring);


Already commented on this.

bodymindarts · 2017-05-05T09:14:53Z

components/sup/src/templating/context.rs

+}
+
+#[derive(Clone, Debug, Serialize)]
+pub struct BindGroup;


Empty? I guess this is WIP.

bodymindarts · 2017-05-05T09:16:58Z

components/sup/src/templating/mod.rs


-impl Template {
+impl Registry {


Already commented on this from the POV of the client code in hooks.rs

fnichol

This is looking pretty good to me!

fnichol · 2017-05-05T21:26:26Z

Cargo.lock

- "libsodium-sys 0.0.14 (registry+https://github.com/rust-lang/crates.io-index)",
- "serde 0.9.15 (registry+https://github.com/rust-lang/crates.io-index)",
+ "libsodium-sys 0.0.14 (git+https://github.com/dnaq/sodiumoxide)",
+ "serde 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)",


Wow, gotta keep up, right?

fnichol · 2017-05-05T21:28:06Z

components/butterfly/src/rumor/service.rs

 pub struct SysInfo {
-    pub ip: String,
+    pub ip: IpAddr,


fnichol · 2017-05-05T21:28:57Z

components/core/src/crypto/hash.rs

    let mut reader = BufReader::new(file);
    hash_reader(&mut reader)
 }

-pub fn hash_string(data: &str) -> Result<String> {
+pub fn hash_string(data: &str) -> String {


That's funny, I wonder if there used to be a possible error here?

fnichol · 2017-05-05T21:29:42Z

components/sup/src/census.rs

 pub struct CensusMember {
    pub member_id: MemberId,
    pub pkg: Option<PackageIdent>,

    service: String,
    group: String,
    org: Option<String>,
-    cfg: toml::value::Table,
+    #[serde(


That's kickass!

fnichol · 2017-05-05T21:31:10Z

components/sup/src/main.rs

@@ -51,8 +51,7 @@ use sup::command;
 use sup::http_gateway;
 use sup::manager::{Manager, ManagerConfig, ServiceStatus};
 use sup::manager::service::{DesiredState, ServiceBind, Topology, UpdateStrategy};
-use sup::manager::service::{ServiceSpec, StartStyle};
-use sup::supervisor::ProcessState;
+use sup::manager::service::{ProcessState, ServiceSpec, StartStyle};


That's probably a good sign!

fnichol · 2017-05-05T21:33:32Z

components/sup/src/manager/mod.rs

@@ -48,16 +49,16 @@ use serde;
 use serde_json;
 use time::{self, Timespec, Duration as TimeDuration};

-pub use manager::service::{Service, ServiceConfig, ServiceSpec, UpdateStrategy, Topology};
-use self::service::{DesiredState, StartStyle};
+pub use self::service::{Service, ServiceSpec, UpdateStrategy, Topology};


I guess this was part of our unspoken conventions that we're taking from Aaron Turon's Rust Guidelines book.

fnichol · 2017-05-05T21:34:08Z

components/sup/src/manager/mod.rs

                                            member,
                                            Trace::default(),
                                            ring_key,
                                            None,
                                            Some(&fs_cfg.data_path),
                                            Box::new(SuitabilityLookup(services.clone())))?;
-        outputln!("Butterfly Member ID {}", server.member_id());
+        outputln!("Supervisor Member-ID {}", sys.member_id);


That's a nice tweak and more clearly communicate with the extra internal jargon.

fnichol · 2017-05-05T21:34:42Z

components/sup/src/manager/mod.rs

+        member.set_persistent(sys.permanent);
+        // JW TODO: Should we really be setting these here? Seems like they should be auto set
+        // by the butterfly server constructor
+        member.set_swim_port(sys.gossip_port as i32);


That is a bit surprising, but works for the moment!

mwrock · 2017-05-05T22:10:54Z

components/core/src/error.rs

@@ -138,7 +139,9 @@ impl fmt::Display for Error {
                format!("Invalid keypath: {}. Specify an absolute path to a file on disk.",
                        e)
            }
-            Error::ConfigFileIO(ref e) => format!("Error reading configuration file: {}", e),
+            Error::ConfigFileIO(ref f, ref e) => {
+                format!("Error reading configuration file, {}, {}", f.display(), e)


nice. Always a bummer when you have no idea what file the error is about

mwrock · 2017-05-05T22:34:36Z

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

    pub preamble: String,
    pub state: ProcessState,
    pub state_entered: Timespec,
    pub has_started: bool,
-    pub runtime_config: RuntimeConfig,
+    pid: Option<PathBuf>,


I don't see anywhere that sets this except where we ser it to None when removing the pid file.

We use this in create_pidfile to record what the path is before we delete it in cleanup_pidfile. After we do that, we set it to None

that function uses child.id() (as it should) I do not see any use of self.pid there. I might just be being my usual dense self here.

It's on line 214, 224, and 231

oh totally missed 214. pid_file_path might be a better name but not a huge deal.

mwrock · 2017-05-05T22:47:36Z

components/sup/src/templating/helpers.rs

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


great move to separate out the helpers.

mwrock · 2017-05-05T23:23:44Z

components/sup/src/manager/mod.rs

-            if self.updater
-                   .check_for_updated_package(service, &self.census_ring) {
-                service.populate(&self.census_ring);
-                self.gossip_latest_service_rumor(&service);


hmm. did you find this gossiping was not necessary? I'd think it would be something you would want to rumor out but maybe something else takes care of the rumor. Just curious about the rationale.

Yep! This should be completely unnecessary. We are going to call tick() pretty much right after this on the service which is going to tell us that it updated. Once it does, we will call gossip_latest_service_rumor() as a very next step

ahh that makes sense

mwrock · 2017-05-05T23:55:02Z

Oh just found a bug here that kills status deserialization. I can add a fix to this branch.

reset · 2017-05-06T00:09:39Z

@mwrock the has_{} field is completely gone. We have enough power with the handlebars templating language that we don't need it anymore (prior we were using just mustache). You can do things like this now:

{{~#eachAlive bind.backend.members as |member|}}
    server {{member.sys.ip}} {{member.sys.ip}}:{{member.cfg.port}}
{{~else}}
    server {{sys.ip}} {{sys.ip}}:8080
{{~/eachAlive}}

You must use the block syntax (as |x|) when using any version of each. It was a bug before that we inherited the outer context there. If we want that functionality then we need to write a withAlive helper

The spirit of this commit is to improve reliability when compiling templated plan artifacts such as configuration files and hooks. We accomplish this by adding a `RenderContext` module which is used in the templating module to bind data to templates. This should improve overall readability of our code and also ensure that we have a typed structure when deserializing data from handlebars in the `templating::helpers` module. We leverage a zero-copy deserialization implementation in `RenderContext` to ensure that we don't duplicate data for each helper in a template. * Add `templating::context` module and `RenderContext` struct. This is what is sent to our templating engine to be bound to all templates. * Add `manager::sys` module and `Sys` struct. This represents the system as a whole and is eventually serialized in the RenderContext as `sys` * Move http_listen to `Sys` struct from `Manager` struct * Move gossip_listen to `Sys` struct `Manager` struct * Add `Arc<Sys>` to `Service` struct. This holds a read only reference to the `Sys` struct of the `Manager` which loaded it. This ensures that we only allocate one `Sys` struct which doesn't change unless the Supervisor is restarted * Rewrote `manager::service::config` module and added `Cfg` struct to it. This contains all configuration logic for the service and is passed to the `RenderContext` in the same way as the `Sys` struct * Add `manager::service::package` module and `Pkg` struct to it. This is similar to both the `Sys` and `Cfg` modules in it's relation with `RenderContext`. It contains all information about the package the service is running * Moved `supervisor` module to `manager::service::supervisor` * Fix serialization/deserialization issues with SysInfo struct * Fix API for `crypto::hash` - only return a `Result` if there is actually an error case * Move command execution functions from the `util` module to `manager::service::exec`. The `create_command` function from that module has been renamed to `run_cmd` and now accepts a reference to a `Pkg` struct which will ensure that all commands are executed within the context of the service's package * Remove `gossip.toml` and `config.toml` cache files from service directory. We no longer need to write these out since we now persist and reload our entire rumor store on supervisor boot. * Improve error when loading a bad package from a package install * Upgrade completely to serde 1.0+ * Temporarily switch to git source for libsodium dependency * Replace "Butterfly" in supervisor output with "Supervisor" or another more appropriate description which hides our implementation details * Add function and module documentation Signed-off-by: Jamie Winsor <jamie@vialstudios.com>

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

mwrock · 2017-05-06T01:59:51Z

added a fix for the deserialization bug. Will unleash sentinals when green

mwrock · 2017-05-06T02:12:08Z

@thesentinels approve

thesentinels · 2017-05-06T02:12:09Z

🤘 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-05-06T02:12:21Z

Travis CI has started testing this PR.

thesentinels · 2017-05-06T02:15:51Z

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

reset added Bug labels May 4, 2017

reset force-pushed the serialization-bugs branch 10 times, most recently from e817035 to b0670d9 Compare May 5, 2017 07:34

bodymindarts reviewed May 5, 2017

View reviewed changes

reset force-pushed the serialization-bugs branch 2 times, most recently from 269bb72 to 95c085d Compare May 5, 2017 10:03

reset added this to the Pokemon Village milestone May 5, 2017

reset force-pushed the serialization-bugs branch 3 times, most recently from ffe4de7 to ce6f986 Compare May 5, 2017 13:19

reset removed the DO NOT MERGE label May 5, 2017

reset force-pushed the serialization-bugs branch from ce6f986 to 0e59e94 Compare May 5, 2017 21:05

fnichol approved these changes May 5, 2017

View reviewed changes

mwrock reviewed May 5, 2017

View reviewed changes

reset force-pushed the serialization-bugs branch from 0e59e94 to 5b9290a Compare May 5, 2017 23:07

mwrock reviewed May 5, 2017

View reviewed changes

reset force-pushed the serialization-bugs branch from f841d41 to 14dc355 Compare May 5, 2017 23:48

reset force-pushed the serialization-bugs branch from 14dc355 to 9609219 Compare May 6, 2017 00:05

reset force-pushed the serialization-bugs branch 2 times, most recently from eff057c to 7f5dcf4 Compare May 6, 2017 00:50

reset force-pushed the serialization-bugs branch from 7f5dcf4 to 341d6cc Compare May 6, 2017 01:13

fix serialization/deserialization of pkg to work with status

31374a9

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

thesentinels merged commit cf3f0eb into master May 6, 2017

thesentinels deleted the serialization-bugs branch May 6, 2017 02:15

mwrock mentioned this pull request May 8, 2017

{{ has_<bind>}} removed #2257

Closed

reset mentioned this pull request May 8, 2017

Service bind data does not contain service group members within the same supervisor #2150

Closed

eeyun added A-supervisor and removed Supervisor labels Jun 6, 2017

christophermaier added Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Type: Bug Issues that describe broken functionality Type: Chore Issues for general code and infrastructure maintenance and removed A-supervisor labels Jul 24, 2020

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

Refactor ServiceConfig into RenderContext in supervisor #2226

Refactor ServiceConfig into RenderContext in supervisor #2226

Conversation

reset commented May 4, 2017 • edited Loading

thesentinels commented May 4, 2017

bodymindarts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reset May 5, 2017 • edited Loading

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

fnichol 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

mwrock commented May 5, 2017

reset commented May 6, 2017

mwrock commented May 6, 2017

mwrock commented May 6, 2017

thesentinels commented May 6, 2017

thesentinels commented May 6, 2017

thesentinels commented May 6, 2017

reset commented May 4, 2017 •

edited

Loading

reset May 5, 2017 •

edited

Loading