Add ObservedGeneration and Provisioned into ServiceInstanceStatus #1748

nilebox · 2018-02-20T10:01:30Z

Add ObservedGeneration and Provisioned fields to ServiceInstanceStatus
Switch to using ObservedGeneration and Provisioned on the read path (while keeping write paths for both ReconciledGeneration and ObservedGeneration for backward compatibility)
Initialize ObservedGeneration and Provisioned fields if they are not set while ReconciledGeneration is set (for API migration between Service Catalog versions)

…/Failed conditions

kibbles-n-bytes · 2018-02-22T00:35:17Z

pkg/apis/servicecatalog/types.go

+	// successfully provisioned (i.e. it exists on the broker's side and can be
+	// updated or deprovisioned). The flag should be reset after successful
+	// deprovisioning.
+	Provisioned bool


Provisioned seems like something appropriate for a condition as opposed to bool. Thoughts?

@kibbles-n-bytes Yeah I agree, that might be a good idea.
Not sure if we'll need to distinguish the initial "unprovisioned" state from the "deprovisioned" (after deletion or orphan mitigation) there.

@nilebox @kibbles-n-bytes I disagree that provisioned should be a condition as opposed to a bool. The move in kube is to use simple fields instead of conditions. See kubernetes/kubernetes#7856 (comment).

Yes, we should avoid using bool fields. How about ProvisionedStatus for the field and an enum of {NotProvisioned,Provisioned} for the values it can take on, similar to the Deprovision status field?

@staebler I don't really have a preference, but the quorum seems to advocate for ProvisionedStatus...

@nilebox That's fine. My issue was using a condition (in the traditional kube sense) as opposed to a field, where kube is moving towards preferring a field. I have no reservation with using ProvisionedStatus instead of a bool.

nilebox · 2018-02-22T05:34:02Z

pkg/controller/controller_instance.go

 	c.recorder.Event(instance, corev1.EventTypeWarning, readyCond.Reason, readyCond.Message)
 	currentReconciledGeneration := instance.Status.ReconciledGeneration

 	setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionReady, readyCond.Status, readyCond.Reason, readyCond.Message)
-	clearServiceInstanceCurrentOperation(instance)
+	setServiceInstanceCondition(instance, v1beta1.ServiceInstanceConditionFailed, failedCond.Status, failedCond.Reason, failedCond.Message)


This is a bugfix: once I switched from ReconciledGeneration to ObservedGeneration + Failed:True, I found that some integration tests were failing on waiting for this condition.
For provisioning we always set the Failed:True condition for terminal errors, and for updates we never were.
I think this is a bug, and fixed it for consistency.

I'm definitely in favor of this, though we should make sure this keeps the current behavior of spec changes to a failed update triggering a new update.

From a cursory glance, it seems like that'd require 1) removing the L383 block on the Failed condition, and 2) removing the Failed condition when recording the operation start at L422.

I don't think that it is a bug. The semantics of Failed:True are that the instance is not usable. If an update fails, the instance is still usable, in its previous state. With that being said, I am on board with changing the semantics of Failed:True to mean that the previous operation failed.

That's true. It's not really a bug, more just an unfortunate design choice that can be easily rectified.

@staebler @kibbles-n-bytes by "bug" I mean that this method is not doing what the comment says:

// processUpdateServiceInstanceFailure handles the logging and updating of a // ServiceInstance that hit a terminal failure during update reconciliation.

The "terminal failure" here means that Instance should be marked with Failed:True, but it is not.
So I wanted to temporarily "fix" this to make it work as it was initially (even though poorly) designed, and then separately fix it according to the new design.

removing the L383 block on the Failed condition, and 2) removing the Failed condition when recording the operation start at L422.

@kibbles-n-bytes I believe the other PRs are already addressing this: #1751 and #1765

nilebox · 2018-02-22T05:56:28Z

Fixed all tests, please review.

MHBauer · 2018-02-24T01:14:55Z

as far as beta api changnes go, do we have to be conncerned with changes in nstatus?

nilebox · 2018-02-24T01:25:29Z

