Updates first-pass proposal #368

eriknelson · 2017-08-15T14:52:23Z

Think this is a decent starting point, there is a lot to this that I think we'll discover as we move through implementation. I'll try to continue to update this document for anything major. IMO we'll probably be iterating on this a bunch.

eriknelson · 2017-08-15T15:14:07Z

(13) Service Instance Update

jmrodri

two very minor changes. All looks good, I like the updates_to section of the graph. Too bad you couldn't incorporate the arrow (->) in the apb.yml :)

jmrodri · 2017-08-15T16:28:20Z

docs/proposals/updates-first-pass.md

+
+NOTE: The Service Catalog currently does **not** implement update, so triggering
+these from the UI is not possible. Updates should be considered very "alpha".
+To demonstrate , plan is to enhance `asbcli` for update support.


change "plan is to enhance asbcli for update support" to read:

the plan is to enhance asbcli to support update.

Thank you, apparently I can't speak english.

@jmrodri @rthallisey @shawn-hurley RE: asbcli

It's the butt of our jokes because it was a load of crap python I hammered out on a Friday afternoon for a Monday demo expecting it to die on Tuesday, but it's proven to be a very useful tool for broker authors, and I'm finding myself now wanting to implement update support. @rthallisey implemented bind support because he needed it. It's clear ASB authors want this kind of tool, we should work together to define what we're looking for out of it and make that happen. IMO, python remains perfect for this. I'm going to log an issue so we have a distinct place to air desires for what we want this tool to be, and let's support it given the collective interest.

jmrodri · 2017-08-15T16:33:52Z

docs/proposals/updates-first-pass.md

+### Broker
+
+As outlined in the spec, broker must support both async and sync update variants.
+Async is requested via `accepts_incomplete` qparam.


change qparam to query param.

eriknelson · 2017-08-15T18:42:20Z

@jmrodri I was unsure of the field name on APBs to describe the plan graphs. I originally wanted to call it transitions, but I think updates_to is more friendly for APB authors, and I think it's important we're working to make their job as easy as possible. CC @cfchase @dymurray

eriknelson · 2017-08-15T20:53:46Z

@jmrodri Addressed your comments 👍 I'm going to be moving off update to look into some upstream catalog things, so this should get merged with the expectation that an implementation may not happen for a sprint or two. This represents the initial design work and requirements for a first pass.

shawn-hurley

👍 LGTM

dymurray · 2017-08-16T14:48:58Z

docs/proposals/updates-first-pass.md

+    parameters: *_p # param anchor
+    updates_to:
+      - gold
+  - name: silver


Assuming this is supposed to say Gold?

dymurray · 2017-08-16T14:57:40Z

@eriknelson +1 on updates_to:.

This looks good to me. My only question is: Does the APB author need to write two paths for the Gold plan? How does the Gold plan know which plan he is transitioning from? I guess I'm a little uncertain how an APB author would control the logic for upgrading different versions.
e.g.

Dev->Gold goes from version 1.0 to 2.0 where there's a massive schema change / db migration
Silver->Gold has no schema change / db migration

I only bring this up in an instance where knowing the previous plan (state of the app) is necessary for the APB. I only see that information being passed around in the broker to manage state.

Overall this looks great, ACK, just bringing up a concern. Sorry if its been worked through :)

cfchase · 2017-08-16T15:41:56Z

docs/proposals/updates-first-pass.md

+    parameters: *_p # param anchor
+    updates_to:
+      - gold
+  - name: silver


name: gold

cfchase · 2017-08-16T15:52:49Z

docs/proposals/updates-first-pass.md

+plans as the nodes. APBs will need to define this graph in their `apb.yml`
+so the broker is able to validate a requested transition.
+
+Proposal is to add an optional `updates_to` list on `Plan` objects that indicates


