#1148: Support updates to Instances #1289

staebler · 2017-09-28T18:37:13Z

Closes #1148, #816, #748, #1062, and #1065.

Allow parameters and plan to be changed for a ServiceInstance.
Send an UpdateInstanceRequest to the Broker when the spec changes.
Add UpdateRequests field to ServiceInstance spec that can be bumped to provoke a new UpdateInstanceRequest to the Broker.

MHBauer · 2017-09-29T16:49:13Z

go vet errors need fixing.

pmorie · 2017-10-03T01:49:24Z

@staebler rebase needed

nilebox

See the first comments, will review in more detail later

nilebox · 2017-10-03T13:50:13Z

pkg/apis/servicecatalog/types.go

+	// can be manually incremented by a user to manually trigger an update. This
+	// allows for parameters to be updated with any out-of-band changes that have
+	// been made to the secrets from which the parameters are sourced.
+	UpdateRequests int64


if it's non-negative as the comment says, shall we use the uint64 type?

From https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#primitive-types

Do not use unsigned integers, due to inconsistent support across languages and libraries. Just validate that the integer is non-negative if that's the case.

nilebox · 2017-10-03T13:53:46Z

pkg/apis/servicecatalog/validation/instance.go

 	allErrs = append(allErrs, internalValidateServiceInstanceUpdateAllowed(new, old)...)
 	allErrs = append(allErrs, internalValidateServiceInstance(new, false)...)
+
+	allErrs = append(allErrs, apivalidation.ValidateImmutableField(new.Spec.ExternalServiceClassName, old.Spec.ExternalServiceClassName, field.NewPath("spec").Child("externalServiceClassName"))...)


We also need to check that the plan hasn't changed if plan updates are not supported (ServiceClass.Spec.PlanUpdatable==false).
If it's costly to validate it there, we can check it before sending an update request to the broker.

I am not sure about this, but it is my understanding that the validation should not be looking at other resources. The validation should only consider the resource being validated. There is an admission controller [1] that blocks updates to ExternalServicePlanName when the PlanUpdatable field of the service class is false.

[1] https://github.com/kubernetes-incubator/service-catalog/blob/master/plugin/pkg/admission/serviceplan/changevalidator/admission.go

@staebler didn't know we have an admission controller for that. then there is no need for checking it again, of course.

nilebox · 2017-10-03T13:57:31Z

pkg/controller/controller_instance.go

+		// Only send the plan ID if the plan name has changed from what the Broker has
+		if toUpdate.Status.ExternalProperties == nil ||
+			toUpdate.Status.InProgressProperties.ExternalServicePlanName != toUpdate.Status.ExternalProperties.ExternalServicePlanName {
+			planID := servicePlan.Spec.ExternalID


We need to check the flag plan_updatable in ServiceClass.Spec.PlanUpdatable

I don't think that this is the right place to check this. The change to the plan name has already been accepted by the API Server, given the state of the resources at that time. I don't think it is appropriate to ignore the accepted change to the plan name given the state of the resources at this time. We would need to do more than just not send the plan change to the Broker if that were the case. We would also need to somehow revert the plan change in the Spec of the ServiceInstance.

There's an admission controller that forbids the plan update.

vaikas · 2017-10-04T21:29:50Z

This has nothing to do with your PR, nicely injected in there and from diffs easy to reason about. But, we really have to do some refactoring here, having a method that's several hundred lines intertwined with all kinds of different ifs sprinkled in all over makes it really really hard to digest what's going on.

pmorie · 2017-10-04T21:31:00Z

I am fully supportive of refactoring after all renames and functional features are done :)

…

On Wed, Oct 4, 2017 at 5:29 PM, Ville Aikas ***@***.***> wrote: This has nothing to do with your PR, nicely injected in there and from diffs easy to reason about. But, we really have to do some refactoring here, having a method that's several hundred lines intertwined with all kinds of different ifs sprinkled in all over makes it really really hard to digest what's going on. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1289 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWXmACV3aCrF3TuuxZ0DyYQz2RzRRHLks5so_jPgaJpZM4PntDx> .

vaikas · 2017-10-04T21:40:39Z

yes, hence LGTM

staebler · 2017-10-04T22:33:35Z

I also whole-heartedly support refactoring.

pmorie

Looks pretty good, but there is one fix that is needed re: orphan mitigation.

pmorie · 2017-10-04T22:59:50Z

pkg/controller/controller_instance.go

