Update schema for instance-update #444

jmontleon · 2017-09-20T14:54:02Z

No description provided.

jmontleon · 2017-10-03T16:37:28Z

Update requests can be performed with asbcli as well, but for sake of demonstrating bad input"

## rhscl-postgresql-apb - no plan specified, continues with current plan (dev)
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/6119f273-5ef6-4899-89b0-bd7ad5c2e05a -d '{}'
{}

## rhscl-postgresql-apb - prod plan specified with current plan dev
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/6119f273-5ef6-4899-89b0-bd7ad5c2e05a -d '{"plan_id": "prod"}'
{}

## rhscl-postgresql-apb - trying to update to a non existent plan
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/6119f273-5ef6-4899-89b0-bd7ad5c2e05a -d '{"plan_id": "blablabla"}'
{
  "description": "plan not found"
}

## rhscl-postgresql-apb - trying to update from prod to dev (not possible)
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/6119f273-5ef6-4899-89b0-bd7ad5c2e05a -d '{"plan_id": "dev"}'
{
  "description": "plan update not possible"
}

## mediawiki123-apb - new parameters specified:
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/ef0efd9e-6982-44da-a600-1e05e2a1cca1 -d '{"parameters": {"mediawiki_site_name": "wackawacka"}}'
{}

## mediawiki123-apb - new plan_id and parameters specified:
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/ef0efd9e-6982-44da-a600-1e05e2a1cca1 -d '{"plan_id": "default", "parameters": {"mediawiki_site_name": "wackawacka"}}'
{}

## mediawiki123-apb - bad plan specified
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/ef0efd9e-6982-44da-a600-1e05e2a1cca1 -d '{"plan_id": "sdfgsdgfsdfgsdfg", "parameters": {"mediawiki_site_name": "wackawacka"}}'
{
  "description": "plan not found"
}

## mediawiki123-apb - trying to update a non-updatable parameter
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/ef0efd9e-6982-44da-a600-1e05e2a1cca1 -d '{"parameters": {"mediawiki_site_lang": "foo"}}'
{
  "description": "parameter not updatable"
}

## mediawiki123-apb - trying to update a non-existent parameter
$ curl -k -XPATCH -H "Authorization: Bearer $(oc whoami -t)" -H 'Content-Type: application/json'  https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/ansible-service-broker/v2/service_instances/ef0efd9e-6982-44da-a600-1e05e2a1cca1 -d '{"parameters": {"bar": "foo"}}'
{
  "description": "parameter not found"
}

eriknelson

Overall, looks very good! I have a couple things I'd suggest for refactoring:

Update request validations
apb.Provision being used for provision and update

Since I'm requesting the change, I would be happy to submit a patch for both if you think the changes make sense @jmontleon .

eriknelson · 2017-10-16T15:34:29Z

pkg/handler/handler.go