What do you think about using updates_from (or updates or can_update) on the destination plan instead of the source plan. It might give some additional flexibility and the logic for updating seems like it would reside in the destination plan. Putting an updates_to on an old version requires that the old version knows all future versions that it would be able to update to. That's workable on a single apb that only upgrades from one plan to another, but in future use cases, that might not be the case. For example, by using updates_from you can use the same syntax to update previous APBs.

# Postgres 9.5 APB name: rhscl-postgresql-9.5-apb # ...snip plans: - name: dev description: A single DB server with no storage free: true metadata: # ...snip parameters: *_p updates_from: - version: 9.4 plan: dev - name: silver description: Silver DB plan with persistence free: false metadata: # ...snip parameters: *_p # param anchor updates_from: - dev # defaults to same version and changes plan - version: 9.4 plan: dev - version: 9.4 plan: silver - name: gold description: A single DB server with persistent storage free: false metadata: # ...snip parameters: *_p # param anchor updates_from: - dev # defaults to same version and changes plan - silver # defaults to same version and changes plan - version: 9.4 plan: dev - version: 9.4 plan: silver - version: 9.4 plan: gold

In this case, the gold plan knows what it can update from and is responsible for the logic for doing so. We may not use this, but it'd be nice to have the option. Also, it feels better putting the option on the plan that will know what to do with the info.

ACK, like the flexibility update_from supports. It feels a little backwards in my mind but your arguments are solid.

WRT versions, I think we need to think through and possibly decouple versions from plans for these reasons. At the very least, that sounds like a problem discussion in-scope for the next pass proposal.

rthallisey · 2017-08-16T15:14:34Z

docs/proposals/updates-first-pass.md

+
+NOTE: The Service Catalog currently does **not** implement update, so triggering
+these from the UI is not possible. Updates should be considered very "alpha".
+To demonstrate , the plan is to enhance `asbcli` to support update.


Can you be more specific about how the service catalog does not implement update?

From my perspective, using the service catalog I can create a resource with plan X and variables Y and Z. If I re-provision the exact same app with the same plan and parameters, I should expect and error because it's already there. If I re-provision with a different parameter it's the same error, correct? Is that what is missing?

How long until the catalog supports update?

Update is a distinct operation/request the platform (service-catalog in this case) is expected to implement. It is not a re-provision. @jmrodri any idea on a timeline? I was not present for the meeting where this came up.

rthallisey · 2017-08-16T15:33:49Z

docs/proposals/updates-first-pass.md