+		// Only send the plan ID if the plan name has changed from what the Broker has
+		if toUpdate.Status.ExternalProperties == nil ||
+			toUpdate.Status.InProgressProperties.ExternalServicePlanName != toUpdate.Status.ExternalProperties.ExternalServicePlanName {
+			planID := servicePlan.Spec.ExternalID


There's an admission controller that forbids the plan update.

pmorie · 2017-10-04T23:01:33Z

pkg/controller/controller_instance.go

-				errorProvisionCallFailedReason,
-				"ClusterServiceBroker returned a failure for provision call; operation will not be retried: "+s)
+				reason,
+				fmt.Sprintf("ClusterServiceBroker returned a failure for %v call; operation will not be retried: %v", provisionOrUpdateText, s))

 			if shouldStartOrphanMitigation(httpErr.StatusCode) {


You should only do orphan mitigation here if you're provisioning, not if you're updating.

pmorie · 2017-10-05T02:27:54Z

pkg/registry/servicecatalog/instance/strategy.go

 	if !apiequality.Semantic.DeepEqual(oldServiceInstance.Spec, newServiceInstance.Spec) {
 		if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) {
 			setServiceInstanceUserInfo(newServiceInstance, ctx)
 		}
 		newServiceInstance.Generation = oldServiceInstance.Generation + 1
+		setServiceInstanceReadyFalseCondition(newServiceInstance)


In a follow-up, we need to take this out. I'm sorry that I missed it earlier, but update should not affect the status of a resource. It's okay that the ready condition remains set after this - the controller will eventually unset it. Remember, the system is not transactional and only has to eventually converge on the right state.

pmorie · 2017-10-05T02:29:53Z

LGTM but we need to correct the status setting in the API server in a follow-up.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 28, 2017

staebler force-pushed the 1148-instance-update branch from 67b0af6 to 3c50265 Compare September 29, 2017 13:25

staebler changed the title ~~(WIP) #1148: Support updates to Instances~~ #1148: Support updates to Instances Sep 29, 2017

staebler force-pushed the 1148-instance-update branch from 8f0b718 to b927d37 Compare October 2, 2017 13:51

nilebox requested review from nilebox and removed request for nilebox October 3, 2017 13:48

nilebox reviewed Oct 3, 2017

View reviewed changes

staebler force-pushed the 1148-instance-update branch from b927d37 to 2221ffc Compare October 3, 2017 17:57

staebler mentioned this pull request Oct 3, 2017

Correct condition message when async operations fail #1320

Closed

staebler force-pushed the 1148-instance-update branch 3 times, most recently from fb6f623 to e7f8c5d Compare October 4, 2017 17:57

vaikas added the LGTM1 label Oct 4, 2017

pmorie reviewed Oct 4, 2017

View reviewed changes

staebler added 2 commits October 4, 2017 20:36

Allow updates to ServiceInstance.

d155864

Only do orphan mitigation when provisioning.

65ba455

staebler force-pushed the 1148-instance-update branch from e7f8c5d to 65ba455 Compare October 5, 2017 00:38

pmorie reviewed Oct 5, 2017

View reviewed changes

pmorie added the LGTM2 label Oct 5, 2017

pmorie merged commit a6ad96a into kubernetes-retired:master Oct 5, 2017

staebler mentioned this pull request Oct 5, 2017

Do not set Ready/False condition in API Server when updating an instance #1335

Merged

pmorie mentioned this pull request Oct 6, 2017

Support parameter updates for Instances #816

Closed

pmorie mentioned this pull request Oct 6, 2017

Support plan updates for Instances #748

Closed

pmorie added this to the 0.0.24 milestone Oct 10, 2017

This was referenced Jan 10, 2018

Storing/checking the checksum over Secret contents #1062

Closed

Checksums for Instance resources #1065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1148: Support updates to Instances #1289

#1148: Support updates to Instances #1289

staebler commented Sep 28, 2017 •

edited

Loading

MHBauer commented Sep 29, 2017

pmorie commented Oct 3, 2017

nilebox left a comment •

edited

Loading

nilebox Oct 3, 2017

staebler Oct 3, 2017

nilebox Oct 4, 2017

nilebox Oct 3, 2017 •

edited

Loading

staebler Oct 3, 2017

nilebox Oct 4, 2017

nilebox Oct 3, 2017

staebler Oct 3, 2017

pmorie Oct 4, 2017

vaikas commented Oct 4, 2017

pmorie commented Oct 4, 2017 via email

vaikas commented Oct 4, 2017

staebler commented Oct 4, 2017

pmorie left a comment

pmorie Oct 4, 2017

pmorie Oct 4, 2017

pmorie Oct 5, 2017

pmorie commented Oct 5, 2017

#1148: Support updates to Instances #1289

#1148: Support updates to Instances #1289

Conversation

staebler commented Sep 28, 2017 • edited Loading

MHBauer commented Sep 29, 2017

pmorie commented Oct 3, 2017

nilebox left a comment • edited Loading

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

nilebox Oct 3, 2017 • edited Loading

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

vaikas commented Oct 4, 2017

pmorie commented Oct 4, 2017 via email

vaikas commented Oct 4, 2017

staebler commented Oct 4, 2017

pmorie 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

pmorie commented Oct 5, 2017

staebler commented Sep 28, 2017 •

edited

Loading

nilebox left a comment •

edited

Loading

nilebox Oct 3, 2017 •

edited

Loading