Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the init hook async #7111

Merged
merged 10 commits into from Oct 25, 2019
Merged

Make the init hook async #7111

merged 10 commits into from Oct 25, 2019

Conversation

@davidMcneil
Copy link
Member

davidMcneil commented Oct 25, 2019

Resolves #5954

Questions for Reviewers

TODO

  • Add end to end tests
davidMcneil added 7 commits Oct 24, 2019
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
@davidMcneil davidMcneil force-pushed the dmcneil/async-init-hook branch from 7efa23d to b1ce8fa Oct 25, 2019
@@ -1217,7 +1252,6 @@ impl<'a> Serialize for ServiceProxy<'a> {
strukt.serialize_field("desired_state", &s.desired_state)?;
strukt.serialize_field("health_check", &s.health_check_result)?;
strukt.serialize_field("hooks", &s.hooks)?;
strukt.serialize_field("initialized", &s.initialized)?;

This comment has been minimized.

Copy link
@davidMcneil

davidMcneil Oct 25, 2019

Author Member

Is this needed? It looks as if we just check if the process is up and use that to determine if the process was initialized.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Oct 25, 2019

Contributor

Yes and no ☹️

The fact that this field is here at all is because waaaaay back in the beginning, the HTTP gateway basically made direct JSON serializations of internal datastructures available to users. I can't really think of a legitimate use for this field in such a situation, but it is available to users.

For additional background, check out #5689 as well as its spiritual predecessor #4823. At some point I'd like to do something like #4872, but for the HTTP gateway data.

For now, though, to preserve existing behavior, this should turn into something like

strukt.serialize_field("initialized", &s.state.initialized())?;
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
Copy link
Contributor

christophermaier left a comment

This is great! The sequence of commits made it incredibly easy to follow.

I'm pleased this turned out to be so straightforward!

enum ServiceState {
Uninitialized,
Initialized,
}

This comment has been minimized.

Copy link
@christophermaier

christophermaier Oct 25, 2019

Contributor

I love enums instead of bools... it's so much more descriptive!

@@ -1217,7 +1252,6 @@ impl<'a> Serialize for ServiceProxy<'a> {
strukt.serialize_field("desired_state", &s.desired_state)?;
strukt.serialize_field("health_check", &s.health_check_result)?;
strukt.serialize_field("hooks", &s.hooks)?;
strukt.serialize_field("initialized", &s.initialized)?;

This comment has been minimized.

Copy link
@christophermaier

christophermaier Oct 25, 2019

Contributor

Yes and no ☹️

The fact that this field is here at all is because waaaaay back in the beginning, the HTTP gateway basically made direct JSON serializations of internal datastructures available to users. I can't really think of a legitimate use for this field in such a situation, but it is available to users.

For additional background, check out #5689 as well as its spiritual predecessor #4823. At some point I'd like to do something like #4872, but for the HTTP gateway data.

For now, though, to preserve existing behavior, this should turn into something like

strukt.serialize_field("initialized", &s.state.initialized())?;
@@ -276,7 +277,8 @@ impl Service {
last_election_status: ElectionStatus::None,
user_config_updated: false,
needs_restart: false,
initialization_state: InitializationState::Uninitialized,
initialization_state:
Arc::new(RwLock::new(InitializationState::Uninitialized)),

This comment has been minimized.

Copy link
@christophermaier

christophermaier Oct 25, 2019

Contributor

🤔 Wonder if it'd be worth implementing Default for InitializationState...

self.service_group.clone(),
self.pkg.clone(),
self.svc_encrypted_password.clone());
// These clones are unfortunate. async/await will make this much better.

This comment has been minimized.

Copy link
@christophermaier
Signed-off-by: David McNeil <mcneil.david2@gmail.com>
@mwrock
mwrock approved these changes Oct 25, 2019
Copy link
Contributor

mwrock left a comment

Looks good to me. Very excited about this!

Signed-off-by: David McNeil <mcneil.david2@gmail.com>
@davidMcneil davidMcneil merged commit 9c6ae7d into master Oct 25, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3931 passed (38 minutes, 27 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #1011 passed (42 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@chef-expeditor chef-expeditor bot deleted the dmcneil/async-init-hook branch Oct 25, 2019
stevendanna added a commit to stevendanna/habitat that referenced this pull request Oct 28, 2019
I believe this broke in habitat-sh#7111. For services with no run hook, hab-sup
would wait forever for the service to enter the InitializerFinished
state.

Signed-off-by: Steven Danna <steve@chef.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.