@MHBauer FWIW, it's not just about breaking API changes, but also about being able to pick up the existing ServiceInstance resources which have ReconciledGeneration set and don't have ObservedGeneration, i.e. data migration.

kibbles-n-bytes · 2018-02-26T18:48:11Z

pkg/controller/controller_instance.go

@@ -1441,12 +1480,13 @@ func (c *controller) processServiceInstanceOperationError(instance *v1beta1.Serv

 // processProvisionSuccess handles the logging and updating of a
 // ServiceInstance that has successfully been provisioned at the broker.
-func (c *controller) processProvisionSuccess(instance *v1beta1.ServiceInstance, dashboardURL *string) error {


Why ever update the ObservedGeneration in these processXY functions? The way I understood it, ObservedGeneration should be set when the controller starts processing a resource at a specific generation. Thus it would have been set correctly earlier on.

That was my understanding as well. I am not keen on us relying on a side effect of clearing the current operation as the mechanism by which we are setting the ObservedGeneration. I would like the ObservedGeneration to be set explicitly in the reconcileXXXAdd and reconcileXXXUpdate functions.

One thing that is missing with the current approach is setting the ObservedGeneration when there is a change to the spec while an operation is in progress. I understand that is not allowed currently by the API validation, but I think we can accommodate that now to cover us in case we do allow for changes in the future.

I am not keen on us relying on a side effect of clearing the current operation as the mechanism by which we are setting the ObservedGeneration

I agree. IMO clearServiceInstanceCurrentOperation should no longer do any generation updating. We started to hack around it once we introduced aborting orphan mitigation in favor of deprovisioning, and we only made it worse with the ReconciledGeneration hack when deleting polling instances. (I can fix the RG problems in a separate PR, though).

@kibbles-n-bytes currently the ObservedGeneration is updated when the synchronous provisioning has succeeded, but you're right that recordStartOfServiceInstanceOperation is probably a better place to do this.

One thing that is missing with the current approach is setting the ObservedGeneration when there is a change to the spec while an operation is in progress.

@staebler do you have suggestions how to address this? the problem is that with async operation in progress ObservedGeneration + Ready:True is a sign that the operation has succeeded. So if you update the ObservedGeneration while the previous generation is still being processed by the broker, once it succeeds the status will be incorrect (i.e. pointing that a newer generation has been successfully reconciled).
Which makes me think that we might still need ReconciledGeneration to cover this... But then the ReconciledGeneration should be updated in both success and failure scenarios, I think (i.e. mean that "the processing of the spec has finished" but not necessarily succeeded)

@kibbles-n-bytes WDYT?

One thing that is missing with the current approach is setting the ObservedGeneration when there is a change to the spec while an operation is in progress.

@nilebox I was not as clear as I could have been in describing what I meant by "while an operation is in progress". I meant that there were pending changes that still needed to be reconciled with the broker, not that there was an async operation in progress.

For example, let's say that the user updates the instance to change the parameters, changing the generation of the instance to 2. The controller observes the new generation and sets the observed generation to 2. The controller sets CurrentOperation to Update. There is some retryable error that prevents the instance from being updated successfully on the broker. Then, the user makes another update to the instance, changing the generation of the instance to 3. Subsequent requests sent to the broker will use the spec from generation 3. However, the controller will not change the observed generation to 2 because CurrentOperation was and remains Update. Later, a request to the broker is finally successful. If the request was sync, the controller sets the observed generation to 3. If the request was async, the controller does not set the observed generation: it stays 2.

Yeah that makes sense. The comment above was written before we discussed the meaning of ObservedGeneration vs LastAttemptedGeneration in Slack.

staebler · 2018-02-26T19:38:31Z

pkg/controller/controller_test.go

@@ -2039,6 +2072,8 @@ func assertServiceInstanceErrorBeforeRequest(t *testing.T, obj runtime.Object, r
 	assertServiceInstanceCurrentOperationClear(t, obj)
 	assertServiceInstanceOperationStartTimeSet(t, obj, false)
 	assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration)
+	assertServiceInstanceObservedGeneration(t, obj, originalInstance.Status.ObservedGeneration)


I would expect that the controller would set ObservedGeneration even if there was an error prior to sending the request to the broker. For example, if there were an error getting the service class, the controller sets the condition for the instance to Ready:False. I would expect that the controller would also set the ObservedGeneration to the current generation.

Yeah good point, but as I mentioned above it breaks the condition to check whether the instance has been reconciled or not. Probably keeping both ObservedGeneration and ReconciledGeneration will help there.

@staebler I checked the controller_instance.go - currently we invoke recordStartOfServiceInstanceOperation() method only after we have checked the service class/plan and prepared an OSB request.
Do you suggest that we need to move this invocation higher (right after checking if we need to process at all)?
@kibbles-n-bytes do you agree?

The problem here is that recordStartOfServiceInstanceOperation() method sets the instance.Status.InProgressProperties field that requires service class/plan.
Thus, we should either split recordStartOfServiceInstanceOperation() into two methods (the first updating ObservedGeneration and the second populating the operation details), or leave it as it is.

If we wait until after the class/plan are checked, then the ObservedGeneration is inconsistent in the presence of an errors with the check. Let's say that there was an error getting the service class. The controller will change the instance to Ready:False. If the controller does not also set the ObservedGeneration, then it will appear as though there was an error with an earlier generation as opposed to there being an error reconciling the current generation.

Yeah good point, but as I mentioned above it breaks the condition to check whether the instance has been reconciled or not.

@nilebox I don't understand this. Why does it break the condition? I would argue that it breaks the condition if we do not set the observed generation.

staebler · 2018-02-26T19:58:10Z

pkg/controller/controller_instance.go

+// needed for the instance based on ObservedGeneration
+func isServiceInstanceProcessedAlready(instance *v1beta1.ServiceInstance) bool {
+	return instance.Status.ObservedGeneration >= instance.Generation &&
+		(isServiceInstanceReady(instance) || isServiceInstanceFailed(instance))


How would you feel about also adding a check for !instance.Status.OrphanMitigationInProgress? It doesn't change the behavior of the controller either way, because isServiceInstanceProcessedAlready is never called with orphan mitigation is in progress. However, I think it covers more completely the criteria that determine when a generation has been completely processed.

nilebox · 2018-02-27T13:15:57Z

pkg/controller/controller_instance.go

 	)
 	resetServiceInstanceCondition(
 		toUpdate,
 		v1beta1.ServiceInstanceConditionFailed,
 	)
+	return c.updateServiceInstanceStatus(toUpdate)


Initially I didn't want to sent an update request there (and instead send it with the first status update), but:

The updateServiceInstanceReferences() updates only references but re-reads the entire instance from the server, overwriting the ObservedGeneration.
Doesn't it lead to re-reading the spec too, thus potentially reading the newer spec than the recorded ObservedGeneration? @kibbles-n-bytes @staebler see controller_instance.go#L1030
If it does, then we can't set ObservedGeneration separately from storing InProgressProperties, i.e. ObservedGeneration should be only updated in recordStartOfServiceInstanceOperation() and not earlier...

if the status update will fail later, we won't update ObservedGeneration until the next iteration, thus the status will be obsolete for a while

@kibbles-n-bytes Ok, I think I will just need to extract InProgressProperties from the prepareProvisionRequest() method (which also requires class/plan that might be invalid), then I will be able to just move the recordStartOfServiceInstanceOperation() higher to the top of processing (before retrieving class and plan) instead of splitting it into two methods.

Actually still not possible, because:

rh.inProgressProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: servicePlan.Spec.ExternalName, ClusterServicePlanExternalID: servicePlan.Spec.ExternalID, ...

so annoying...

@nilebox

re-reads the entire instance from the server, overwriting the ObservedGeneration

What do you mean be overwriting the ObservedGeneration? If that field has been changed in the instance held by the controller, then it will be written to the server. My reading of what you wrote is that you think that the change to the ObservedGeneration made by the controller will be overwritten by whatever is currently held in storage prior to the update. So, for example, if the ObservedGeneration in storage is 2, the controller set it to 3, then updateServiceInstanceReferences will end up setting it back to 2. My understanding is different. I would expect updateServiceInstanceReferences to cause the ObservedGeneration in storage to change to 3. The ObservedGeneration of the instance held by the controller will remain 3.

Doesn't it lead to re-reading the spec too, thus potentially reading the newer spec than the recorded ObservedGeneration?

@nilebox It will not lead to reading in a newer spec. If the spec is newer in storage, then the update will fail with a conflict. The reconciliation will end there with no changes affected. The instance will be re-queued, at which time the controller will attempt to reconcile the newer spec.

@staebler thanks, I completely forgot that update will fail with conflict, so there is no possibility of re-reading a newer spec.

I would expect updateServiceInstanceReferences to cause the ObservedGeneration in storage to change to 3.

No, this method is a subresource IIRC, and it will ignore the status (won't update ObservedGeneration) and will overwrite any changes to the status when instance is re-read. That's why we need to send a status update with updated ObservedGeneration separately before updating references. Please correct me if I'm missing something again.

Alternatively, we can just remember the local changes to status and overwrite it again on the controller side. Then we don't have to make a separate call for updating ObservedGeneration - it will be updated with the first status update (i.e. in case of any error or when we record the operation)

nilebox · 2018-02-27T23:16:34Z

pkg/controller/controller_instance.go

 	updatedInstance, err := c.serviceCatalogClient.ServiceInstances(toUpdate.Namespace).UpdateReferences(toUpdate)
 	if err != nil {
 		glog.Errorf(pcb.Messagef("Failed to update references: %v", err))
 	}
+	// The UpdateReferences method ignores status changes.
+	// Restore status that might have changed locally to be able to update it later.
+	updatedInstance.Status = status


Instead of moving the ObservedGeneration change until after updateServiceInstanceReferences() method is invoked, I just "fixed" the updateServiceInstanceReferences method so that ordering doesn't matter anymore.
I think it's better than having to remember that the ordering matters.

Yup, seems reasonable, and with the no-multi-update changes coming it probably won't end up mattering soon anyway. 😃

staebler · 2018-02-28T02:36:09Z

pkg/controller/controller_instance.go

@@ -468,6 +514,10 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns
 	glog.V(4).Info(pcb.Message("Processing deleting event"))

 	instance = instance.DeepCopy()
+	// Any status updates from this point should have an updated observed generation
+	if instance.Status.ObservedGeneration != instance.Generation {
+		c.prepareObservedGeneration(instance)


We don't want to set ObservedGeneration when doing orphan mitigation. That could cause the latest generation to be skipped. There's an argument to be made that we don't need to bother setting ObservedGeneration for a pure delete either, given that there will never be another provision or update for the object.

Here is an example of skipping a generation.

User creates the instance.

Controller sends a failing provision request to the broker.

Prior to the controller starting the orphan mitigation process, the user changes the spec to generation 2.

Controller does an orphan-mitigation reconcile with generation 2 of the instance. Controller sets ObservedGeneration to 2.

Deprovision request completes successfully. Controller sets OrphanMitigationInProgress to false.

Controller does an instance-add reconcile with generation 2 of the instance. ObservedGeneration is 2. Controller stops reconcile early because isServiceInstanceProcessedAlready returns true.

Yeah, I agree that we might just stop updating ObservedGeneration for both orphan mitigation and actual deletion. The latter is the one that is triggered by the user though, so it might be still better to report that we have started processing the deletion of instance.
Will add a condition on orphan mitigation there (ugly but should work).

staebler · 2018-02-28T02:57:42Z

pkg/controller/controller_instance.go

+// to "false" with empty reason and message
+func resetServiceInstanceCondition(toUpdate *v1beta1.ServiceInstance,
+	conditionType v1beta1.ServiceInstanceConditionType) {
+	setServiceInstanceConditionInternal(toUpdate, conditionType, v1beta1.ConditionFalse, "", "", metav1.Now())


Is there precedence for having blank reason and message in conditions? My inclination is to use a reason like "ObservedNewGeneration" and a message like "Observed a new generation of the ServiceInstance". I am also inclined to call setServiceInstanceCondition directly in prepareObservedGeneration rather than having the resetServiceInstanceCondition function.

Ok, will add some meaningful reason and message there.

staebler · 2018-02-28T02:58:19Z

pkg/controller/controller_instance.go

@@ -950,6 +1009,13 @@ func newServiceInstanceFailedCondition(status v1beta1.ConditionStatus, reason, m
 	}
 }

+// resetServiceInstanceCondition sets a single condition on Instnace's status


s/Instnace/Instance

staebler · 2018-02-28T03:02:44Z

pkg/controller/controller_test.go

@@ -2148,7 +2193,7 @@ func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object,
 	case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationDeprovision:
 		assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason)
 	case v1beta1.ServiceInstanceOperationUpdate:
-		assertServiceInstanceConditionsCount(t, obj, 1)
+		assertServiceInstanceCondition(t, obj, v1beta1.ServiceInstanceConditionFailed, v1beta1.ConditionTrue, failureReason)


The switch is not needed at all any more. The assert is the same for all operations.

nice catch!

staebler · 2018-02-28T03:11:55Z

test/util/util.go

@@ -216,7 +216,8 @@ func WaitForInstanceReconciledGeneration(client v1beta1servicecatalog.Servicecat
 				return false, fmt.Errorf("error getting Instance %v/%v: %v", namespace, name, err)
 			}

-			if instance.Status.ReconciledGeneration == reconciledGeneration {
+			if instance.Status.ObservedGeneration >= reconciledGeneration &&


Should change the name of the WaitForInstanceReconciledGeneration function and its reconciledGeneration parameter since it is now waiting for the generation to be processed rather than waiting for a particular reconciled generation.

staebler · 2018-02-28T03:15:33Z

pkg/openapi/openapi_generated.go

@@ -1518,6 +1525,13 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA
 								Ref:         ref("github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.ServiceInstancePropertiesState"),
 							},
 						},
+						"provisioned": {


This has not been updated with the json name change.

I wonder if this is supposed to be caught by the portion of the CI that verifies that the generated files are up-to-date.

staebler · 2018-02-28T03:23:59Z

test/util/util.go

+// status true.
+func isServiceInstanceFailed(instance *v1beta1.ServiceInstance) bool {
+	for _, condition := range instance.Status.Conditions {
+		if condition.Type == v1beta1.ServiceInstanceConditionFailed && condition.Status == v1beta1.ConditionTrue {


I understand that isServiceInstanceReady and isServiceInstanceFailed are functions copied from elsewhere. But it makes me sad to see them so close together, doing the same thing, and yet differing slightly. Can we have each one call a common isServiceInstanceConditionTrue(*v1beta1.ServiceInstance, v1beta1.ServiceInstanceCondition) function? Barring that, can we be consistent about whether we short-circuit when the condition is found or whether we only short-circuit when the condition is found with a True status?

nilebox · 2018-02-28T11:46:17Z

@staebler I believe all your recent comments were addressed, PTAL.

staebler

Nice work!

pmorie · 2018-03-01T22:43:58Z

pkg/apis/servicecatalog/types.go

@@ -537,8 +537,14 @@ type ServiceInstanceStatus struct {
 	// ReconciledGeneration is the 'Generation' of the serviceInstanceSpec that
 	// was last processed by the controller. The reconciled generation is updated
 	// even if the controller failed to process the spec.
+	// Deprecated: use ObservedGeneration instead


I think it would be helpful to specify what 'use' pertains to here (totally fine to do this in a follow-up)

pmorie · 2018-03-01T22:46:08Z

pkg/apis/servicecatalog/v1beta1/types.go

 	ReconciledGeneration int64 `json:"reconciledGeneration"`

+	// ObservedGeneration is the 'Generation' of the serviceInstanceSpec that
+	// was last processed by the controller. The observed generation is updated
+	// whenever the status is updated regardless of operation result


Nit, this should end with a period.

pmorie · 2018-03-01T23:02:30Z

pkg/controller/controller_instance.go

+	if err != nil {
+		return err
+	}
+	if handled {


Thanks for returning early here 👍

pmorie · 2018-03-01T23:04:51Z

pkg/controller/controller_instance.go

+// isServiceInstanceProcessedAlready returns true if there is no further processing
+// needed for the instance based on ObservedGeneration
+func isServiceInstanceProcessedAlready(instance *v1beta1.ServiceInstance) bool {
+	return instance.Status.ObservedGeneration >= instance.Generation &&


I would really appreciate a comment here that explains this logic.

pmorie · 2018-03-01T23:12:06Z

pkg/controller/controller_instance_test.go

+		deprovisionStatus    v1beta1.ServiceInstanceDeprovisionStatus
+		serviceBinding       *v1beta1.ServiceBinding
+		generation           int64
+		reconciledGeneration int64


thank you, lol

pmorie · 2018-03-01T23:59:14Z

I'll leave LGTM2 to @kibbles-n-bytes, but this has my stamp of approval, thanks a lot for driving this to completion.

kibbles-n-bytes · 2018-03-02T18:09:28Z

sorry for the delay; diving into this now.

kibbles-n-bytes

LGTM! If you want to address the Failed condition stuff later I'm not opposed; it's more applicable for the retries/reattempt situations anyway.

kibbles-n-bytes · 2018-03-02T19:31:10Z

pkg/controller/controller_instance.go

+	toUpdate.Status.ObservedGeneration = toUpdate.Generation
+	reason := observedNewGenerationReason
+	message := observedNewGenerationMessage
+	setServiceInstanceCondition(


This condition will never actually surface to the user as far as I'm aware. Whenever we update the status of our resource, we also update the Ready condition, so it'll always be overwritten.

kibbles-n-bytes · 2018-03-02T19:38:10Z

pkg/controller/controller_instance.go

+	setServiceInstanceCondition(
+		toUpdate,
+		v1beta1.ServiceInstanceConditionFailed,
+		v1beta1.ConditionFalse,


I'd argue that instead of setting the Failed condition to false, we should be removing the condition entirely. That seems the more Kube-way™ to me. For example, when transient errors occur on Deployments during pod creation, they are tagged with the ReplicaFailure condition, but once they're resolved that condition is removed as opposed to unset. And I think it makes the user experience more straightforward during reattempts after spec edits, as the conditions a user would see wouldn't be different depending on whether they experienced a failure in the past, and they wouldn't have to walk through the logic of "oh, I have a Failed condition; wait, but it's false, so we're not actually failed".

kibbles-n-bytes · 2018-03-02T19:39:17Z

pkg/controller/controller_instance.go

 	updatedInstance, err := c.serviceCatalogClient.ServiceInstances(toUpdate.Namespace).UpdateReferences(toUpdate)
 	if err != nil {
 		glog.Errorf(pcb.Messagef("Failed to update references: %v", err))
 	}
+	// The UpdateReferences method ignores status changes.
+	// Restore status that might have changed locally to be able to update it later.
+	updatedInstance.Status = status


Yup, seems reasonable, and with the no-multi-update changes coming it probably won't end up mattering soon anyway. 😃

nilebox · 2018-03-03T01:24:27Z

I'll address remaining comments in other PRs.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 20, 2018

Nail Islamov added 7 commits February 20, 2018 21:02

Add ObservedGeneration and Provisioned into ServiceInstanceStatus

66b51b5

Fix tests

f15cda5

Add test for API migration path

c033bb5

Add checking ObservedGeneration to tests

d0efd0f

Add Provisioned flag checks to unit tests

1816912

Update generated files

50a81bb

Determine reconciled generation based on ObservedGeneration and Ready…

fd2c9e7

…/Failed conditions

nilebox changed the title ~~WIP: Add ObservedGeneration and Provisioned into ServiceInstanceStatus~~ Add ObservedGeneration and Provisioned into ServiceInstanceStatus Feb 21, 2018

Fix typo in comment

4091566

nilebox mentioned this pull request Feb 21, 2018

Switch from ReconciledGeneration to ObservedGeneration (or change its semantics) #1747

Closed

Fix test for reconciled generation

4fcefee

kibbles-n-bytes reviewed Feb 22, 2018

View reviewed changes

Nail Islamov added 3 commits February 22, 2018 12:50

Update cached instance after update succeeds

200d0e4

Add debugging information for the test

8990e75

Bugfix: set Failed condition in case of terminal failure during update

cc99cce

nilebox commented Feb 22, 2018

View reviewed changes

Remove debugging information

77c79ba

nilebox assigned pmorie, kibbles-n-bytes, jboyd01 and staebler Feb 25, 2018

nilebox mentioned this pull request Feb 26, 2018

4xx, 5xx and Connection timeout should be retriable (not terminal errors) #1765

Merged

kibbles-n-bytes reviewed Feb 26, 2018

View reviewed changes

staebler mentioned this pull request Feb 26, 2018

Allow retries for instances with Failed condition after spec changes #1751

Merged

staebler reviewed Feb 26, 2018

View reviewed changes

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 27, 2018

nilebox commented Feb 27, 2018

View reviewed changes

Restore status after invoking UpdateReferences

c1405f8

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 27, 2018

nilebox commented Feb 27, 2018

View reviewed changes

staebler reviewed Feb 28, 2018

View reviewed changes

Fix review comments

afad585

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2018

Nail Islamov added 2 commits February 28, 2018 17:37

Fix tests (ObservedGeneration for orphan mitigation)

a4389d4

Fix test

2af0932

staebler approved these changes Mar 1, 2018

View reviewed changes

staebler added the LGTM1 label Mar 1, 2018

pmorie reviewed Mar 1, 2018

View reviewed changes

Fix review comments

bc770d1

kibbles-n-bytes approved these changes Mar 2, 2018

View reviewed changes

kibbles-n-bytes added the LGTM2 label Mar 2, 2018

nilebox merged commit e5c37ad into kubernetes-retired:master Mar 3, 2018

nilebox deleted the observed-generation branch March 3, 2018 01:25

nilebox mentioned this pull request Mar 5, 2018

Remove Failed condition if there was no terminal failure #1788

Merged

MHBauer mentioned this pull request Mar 6, 2018

Synchronize some generated code that was missed along the way #1801

Merged

nilebox added the non-happy-path label Mar 7, 2018

cblecker unassigned kibbles-n-bytes, jboyd01 and staebler Jun 4, 2019

Add ObservedGeneration and Provisioned into ServiceInstanceStatus #1748

Add ObservedGeneration and Provisioned into ServiceInstanceStatus #1748

Conversation

nilebox commented Feb 20, 2018 • edited Loading

kibbles-n-bytes Feb 22, 2018 • 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

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 Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

nilebox Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

nilebox commented Feb 22, 2018

MHBauer commented Feb 24, 2018

nilebox commented Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibbles-n-bytes Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox Feb 26, 2018 • 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

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

nilebox Feb 27, 2018 • 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

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

nilebox commented Feb 28, 2018

staebler 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

pmorie commented Mar 1, 2018

kibbles-n-bytes commented Mar 2, 2018

kibbles-n-bytes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibbles-n-bytes Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox commented Mar 3, 2018

nilebox commented Feb 20, 2018 •

edited

Loading

kibbles-n-bytes Feb 22, 2018 •

edited

Loading

nilebox Feb 26, 2018 •

edited

Loading

nilebox Feb 26, 2018 •

edited

Loading

nilebox commented Feb 24, 2018 •

edited

Loading

kibbles-n-bytes Feb 26, 2018 •

edited

Loading

nilebox Feb 26, 2018 •

edited

Loading

nilebox Feb 27, 2018 •

edited

Loading

kibbles-n-bytes left a comment •

edited

Loading

kibbles-n-bytes Mar 2, 2018 •

edited

Loading