+```
+
+* Q: Is there any value in versioning this object? Ex: `_apb_creds_version` field
+somewhere that begins at 1 and increments each time an update occurs?


If we want to have rollbacks in the future, it would come in handy. But, that's not part of this spec and can be added then.

rthallisey · 2017-08-16T15:43:33Z

docs/proposals/updates-first-pass.md

+    free: true
+    metadata: # ...snip
+    parameters: *_p
+    updates_to:


Looking at this file from a 100ft view, there's a lot of yaml options here. I would be nice if we didn't need updates_to and let the APB writer document that so we don't have to carry as many options.

We need some way for the APBs to express a valid plan transition graph. In the absence of any of these, that implicitly says to me "I don't support plan updates".

rthallisey · 2017-08-16T15:45:49Z

docs/proposals/updates-first-pass.md

+will allow that transition.
+
+## Next steps
+* Updating instances with outstanding bindings, consider both binding consumers


There's also multi app updates, which is a whole can of worms.

rthallisey · 2017-08-16T16:13:16Z

docs/proposals/updates-first-pass.md

+
+#### APB
+
+Overall, the goal with update should be to push the complexity of what an update


Completely agree with this sentence

eriknelson · 2017-08-21T14:23:16Z

@dymurray

How does the Gold plan know which plan he is transitioning from?

The UpdatePayload that gets passed into the APB with a previous_values section that contains the previous plan_id:

{
  "context": {
    "platform": "cloudfoundry",
    "some_field": "some-contextual-data"
  },
  "service_id": "service-guid-here",
  "plan_id": "plan-guid-here",
  "parameters": {
    "parameter1": 1,
    "parameter2": "foo"
  },
  "previous_values": {
    "plan_id": "old-plan-guid-here",
    "service_id": "service-guid-here",
    "organization_id": "org-guid-here",
    "space_id": "space-guid-here"
  }
}

Dev->Gold goes from version 1.0 to 2.0 where there's a massive schema change / db migration
Silver->Gold has no schema change / db migration

This is a much bigger question. The spec reads to me like plans and plan updates were originally designed for tiers of service (multi-tenant DB vs dedicated instance) rather than versions and their updates. I don't know that it makes sense to couple versions to plans, because then you start talking about having a plan for every permutation of <tier>+<version>+...+<N-deployment-variant>, and that starts to get ugly quickly. Version is probably more appropriate passed as a parameter, but that opens up some interesting (and potentially complex) scenarios:

I have an instance of MyFooApp-v1.2.1 that holds an active binding to an instance of PostgreSQL-v9.5. Its database has schema v1 with a bunch of important data in it. What happens when:

I update MyFooApp instance to v2.0.0, which requires a schema+data migration. More importantly, what happens when that fails?
I update PostgreSQL-v9.5to v9.6, does this impact MyFooApp at all? Again, what if it fails?

These are both fair game for the next pass at this that takes active bindings into account, IMO.

eriknelson added the proposal label Aug 15, 2017

Updates first-pass proposal

35f0c54

eriknelson force-pushed the updates-first-pass branch from 73b2ff3 to 35f0c54 Compare August 15, 2017 14:59

jmrodri self-requested a review August 15, 2017 16:22

jmrodri suggested changes Aug 15, 2017

View reviewed changes

Feedback

234c4a2

jmrodri approved these changes Aug 15, 2017

View reviewed changes

jmrodri requested review from cfchase and dymurray August 15, 2017 21:16

eriknelson requested review from rthallisey and shawn-hurley August 16, 2017 01:18

shawn-hurley approved these changes Aug 16, 2017

View reviewed changes

dymurray reviewed Aug 16, 2017

View reviewed changes

cfchase reviewed Aug 16, 2017

View reviewed changes

rthallisey reviewed Aug 17, 2017

View reviewed changes

fix typo

f9b65cf

cfchase approved these changes Aug 22, 2017

View reviewed changes

eriknelson merged commit 2568f4b into openshift:master Aug 24, 2017

eriknelson deleted the updates-first-pass branch August 24, 2017 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates first-pass proposal #368

Updates first-pass proposal #368

eriknelson commented Aug 15, 2017

eriknelson commented Aug 15, 2017

jmrodri left a comment

jmrodri Aug 15, 2017

eriknelson Aug 15, 2017

eriknelson Aug 15, 2017

jmrodri Aug 15, 2017

eriknelson commented Aug 15, 2017 •

edited

eriknelson commented Aug 15, 2017 •

edited

shawn-hurley left a comment

dymurray Aug 16, 2017

dymurray commented Aug 16, 2017

cfchase Aug 16, 2017

cfchase Aug 16, 2017

eriknelson Aug 21, 2017

rthallisey Aug 16, 2017

eriknelson Aug 21, 2017

rthallisey Aug 16, 2017

rthallisey Aug 16, 2017

eriknelson Aug 21, 2017

rthallisey Aug 16, 2017

rthallisey Aug 16, 2017

eriknelson commented Aug 21, 2017


		#### APB

		Overall, the goal with update should be to push the complexity of what an update

Updates first-pass proposal #368

Updates first-pass proposal #368

Conversation

eriknelson commented Aug 15, 2017

eriknelson commented Aug 15, 2017

jmrodri 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

eriknelson commented Aug 15, 2017 • edited

eriknelson commented Aug 15, 2017 • edited

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dymurray commented Aug 16, 2017

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

eriknelson commented Aug 21, 2017

eriknelson commented Aug 15, 2017 •

edited

eriknelson commented Aug 15, 2017 •

edited