+
+	if err != nil {
+		switch err {
+		case broker.ErrorAlreadyProvisioned:


This isn't a valid error case for Update: https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-3

I think an httpStatusOK (200) should be returned in the case of a successful synchronous update

eriknelson · 2017-10-16T15:36:03Z

pkg/broker/broker.go

+	si, err := a.dao.GetServiceInstance(instanceUUID.String())
+	if err != nil {
+		a.log.Debug("Error retrieving instance")
+		return nil, ErrorNotFound


👍 400 BadRequest results.

eriknelson · 2017-10-16T17:29:37Z

pkg/broker/broker.go

+	   * normal async return UpdateResponse
+
+	   handler returns the following
+	   * synchronous update return 201 created


Sync Updates should return 200 httpOK

eriknelson · 2017-10-16T17:36:56Z

pkg/broker/broker.go

+
+	   handler returns the following
+	   * synchronous update return 201 created
+	   * instance already exists with IDENTICAL parameters to existing instance, 200 OK


The spec is a little ambiguous with this one:

(200) MUST be returned if the request's changes have been applied.

What if no changes are required because the requested update is identical to the current state of the instance? Trying to get clarification with the SIG.

eriknelson · 2017-10-16T17:47:00Z

pkg/broker/broker.go

+
+	// If no plan was specified we'll continue with the current plan
+	if req.PlanID == "" {
+		req.PlanID = (*si.Parameters)["_apb_plan_id"].(string)


It's a good idea to check type assertions for success and deal with surprises explicitly:

var currentPlan string if currentPlan, ok := (*si.Parameters)["_apb_plan_id"].(string); !ok { a.log.Error("No plan_id passed as part of update request, and error occurred while trying to retrieve current plan from service instance") return nil, fmt.Errorf("could not get service instance %s current plan", si.ID) } // Lock down current plan so nothing changes. // Quoting spec: "If this field [plan_id] is not present in the request message, then the service broker // MUST NOT change the plan of the instance as a result of this request. req.PlanID = currentPlan

One other note now that I look at it again: There's a const that should be used instead of the string literal _apb_plan_id for this key: https://github.com/openshift/ansible-service-broker/blob/master/pkg/broker/types.go#L35

So any "_apb_plan_id" should get updated to be planParameterKey instead.

eriknelson · 2017-10-16T18:28:45Z

pkg/broker/broker.go

+		// is set and the job was already started. But I need the token.
+		a.dao.SetState(instanceUUID.String(), apb.JobState{Token: token, State: apb.StateInProgress})
+	} else {
+		// TODO: do we want to do synchronous updating?


We'll likely only see async requests for update from the catalog, but we should probably support sync updates. If not, we should explicitly 422.

eriknelson · 2017-10-16T18:30:56Z

pkg/broker/provision_job.go

 }

 // Run - run the provision job.
 func (p *ProvisionJob) Run(token string, msgBuffer chan<- WorkMsg) {
-	podName, extCreds, err := apb.Provision(p.serviceInstance, p.clusterConfig, p.log)
+	podName, extCreds, err := apb.Provision(p.task, p.serviceInstance, p.clusterConfig, p.log)


I'm not a huge fan of parameterizing Provision with a couple variants of behavior; it breaks the semantics of the function from a caller's perspective. One thought would be to have two distinct Provision and Update available on the public interface, and behind the scenes they actually do some specific behavior for their case, and call into some shared function.

eriknelson · 2017-10-16T18:32:42Z

pkg/broker/util.go

+					SchemaRef:  schema.SchemaURL,
+					Type:       []schema.PrimitiveType{schema.ObjectType},
+					Properties: updatableProperties,
+					Required:   updatableRequired,


Will this always be the set of required parameters that also are update-able?

eriknelson · 2017-10-16T18:33:40Z

pkg/broker/util.go

@@ -291,6 +302,33 @@ func extractRequired(params []apb.ParameterDescriptor) []string {
 	return req
 }

+func extractUpdatable(params []apb.ParameterDescriptor) map[string]*schema.Schema {


👍 spec transforms look good.

eriknelson · 2017-10-16T18:35:03Z

pkg/handler/handler.go

+	} else if async {
+		writeDefaultResponse(w, http.StatusAccepted, resp, err)
+	} else {
+		writeDefaultResponse(w, http.StatusCreated, resp, err)


This should be an http.StatusOK

* Abstract out validations * Separate shared provision/update logic and wrap with public funcs

Instance update refactoring

eriknelson · 2017-10-17T20:34:04Z

Confirmed I'm able to transition from "dev" to "prod" plans as well as modify postgresql_version from 9.4 -> 9.5. 🎉

shawn-hurley · 2017-10-18T13:23:03Z

pkg/apb/provision_or_update.go

+	ns := instance.Context.Namespace
+	log.Info("Checking if project %s exists...", ns)
+	if !projectExists(ns) {
+		log.Error("Project %s does NOT exist! Cannot provision requested %s", ns, instance.Spec.FQName)


I think this should be log.Errorf

shawn-hurley · 2017-10-18T13:27:40Z

pkg/apb/provision_or_update.go

+}
+
+func projectExists(project string) bool {
+	_, _, code := runtime.RunCommandWithExitCode("kubectl", "get", "project", project)


I would really prefer to not rely on kubectl and use the namespace client-go interface. This is going to help us remove places that we are running commands rather than using the client.

If you feel that we should let this pass through and deal with it during the other refactors I understand that just want to call it out here.

I agree, this code has been here for some time however and simply got moved over from Provision, so I'd rather address it in another PR.

Existing code fix in another PR. 👍

shawn-hurley · 2017-10-18T13:39:58Z

pkg/handler/handler.go


-	writeDefaultResponse(w, http.StatusOK, resp, err)
+	// ignore the error, if async can't be parsed it will be false
+	async, _ = strconv.ParseBool(r.FormValue("accepts_incomplete"))


We must be checking for auto escalate and checking for users access to the namespace. see: https://github.com/openshift/ansible-service-broker/blob/master/pkg/handler/handler.go#L246

I'll take a look at this, thanks for pointing it out @shawn-hurley

* Also added Context for update requests

Check autoescalate and user privileges for update

* Move to proper Update job/subscribers instead of trying to use Provision jobs

Full async update support and burst request guard

jmrodri

The OSB spec states:

To enable support for the update of the plan, a service broker MUST declare support per service by including plan_updateable: true in its catalog endpoint.

I think the apb needs to have this field set on its metadata. I expect the catalog to look at that before calling update.

We have PlanUpdatable on the struct, but we're not looking at it. I also think the broker should look at it before performing an update.

https://github.com/openshift/ansible-service-broker/blob/master/pkg/broker/types.go#L69

jmrodri · 2017-10-19T20:40:51Z

pkg/apb/provision.go

 func Provision(
 	instance *ServiceInstance,
-	clusterConfig ClusterConfig, log *logging.Logger,
+	clusterConfig ClusterConfig,
+	log *logging.Logger,


jmrodri · 2017-10-19T20:50:53Z

pkg/apb/provision_or_update.go

+}
+
+func projectExists(project string) bool {
+	_, _, code := runtime.RunCommandWithExitCode("kubectl", "get", "project", project)


Existing code fix in another PR. 👍

jmrodri · 2017-10-19T20:53:53Z

pkg/apb/provision_or_update.go

+	executionMethodUpdate    executionMethod = "update"
+)
+
+func provisionOrUpdate(


Good refactoring. I also like the comment in provision and update which is helpful.

jmrodri · 2017-10-19T20:56:24Z

pkg/app/app.go

+	if err != nil {
+		app.log.Errorf("Failed to attach subscriber to WorkEngine: %s", err.Error())
+		os.Exit(1)
+	}


+1 to the new subscriber

jmrodri · 2017-10-19T21:05:35Z

pkg/broker/broker.go

@@ -71,7 +81,7 @@ type Broker interface {
 	Bootstrap() (*BootstrapResponse, error)
 	Catalog() (*CatalogResponse, error)
 	Provision(uuid.UUID, *ProvisionRequest, bool) (*ProvisionResponse, error)
-	Update(uuid.UUID, *UpdateRequest) (*UpdateResponse, error)
+	Update(uuid.UUID, *UpdateRequest, bool) (*UpdateResponse, error)


good catch for async

jmrodri · 2017-10-19T21:09:07Z

pkg/broker/broker.go

+			} else if rs.State.Method == apb.JobMethodUpdate {
+				job = NewUpdateJob(instance, a.clusterConfig, a.log)
+				topic = UpdateTopic
+			} else {


Logic seems correct.

jmrodri · 2017-10-19T21:09:29Z

pkg/broker/broker.go

+					"Attempted to recover job %s, but found an unrecognized "+
+						"MethodType: %s, skipping...",
+					rs.State.Token, rs.State.Method,
+				)


You really want me to make the line length 80 characters :)

jmrodri · 2017-10-19T21:14:50Z

pkg/broker/broker.go

+	}
+
+	// Retrieve from/to plans by name, else respond with appropriate error
+	if fromPlan = spec.GetPlan(fromPlanName); fromPlan == nil {


I wasn't aware this syntax was legal :)

spadgett · 2017-10-19T21:58:32Z

I think the apb needs to have this field set on its metadata. I expect the catalog to look at that before calling update.

The web console will only allow you to change plans if planUpdatable is true.

jmrodri · 2017-10-19T22:37:34Z

@spadgett thanks for the confirmation that's what I suspected based on the spec. So we'll fix it.

* asbcli update to support PATCH request * Add rudimentary plan update support, catch asbcli exception * merge provision and update code * add comments, clean up logic, add return errors * Fix schema test * fix style errors * customize yaml serializer for style fix * Fix test * Instance update refactoring * Abstract out validations * Separate shared provision/update logic and wrap with public funcs * Lookup plan names by ID for update * asbcli update to support PATCH request * Add rudimentary plan update support, catch asbcli exception * merge provision and update code * add comments, clean up logic, add return errors * Fix schema test * fix style errors * customize yaml serializer for style fix * Fix test * Instance update refactoring * Abstract out validations * Separate shared provision/update logic and wrap with public funcs * fixes * more fixes * Use Errorf * Perform auto escalation check for update * Also added Context for update requests * Trigger metrics for both provision and update * Add metrics import * Return correct status code for sync update * Add some logging * Implement distinct async Update support * Move to proper Update job/subscribers instead of trying to use Provision jobs * Prevent multiple update pods from spawning * set plan_updateable, fix Lint errors * style/syntax fixes

jmontleon changed the base branch from instance-update to master September 26, 2017 18:39

jmontleon changed the title ~~Update schema for instance-update~~ [WIP][DO NOT MERGE]Update schema for instance-update Sep 26, 2017

shawn-hurley added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. feature labels Sep 29, 2017

jmrodri added needs-bugzilla and removed needs-bugzilla labels Oct 2, 2017

jmontleon changed the title ~~[WIP][DO NOT MERGE]Update schema for instance-update~~ Update schema for instance-update Oct 3, 2017

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 5, 2017

eriknelson added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-review labels Oct 16, 2017

eriknelson suggested changes Oct 16, 2017

View reviewed changes

jmontleon and others added 9 commits October 17, 2017 10:24

asbcli update to support PATCH request

e29cfdb

Add rudimentary plan update support, catch asbcli exception

900e3f9

merge provision and update code

f3c4f58

add comments, clean up logic, add return errors

398bc1f

Fix schema test

4f54ae1

fix style errors

55861d7

customize yaml serializer for style fix

e503362

Fix test

e78e0cb

Instance update refactoring

a507619

* Abstract out validations * Separate shared provision/update logic and wrap with public funcs

eriknelson removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2017

jmontleon and others added 8 commits October 17, 2017 11:02

Merge pull request #2 from eriknelson/instance-update

2c67041

Instance update refactoring

Lookup plan names by ID for update

4a22866

asbcli update to support PATCH request

e53fb32

Add rudimentary plan update support, catch asbcli exception

e022fcf

merge provision and update code

a00c76f

add comments, clean up logic, add return errors

838a6ad

Fix schema test

b7535a1

fix style errors

de7fd83

jmontleon added 2 commits October 17, 2017 14:16

fixes

ff52680

more fixes

87791c2

eriknelson requested review from jmrodri and rthallisey October 17, 2017 18:52

eriknelson removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 17, 2017

shawn-hurley requested changes Oct 18, 2017

View reviewed changes

eriknelson and others added 7 commits October 18, 2017 10:31

Use Errorf

68c8a5e

Perform auto escalation check for update

c500c66

* Also added Context for update requests

Merge pull request #5 from eriknelson/instance-update

4a37e9f

Check autoescalate and user privileges for update

Merge branch 'master' into instance-update

2fa0ff2

Trigger metrics for both provision and update

89dc889

Add metrics import

433c383

Return correct status code for sync update

88b85dd

shawn-hurley approved these changes Oct 18, 2017

View reviewed changes

eriknelson and others added 4 commits October 18, 2017 20:12

Add some logging

d08467e

Implement distinct async Update support

7237665

* Move to proper Update job/subscribers instead of trying to use Provision jobs

Prevent multiple update pods from spawning

6675fd1

Merge pull request #6 from eriknelson/instance-update

d28b45b

Full async update support and burst request guard

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 19, 2017

eriknelson approved these changes Oct 19, 2017

View reviewed changes

jmrodri suggested changes Oct 19, 2017

View reviewed changes

jmontleon added 2 commits October 20, 2017 09:45

set plan_updateable, fix Lint errors

203a428

style/syntax fixes

be32615

jmrodri approved these changes Oct 20, 2017

View reviewed changes

jmontleon merged commit 439ce3e into openshift:master Oct 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update schema for instance-update #444

Update schema for instance-update #444

jmontleon commented Sep 20, 2017

jmontleon commented Oct 3, 2017

eriknelson left a comment •

edited

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017 •

edited

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson Oct 16, 2017

eriknelson commented Oct 17, 2017 •

edited

shawn-hurley Oct 18, 2017

shawn-hurley Oct 18, 2017

eriknelson Oct 18, 2017

jmrodri Oct 19, 2017

shawn-hurley Oct 18, 2017

eriknelson Oct 18, 2017

jmrodri left a comment

jmrodri Oct 19, 2017

jmrodri Oct 19, 2017

jmrodri Oct 19, 2017

jmrodri Oct 19, 2017

jmrodri Oct 19, 2017

jmrodri Oct 19, 2017

jmrodri Oct 19, 2017

jmrodri Oct 19, 2017

spadgett commented Oct 19, 2017

jmrodri commented Oct 19, 2017

Update schema for instance-update #444

Update schema for instance-update #444

Conversation

jmontleon commented Sep 20, 2017

jmontleon commented Oct 3, 2017

eriknelson left a comment • edited

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 Oct 16, 2017 • edited

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 Oct 17, 2017 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Oct 19, 2017

jmrodri commented Oct 19, 2017

eriknelson left a comment •

edited

eriknelson Oct 16, 2017 •

edited

eriknelson commented Oct 17, 2017 •

edited