Skip to content

Commit

Permalink
Add leader key to render context bind information
Browse files Browse the repository at this point in the history
Previously, determining the leader of a service group was
complicated. If the service was running in a leader topology, the
`{{bind.<BIND>.first}}` key would be the leader, but we didn't
directly indicate if that was the case (you could have queried to see
if `{{bind.<BIND>.first.leader}}` was `true`, but that leads to some
needlessly verbose templates.

Now, we introduce an explicit `leader` key, which will be non-null if
the service group has a leader. Furthermore, the use of `first` is
deprecated, since the meaning of it is not deterministic (it could be
a non-alive member of the service group, for instance, which is not
what anyone wants). The `first` key will remain in rendering contexts
for the foreseeable future, but users are encouraged to use
`{{bind.<BIND>.leader}}` if they want the leader, and
`{{bind.<BIND>.members[0]}}` otherwise, pending future refactorings.

Documentation is updated to reflect this, and a new `$deprecated` and
`$since` key are used in our JSON Schema to indicate when an old field
is deprecated, and when a new one is introduced. Our JSON Schema
parser is configured to allow non-standard keywords like this through
its processing, and this provides some additional hooks that a future
automated documentation generator can use (see #4824)

Fixes #4127

Signed-off-by: Christopher Maier <cmaier@chef.io>
  • Loading branch information
christophermaier committed Apr 12, 2018
1 parent 9d7db22 commit f2e4467
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 1 deletion.
10 changes: 10 additions & 0 deletions components/sup/doc/render_context_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -498,8 +498,17 @@
"properties": {
"first": {
"description": "The first member of this service group. If the group is running in a leader topology, this will also be the leader.",
"$deprecated": "Since 0.56.0; if you want the leader, use `leader` explicitly. 'first' isn't deterministic, either, so you can just use `members[0]` instead",
"$ref": "#/definitions/svc_member"
},
"leader": {
"description": "The current leader of this service group, if running in a leader topology",
"$since": "0.56.0",
"oneOf": [
{ "$ref": "#/definitions/svc_member" },
{ "type": "null" }
]
},
"members": {
"description": "All members of the service group, across the entire ring. Includes all liveness states!",
"type": "array",
Expand All @@ -510,6 +519,7 @@
},
"required": [
"first",
"leader",
"members"
],
"additionalProperties": false
Expand Down
64 changes: 63 additions & 1 deletion components/sup/src/templating/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,13 +417,15 @@ impl<'a> Binds<'a> {
#[derive(Clone, Debug, Serialize)]
struct BindGroup<'a> {
first: Option<SvcMember<'a>>,
leader: Option<SvcMember<'a>>,
members: Vec<SvcMember<'a>>,
}

impl<'a> BindGroup<'a> {
fn new(group: &'a CensusGroup) -> Self {
BindGroup {
first: select_first(group),
leader: group.leader().map(|m| SvcMember::from_census_member(m)),
members: group
.members()
.iter()
Expand Down Expand Up @@ -627,6 +629,7 @@ mod tests {

use manager::service::Cfg;
use manager::service::config::PackageConfigPaths;
use templating::TemplateRenderer;

/// Asserts that `json_string` is valid according to our render
/// context JSON schema.
Expand Down Expand Up @@ -674,7 +677,8 @@ JSON:
serde_json::from_str(&raw_schema).expect("Could not parse schema as JSON");
let mut scope = json_schema::scope::Scope::new();
// NOTE: using `false` instead of `true` allows us to use
// `$comment` keys
// `$comment` keyword, as well as our own `$deprecated` and
// `$since` keywords.
let schema = scope.compile_and_return(parsed_schema, false).expect(
"Could not compile the schema",
);
Expand Down Expand Up @@ -870,6 +874,7 @@ two = 2
let mut bind_map = HashMap::new();
let bind_group = BindGroup {
first: Some(me.clone()),
leader: None,
members: vec![me.clone()],
};
bind_map.insert("foo".into(), bind_group);
Expand All @@ -884,6 +889,20 @@ two = 2
}
}

/// Render the given template string using the given context,
/// returning the result. This can help to verify that
/// RenderContext data are accessible to users in the way we
/// expect.
fn render(template_content: &str, ctx: &RenderContext) -> String {
let mut renderer = TemplateRenderer::new();
renderer
.register_template_string("testing", template_content)
.expect("Could not register template content");
renderer.render("testing", ctx).expect(
"Could not render template",
)
}

////////////////////////////////////////////////////////////////////////

/// Reads a file containing real rendering context output from an
Expand Down Expand Up @@ -931,4 +950,47 @@ two = 2
assert_valid(&j);
}

#[test]
fn no_leader_renders_correctly() {
let ctx = default_render_context();

// Just make sure our default context is set up how this test
// is expecting
assert!(ctx.bind.0.get("foo").unwrap().leader.is_none());

let output = render(
"{{#if bind.foo.leader}}THERE IS A LEADER{{else}}NO LEADER{{/if}}",
&ctx,
);

assert_eq!(output, "NO LEADER");
}

#[test]
fn leader_renders_correctly() {
let mut ctx = default_render_context();

// Let's create a new leader, with a custom member_id
let mut svc_member = default_svc_member();
svc_member.member_id = Cow::Owned("deadbeefdeadbeefdeadbeefdeadbeef".into());

// Set up our own bind with a leader
let mut bind_map = HashMap::new();
let bind_group = BindGroup {
first: Some(svc_member.clone()),
leader: Some(svc_member.clone()),
members: vec![svc_member.clone()],
};
bind_map.insert("foo".into(), bind_group);
let binds = Binds(bind_map);
ctx.bind = binds;

// This template should reveal the member_id of the leader
let output = render(
"{{#if bind.foo.leader}}{{bind.foo.leader.member_id}}{{else}}NO LEADER{{/if}}",
&ctx,
);

assert_eq!(output, "deadbeefdeadbeefdeadbeefdeadbeef");
}
}
1 change: 1 addition & 0 deletions components/sup/tests/fixtures/sample_render_context.json
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@
"update_follower": false,
"update_leader": false
},
"leader": null,
"members": [
{
"alive": true,
Expand Down

0 comments on commit f2e4467

Please sign in to comment.