APB state support #809

maleck13 · 2018-03-02T15:47:00Z

Proposal outlining a new feature for storing state as an APB developer

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #530

@jmrodri ping initial proposal.

shawn-hurley

I have some minor questions about a slightly different implementation workflow but the core of this proposal is 👍 from me.

shawn-hurley · 2018-03-05T19:40:55Z

docs/proposals/apb-state-support.md

+
+# Proposed Solution
+
+## APB module for handling state


Can we rename bundle contract for handling state? A minor nit right now because we don't have other modules. I just think this will need to be supported more broadly by the bundle/broker contract that @djzager and @mhrivnak have been working on.

It makes sense to have this section detail the ansible module for handling state in the ansible-asb-modules.

I am a bit uncertain here. Should I just replace the heading or all instances of APB?

shawn-hurley · 2018-03-05T19:42:30Z

docs/proposals/apb-state-support.md

+```
+
+Under the hood this APB module would take the key value pair, and store it in a
+ConfigMap (or possibly a secret) labelled ```apb:state```. This ConfigMap would live within the 


I wonder if a name for the config map/secret that conforms to a certain standard might be more appropriate then a label here?

I like the idea of following the pattern we established for bind credentials. Create a configmap with name=POD_NAME in namespace=POD_NAMESPACE

No issue with this approach will update 👍

shawn-hurley · 2018-03-05T19:49:38Z

docs/proposals/apb-state-support.md

+## Update broker to pass through initial state to APB
+
+For every APB action, except provision as there would be no state at this point, if a ConfigMap (ServiceInstanceID-state) is present, 
+in the broker's namespace, its key value pairs will be passed through to the APB prefixed with ```state_<key> = value ```


I wonder if this should actually be that the configmap/secret gets copied into the transient namespace and we add it to the pod. The contract then just has to define where the state gets mounted and it is up to the bundle to use that?

Originally, I thought that this Proposal was suggesting that the APB (or service bundle) has the option to create a configmap that the broker would save for subsequent runs for that service instance. However, if it were possible for the broker to create a configmap on behalf of the APB (or service bundle) mounted at /bundle/state that the bundle could use for saving state, that sounds even better to me (if it is possible).

@maleck13 I'm curious your thoughts on the above. If the broker were to provide Service Bundles a mount point /bundle/state as an example for saving state information, is that keeping with what you had in mind?

I'm actually curious if the broker could create a configmap in the ansible-service-broker namespace and provide that as a volume to an APB pod in the sandbox namespace.

I don't think it is possible to mount a configmap from one namespace into a pod in another namespace .
However it would be possible for the broker to create a configmap in the transient namespace during a provision and for the service bundle to be able to save state to it, then right before clean up the broker would copy this configmap to the broker's namespace. During any follow on action for that serviceinstance, the broker could then create a copy of the configmap in the transient namespace and add a volume to the pod. The service bundle could then add more state if needed and access any previous state from the mount point.

How would you see the service bundle accessing the state? would this be via a apb_get_state <key> or just directly accessing the mount location?

mounted at /bundle/state that the bundle could use for saving state, that sounds even better to me (if it is possible).

Not sure this is possible. I don't think the contents of the configmap would be updated just by saving the state here.

I played around with configmaps in pods a little and see that it is a read-only file system. I should have known better about mapping configmaps across namespaces...I've done worse 😎.

I think we may be back to @shawn-hurley 's original comment. Instead of passing through the key/value pairs could we:

Create the configmap $POD_NAME in $POD_NAMESPACE with the current set of key/value pairs

Mount the configmap, read-only 😎, to a known location on the image for reading those values

shawn-hurley

above comments

djzager · 2018-03-05T20:18:30Z

docs/proposals/apb-state-support.md

+that name was already present, the broker would update and append the values. The broker would also watch 
+for ``Modified`` events on the ConfigMap and ensure to update the corresponding ConfigMap in the broker's namespace. 
+There should only ever be one ConfigMap per ServiceInstance. The ConfigMap would be removed from the broker's namespace 
+once the ServiceInstance was deleted. 


I imagine what you could do, similar to how we handle bind credentials, would be to wait for the APB to be successfully executed (complete with $? == 0) and then move the ConfigMap under the Broker's control.

This sounds reasonable 👍

rthallisey · 2018-03-07T13:49:32Z

docs/proposals/apb-state-support.md

+
+```
+
+Under the hood this APB module would take the key value pair, and store it in a


Can we decide on secret or configmap? I don't think secrets are meant for large amounts of key value pairs, but we probably want all the application data to be hidden. IMO let's go with a secret until we have a need to change.

