Switch from ReconciledGeneration to ObservedGeneration (or change its semantics)

[nilebox]

Currently, if ReconciledGeneration is equal to instance's Generation, we don't process Add/Update events:

https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/controller/controller_instance.go#L257

if instance.Status.ReconciledGeneration == instance.Generation {
	glog.V(4).Info(pcb.Message("Not processing event because reconciled generation showed there is no work to do"))
	return nil
}

Hence, as far as I can see, we don't update ReconciledGeneration if the retriable error occurs.

This is bad, because then the user can't determine which Generation the current status conditions are relevant for (and whether a particular Generation has been processed).

Correct semantics is ObservedGeneration:

1. Always set ObservedGeneration along with updating status (even if there was a temporary retriable error)
2. The condition for reconcile function (see above) should be changed to:

if instance.Status.ObservedGeneration == instance.Generation && (isReady(instance.Status) || isFailed(instance.Status)) {
	// Nothing to do
	return nil
}

i.e. additionally to checking if ObservedGeneration == Generation, also check if we have actually finished processing this particular version of resource.

[nilebox]

This issue is related to the proposed changes for retries #1715, but can be resolved independently

[nilebox]

To support such usage of ObservedGeneration - deployments: deployment_util.go#L791

// DeploymentComplete considers a deployment to be complete once all of its desired replicas
// are updated and available, and no old pods are running.
func DeploymentComplete(deployment *extensions.Deployment, newStatus *extensions.DeploymentStatus) bool {
	return newStatus.UpdatedReplicas == *(deployment.Spec.Replicas) &&
		newStatus.Replicas == *(deployment.Spec.Replicas) &&
		newStatus.AvailableReplicas == *(deployment.Spec.Replicas) &&
		newStatus.ObservedGeneration >= deployment.Generation
}

[ash2k]

Item 10 from the list:

If the primary resource your controller is reconciling supports ObservedGeneration in its status, make sure you correctly set it to metadata.Generation whenever the values between the two fields mismatches.

This lets clients know that the controller has processed a resource. Make sure that your controller is the main controller that is responsible for that resource, otherwise if you need to communicate observation via your own controller, you will need to create a different kind of ObservedGeneration in the Status of the resource.

Current reconciledGeneration has different semantics and hence the client does not have means to find out where the controller has seen the update or not (separate concern to whether the update has been processed successfully or has failed).

[nilebox]

There are places where switching from ReconciledGeneration to ObservedGeneration could be tricky, e.g.

	isProvisioning := false
	if instance.Status.ReconciledGeneration == 0 {
		isProvisioning = true
	}

or

	case instance.Status.ReconciledGeneration != 0:
		return reconcileUpdate
	default: // instance.Status.ReconciledGeneration == 0
		return reconcileAdd

so maybe we need both (and set ReconciledGeneration only after success).
Or we could just store Provisioned bool additionally to ObservedGeneration.

[pmorie]

I'm processing all this still. At first, I'm not exactly sure what this buys us - do we need the new fields, or do we need to be smarter about when we updated ReconciledGeneration?

[nilebox]

@pmorie we have a number of problems with ReconciledGeneration:

1. As described in this issue, we need to tell the user which version the status is relevant for (including intermediate progress status, not just which version we have finished processing), i.e. ReconciledGeneration doesn't provide enough information.
2. You can find examples of ObservedGeneration in Kubernetes code, while I haven't found analogue of ReconciledGeneration there, i.e. ObservedGeneration is a "kube way".
3. As I mentioned in Deleting a ServiceInstance or ServiceBinding while async operation in progress fails #1587 (comment), the problem of "accidental" marking a deleted instance with ReconciledGeneration and leading to no-op would not even existed.
4. Currently, after you deprovision an instance, it's still considered as "provisioned" since ReconciledGeneration != 0. As I mentioned in Instances: 4xx, 5xx and Connection timeout should be retriable (not terminal errors) #1715 (comment) (item 3.), we will need to check whether we need to set ReconciledGeneration to the latest generation or 0 ("reset") after deprovisioning (depending on whether deprovisioning was due to DeletionTimestamp or orphan mitigation).

With ObservedGeneration and separate Provisioned flag these problems go away.

[nilebox]

do we need to be smarter about when we updated ReconciledGeneration

I think that ReconciledGeneration is redundant, and raised a PR #1748 where I marked ReconciledGeneration as deprecated, and still set for backward compatibility, but don't read it for any reconciliation purpose anymore.

[nilebox]

Closed by #1748