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

Adds service update started event #6611

Merged
merged 4 commits into from Jun 6, 2019

Conversation

@gpeers
Copy link
Contributor

commented Jun 3, 2019

This PR fires a service update started event when the supervisor sees that a new hab package is available and thus starts the package update process. Automate uses this event to help determine and display the deployment status of a service.

Note: there's work in progress to refactor the supervisor to perform certain operations, including starting packages, asynchronously. Until this work is done, providing additional deployment events (service update completed, service update failed) is awkward because we don't have a concrete handle on when these events occur without adding more state as a stopgap. I'm thinking it might be best to let Automate derive completed and failure states on its side until we get the supervisor refactored and can implement additional events in the correct way. One possible way Automate might accomplish this would be to compare the update fully-qualified package identifier from the service update started event with the fully-qualified package identifier from the most recent service started event for a given service. If they match, Automate could assume a successful update. If this is not the case (and a specified period of time has passed), Automate could assume an update failure.

There are nuances to this we'd need to discuss, e.g., multiple entities trying to update the same service. Additionally, depending upon the level of effort on the Automate side, we may wish to revisit adding state on the Habitat side, bumping up the priority of the supervisor work, etc. Another option would be just using the service update started event on the Automate side without trying to derive any additional information. This needs discussion with the Automate team and Product.

Shake 'n' Bake!
image

Signed-off-by: Gina Peers gpeers@chef.io

adds service update started event
Signed-off-by: Gina Peers <gpeers@chef.io>
@chef-expeditor

This comment has been minimized.

Copy link

commented Jun 3, 2019

Hello gpeers! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

string update_origin = 3;
string update_name = 4;
string update_version = 5;
string update_release = 6;

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 3, 2019

Contributor

Is there a reason in Automate to have these as separate fields, rather than a single string?

This comment has been minimized.

Copy link
@gpeers

gpeers Jun 3, 2019

Author Contributor

@christophermaier i thought the ergonomics might be easier on the automate side instead of that code having to parse out what it needs for comparisons, etc. lmk if you disagree.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 3, 2019

Contributor

@gpeers maybe we should chat about that usecase. It may be time to create some protobuf types to represent the different types of identifiers that we could have.

This comment has been minimized.

Copy link
@gpeers

gpeers Jun 4, 2019

Author Contributor

@christophermaier i think i'll create a separate ticket and pr for any new types we decide on if that works for you.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 4, 2019

Contributor

@gpeers I guess that depends on when Automate will start consuming this. If they're not going to do anything with this until those new types get added (and if the new types get added as the immediate next PR), then that's probably OK. I'd just hate for them to write code to deal with one form, only to have it changed right out from under them.

Alternatively, if it's not too much work, adding a new type to this PR would also be fine.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

The current and updating-to package must be the same in terms of origin/name, though, right?

This comment has been minimized.

Copy link
@gpeers

gpeers Jun 6, 2019

Author Contributor

@baumanj yes, that's true. at this point in code we also have the version and release of the update package as well. are you ok with sending the update package ident to automate as a single string (origin/name/version/release) in the same way the current package ident is sent over?

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

I'm torn. On one hand, I generally prefer to avoid mandatory duplication (especially in protocols) since it opens the door to inconsistent states. How should consumers behave if the ServiceMetadata says foo/bar/XXX/YYY but the update package says baz/qux/WWW/ZZZ? On the other, we don't have anywhere else where we have a release and version without origin and name, so it's a bit odd. I'd still say my preference would be populating ServiceMetadata with the pre-update FQPI and having just update_version and update_release in this message. If that leads to major awkwardness, I'll defer to your discretion.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 6, 2019

Contributor

Hrmm... it's a good point.

Given that we don't (yet?) have a concept of just "version + release", I'm inclined to keep the code as it is now, sending the fully-qualified identifier of the new version.

As we dig into additional refactorings around our identifier types, though, this distinction could be good to keep in mind.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

We should be careful; changing protocols down the road is much harder than changing code. If we send FQPIs for both now, we're likely locked into that for awhile.

components/sup/protocols/event.proto Outdated Show resolved Hide resolved
components/sup/protocols/event.proto Outdated Show resolved Hide resolved
components/sup/src/event.rs Outdated Show resolved Hide resolved
components/sup/src/event/types.rs Show resolved Hide resolved
components/sup/protocols/event.proto Show resolved Hide resolved
components/sup/Cargo.toml Outdated Show resolved Hide resolved
addresses review comments
Signed-off-by: Gina Peers <gpeers@chef.io>

@gpeers gpeers force-pushed the gcp/service_update_started_event2 branch from 441ec31 to 3a2ccb1 Jun 5, 2019

string update_origin = 3;
string update_name = 4;
string update_version = 5;
string update_release = 6;

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

Aren't these individual field types redundant with package_ident inside ServiceMetadata. I worry about redundancy like this because it can lead to inconsistent states.

components/sup/src/event.rs Outdated Show resolved Hide resolved
components/sup/src/event.rs Outdated Show resolved Hide resolved
refactors update package ident per review comments
Signed-off-by: Gina Peers <gpeers@chef.io>
@christophermaier
Copy link
Contributor

left a comment

A few small tweaks, and then I think we're good to go!

components/sup/src/event.rs Outdated Show resolved Hide resolved
components/sup/src/event/types.rs Outdated Show resolved Hide resolved
refactors event message macro and service update started msg per revi…
…ew comments

Signed-off-by: Gina Peers <gpeers@chef.io>

@gpeers gpeers merged commit 53c37c0 into master Jun 6, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2191 passed (35 minutes, 3 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2420 passed (30 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

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

Update CHANGELOG.md with details from pull request #6611
Obvious fix; these changes are the result of automation not creative thinking.
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.