I'm not certain to what extent we want this data to be hidden. Given that the configmap would exist in the transient namespace and not the target namespace, it sounds sufficiently hidden. With this being configuration data, I vote for it being a configmap.

Yeah my instinct was also a configmap

I think I agree with @rthallisey, why not use secrets? Or maybe we have a store_secret_state and store_state variants?

I like the store_secret_state, this wayPostgreSQL would never want to return w/ provision time credentials because they are admin credentials. They could store them in this state to make the subsequent bind calls functional.

I do think that this creates a larger scope for this PR. If we have two variants of saving state, I could see a first phase and then a second phase IMO.

djzager

I am happy with this proposal. I think it is a great idea to share state. The only concern that I have is our language going forward with respect to Service Bundles (Bundles) and APBs.

I agree that an ansible-asb-module addition, like asb_get_state and asb_set_state, is necessary for this to move forward. My only request is that you also explain t he proposal in the more generic sense as it relates to the "Bundle contract". For example, when saving state we update a configmap|secret in $POD_NAMESPACE with name $POD_NAME and .

djzager · 2018-03-07T15:48:08Z

docs/proposals/apb-state-support.md

@@ -0,0 +1,60 @@
+# Abstract
+
+This proposal will outline a simple and secure method for allowing APB developers to set state during a APB


There is an ongoing discussion about the APB as an instance of a more generic Service Bundle (Bundle). This is the reason for @shawn-hurley earlier comment about "bundle contract".

My gut says that we should say Service Bundle or Bundle when referring to the generic and only use APB when we are specifically talking about Ansible Playbook Bundles.

djzager · 2018-03-07T15:49:22Z

docs/proposals/apb-state-support.md

+
+
+
+# Proposed Solution


This is where I think you should talk about the generic solution for Service Bundles or Bundles and a proposed module addition for use in APBs.

djzager · 2018-03-07T15:50:15Z

docs/proposals/apb-state-support.md

+
+# Proposed Solution
+
+## APB module for handling state


It makes sense to have this section detail the ansible module for handling state in the ansible-asb-modules.

maleck13 · 2018-03-07T16:12:40Z

@djzager is there a link to more details on this contract? I am unaware of it currently

djzager · 2018-03-07T17:13:45Z

@maleck13 https://github.com/openshift/ansible-service-broker/blob/master/docs/service-bundle.md

maleck13 · 2018-03-13T20:06:09Z

@djzager @shawn-hurley @rthallisey Updated to use the term ServiceBundle removed references to secrets as configmaps seem like the correct approach. Updated name the configmap $POD_NAME in $POD_NAMESPACE

Could you guys give it another look over when you get chance. Still a little unsure about the ServiceBudle wording but think I have made the changes suggested now

djzager

Just one question related to handling of "Update broker to pass through initial state to APB." Other than that, this looks great.

djzager · 2018-03-14T13:01:31Z

docs/proposals/apb-state-support.md

+## Update broker to pass through initial state to APB
+
+For every APB action, except provision as there would be no state at this point, if a ConfigMap (ServiceInstanceID-state) is present, 
+in the broker's namespace, its key value pairs will be passed through to the APB prefixed with ```state_<key> = value ```


I think we may be back to @shawn-hurley 's original comment. Instead of passing through the key/value pairs could we:

Create the configmap $POD_NAME in $POD_NAMESPACE with the current set of key/value pairs

Mount the configmap, read-only 😎, to a known location on the image for reading those values

maleck13 · 2018-03-15T09:34:42Z

@djzager ok I will update with an apb_get_state <key> which will read the file in the container

eriknelson

Some questions and clarity I think could be added. Overall though, this will be a nice improvement.

eriknelson · 2018-03-16T14:37:34Z

docs/proposals/apb-state-support.md

+```
+- name: Save some stuff
+  asb_set_state:
+    service_name: "{{ service_name }}"


service_name in this case is the arbitrary state data, correct? Just want to make sure it's not suggested that this is some kind of special key.

just arbitrary

eriknelson · 2018-03-16T14:40:14Z

docs/proposals/apb-state-support.md

+
+```
+
+Under the hood this APB module would take the key value pair, and store it in a


I think I agree with @rthallisey, why not use secrets? Or maybe we have a store_secret_state and store_state variants?

eriknelson · 2018-03-16T14:41:56Z

docs/proposals/apb-state-support.md

+## Update broker to manage Service Bundle created state ConfigMaps
+
+To ensure the state is persisted across Service Bundle actions, the broker will create a ConfigMap within the ```$POD_NAMESPACE``` named ```$POD_NAME```. 
+After an action was successfully completed (ie the Service Bundle exited with a 0 exit code) and before the sandbox namespace was removed, the broker would copy the ConfigMap back to the broker's namespace and name it ```<ServiceInstanceID>-state```. If a ConfigMap with 


If we consider this in a large scale production deployment, are there scaling or quota concerns having potentially 10s of thousands of state maps around?

I initially suggested that when the developer calls set_state it would create a configmap if was not already present. I think it would be better to have lazy creation over eager creation as the requirement for state is not going to be something every apb needs. So happy to go back to that approach. It should significantly reduce the number of objects created although there is still the potential for many objects over a long period of time.
Is this enough to reduce the concern? I do not know enough about what the upper bound would be for configmaps in a namespace, it is also worth noting that we are not iterating through these objects but instead are accessing them by name. I am assuming that in etcd this is a O(1) action in cost rather than O(n)

I think it would be better to have lazy creation over eager creation as the requirement for state is not going to be something every apb needs.

That's a fair point that will probably dramatically reduce the number of configmaps lying around. 👍

eriknelson · 2018-03-16T14:42:47Z

docs/proposals/apb-state-support.md

+After an action was successfully completed (ie the Service Bundle exited with a 0 exit code) and before the sandbox namespace was removed, the broker would copy the ConfigMap back to the broker's namespace and name it ```<ServiceInstanceID>-state```. If a ConfigMap with 
+that name was already present, the broker would update and append the values. 
+There should only ever be one ConfigMap per ServiceInstance. The ConfigMap would be removed from the broker's namespace 
+once the the deprovision action completed successfully. 


Is there any concern that a failed deprovision will leak a state configmap? Does there need to be some kind of garbage collector to remove these? (I'm wondering if the operator pattern is valuable here for managing them).

This is interesting as I know we should clean up all resources created and this would count as one of those. If a deprovision can be retried by the user then we should keep the state as it will be needed again during the next attempt. If it cannot be retried (which I think is the case as you cannot delete the service instance twice) then you are correct and we should always clean up the state configmap no matter what the exit code is. Good spot 👍

eriknelson · 2018-03-16T14:44:09Z

docs/proposals/apb-state-support.md

+## Update broker to pass through initial state to Service Bundle
+
+For every Service Bundle action, except provision as there would be no state at this point, if a ConfigMap (ServiceInstanceID-state) is present, 
+in the broker's namespace, its key value pairs will be passed through to the Service Bundle prefixed with ```state_<key> = value ```


We have precedence for passing _apb prefixed params for system params like this. Would it be acceptable to pass _apb_state=<state_json_blob>?

Based on other comments, we were looking at adding a apb_get_state that would read the state from a mount point in the pod where the configmap would be mounted. WDYT?

I like that a lot because it provides an abstraction layer on top of however it is that we choose to implement it. We'd be able to transparently change how we're actually implementing it without the APBs ever caring. I'm tempted to suggest that's a better pattern than using "_apb" parameters, but there's also the consideration that not all Service Bundles are APBs and have this luxury, and they aren't all going have the modules available. It's a problem we need to solve independently though, since we already have some unfriendly Service Bundle features that need to be made more independent.

eriknelson · 2018-03-16T14:45:27Z

docs/proposals/apb-state-support.md

+Service Bundle's are stateless. All state is managed for them by the broker. Currently the broker passes in parameters specified
+during a request to the catalog. It also passes in some additional parameters such as the namespace etc, additionally 
+we also pass in credentials created during the provision. While this is useful, there is no mechanism for a Service Bundle to store 
+and access data across actions without exposing it to the end user. We want to avoid Service Bundle developers working around this 


We want to avoid Service Bundle developers working around this

Big +1, if there's enough desire to have folks hacking around to make this happen, we need to provide first-class tools to enable them.

eriknelson · 2018-03-16T15:17:22Z

One additional comment, this is going to require apb module development, I would call that out here and maybe submit something to github.com/ansibleplaybookbundle/ansible-playbook-bundle (not sure if that's where our module lives).

shawn-hurley

I think that this is good.

We should determine if we want two different ways of storing state. But I think that could/should be either a follow-on proposal or a second PR after the initial configmap approach IMO.

shawn-hurley · 2018-03-19T13:23:38Z

docs/proposals/apb-state-support.md

+
+```
+
+Under the hood this APB module would take the key value pair, and store it in a


I like the store_secret_state, this wayPostgreSQL would never want to return w/ provision time credentials because they are admin credentials. They could store them in this state to make the subsequent bind calls functional.

I do think that this creates a larger scope for this PR. If we have two variants of saving state, I could see a first phase and then a second phase IMO.

jmrodri

NIT: some very minor grammar changes which we can do later.

jmrodri · 2018-03-19T14:01:31Z

docs/proposals/apb-state-support.md

+
+## Problem Description
+
+Service Bundle's are stateless. All state is managed for them by the broker. Currently the broker passes in parameters specified


typo: Bundle's -> Bundles

add a comma after "Currently"

jmrodri · 2018-03-19T14:02:09Z

docs/proposals/apb-state-support.md

+## Problem Description
+
+Service Bundle's are stateless. All state is managed for them by the broker. Currently the broker passes in parameters specified
+during a request to the catalog. It also passes in some additional parameters such as the namespace etc, additionally 


add comma after "additionally"

jmrodri · 2018-03-19T14:02:50Z

docs/proposals/apb-state-support.md

+
+```
+
+Under the hood this APB module would take the key value pair, and store it in a


add comma after hood

jmrodri · 2018-03-19T14:03:21Z

docs/proposals/apb-state-support.md

+After an action was successfully completed (ie the Service Bundle exited with a 0 exit code) and before the sandbox namespace was removed, the broker would copy the ConfigMap back to the broker's namespace and name it ```<ServiceInstanceID>-state```. If a ConfigMap with 
+that name was already present, the broker would update and append the values. 
+There should only ever be one ConfigMap per ServiceInstance. The ConfigMap would be removed from the broker's namespace 
+once the the deprovision action completed successfully. 


typo: duplicate "the". Remove the second one.

jmrodri · 2018-03-19T14:03:57Z

docs/proposals/apb-state-support.md

+There should only ever be one ConfigMap per ServiceInstance. The ConfigMap would be removed from the broker's namespace 
+once the the deprovision action completed successfully. 
+
+## Update broker to pass through initial state to Service Bundle


add a "the" in front of initial state.

maleck13 · 2018-03-20T09:44:21Z

Going to make a small update to ensure the proposal reflects the decisions.

Add reference to mounting the configmap in the ServiceBundle Pods rather than just passing the params through
Add reference to apb_get_state: key: "somekey"
Remove reference to passing through the state via the extra vars
Call out always removing the configmap on deprovision

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 2, 2018

shawn-hurley added proposal 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 labels Mar 3, 2018

shawn-hurley reviewed Mar 5, 2018

View reviewed changes

shawn-hurley requested changes Mar 5, 2018

View reviewed changes

djzager reviewed Mar 5, 2018

View reviewed changes

maleck13 changed the title ~~first pass at apb state support~~ Apb state support Mar 6, 2018

rthallisey reviewed Mar 7, 2018

View reviewed changes

djzager requested changes Mar 7, 2018

View reviewed changes

first pass at apb state support

9cc76cf

maleck13 force-pushed the issue-530-apb-state-proposal branch from 8cb8747 to 966ef7b Compare March 13, 2018 20:03

maleck13 force-pushed the issue-530-apb-state-proposal branch from 966ef7b to 2da4396 Compare March 13, 2018 20:56

update proposal based on feedback

fa97a3d

maleck13 force-pushed the issue-530-apb-state-proposal branch from 2da4396 to fa97a3d Compare March 13, 2018 20:59

djzager requested changes Mar 14, 2018

View reviewed changes

jmrodri changed the title ~~Apb state support~~ APB state support Mar 15, 2018

eriknelson suggested changes Mar 16, 2018

View reviewed changes

shawn-hurley approved these changes Mar 19, 2018

View reviewed changes

eriknelson approved these changes Mar 19, 2018

View reviewed changes

djzager approved these changes Mar 19, 2018

View reviewed changes

jmrodri approved these changes Mar 19, 2018

View reviewed changes

jmrodri merged commit 2c3a600 into openshift:master Mar 19, 2018

shawn-hurley mentioned this pull request Mar 26, 2018

Randomize object names to avoid clashes #853

Closed


		```

		Under the hood this APB module would take the key value pair, and store it in a

		@@ -0,0 +1,60 @@
		# Abstract

		This proposal will outline a simple and secure method for allowing APB developers to set state during a APB


		## Problem Description

		Service Bundle's are stateless. All state is managed for them by the broker. Currently the broker passes in parameters specified

APB state support #809

APB state support #809

Conversation

maleck13 commented Mar 2, 2018

shawn-hurley 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

maleck13 Mar 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley 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

djzager 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

maleck13 commented Mar 7, 2018

djzager commented Mar 7, 2018

maleck13 commented Mar 13, 2018 • edited

djzager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 commented Mar 15, 2018

eriknelson 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

eriknelson commented Mar 16, 2018

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 commented Mar 20, 2018 • edited

maleck13 Mar 6, 2018 •

edited

maleck13 commented Mar 13, 2018 •

edited

maleck13 commented Mar 20, 2018 •

edited