Use Generation instead of checksum to detect changes in spec

[pmorie]

There is a field in ObjectMeta called Generation that represents a specific version of an object's spec.

This field is used to solve the 'have i reconciled this version of this object's spec yet?' problem as follows:

• The resource's REST strategy bumps the Generation field when the the spec changes
• The resource's status has a field called ObservedGeneration that contains the last successfully reconciled value of Generation
• When a controller reconciles an object, it can check to see whether the Generation and Status.ObservedGeneration match; if they match there is no work to do.

This seems like a much better solution than using a checksum, and has more precedent in the existing API. I propose that we adopt this for Instance and Binding types. I believe Broker should use the same pattern, but there are complications, as I have written about here.

[pmorie]

This should definitely be done before moving the API to beta, so I have set the 0.1.0 milestone.

[duglin]

@pmorie was there a resolution on whether Brokers can be Generation or not?

[pmorie]

@duglin I think there's no reason why Broker cannot use generation.

[pmorie]

design proposal coming

[pmorie]

Proposed changes

1. Remove the Checksum field from {Broker,Instance,Binding}Status and all associated checksum calculation code
2. The rest strategies for {Broker,Instance,Binding} should be made to increment Generation when the spec is changed
3. Add a field called ReconciledGeneration to {Broker,Instance,Binding}Status
4. The controller should set the value of ReconciledGeneration to be the value of Generation when it sets a resource's ready condition to have status true

Special considerations for Broker

This is a touchpoint with issue #705 for broker relist API surface.

Brokers must support manual relist. There should be a field added for BrokerSpec called RelistRequests that is a monotonically increasing counter:

type BrokerSpec struct {
	// other fields omitted

	RelistRequests int `json:"relistRequests"`
}

This field should have a default value of zero, and should only be allowed to increase by one.  These are somewhat arbitrary semantics, but they seem to be the simplest thing that can work.  

[nilebox]

There should be a field added for BrokerSpec called RelistRequests that is a monotonically increasing counter

1. Is there any example of using such pattern in Kubernetes core?
2. What's the benefit of having incremental counter as opposed to just bool?

NeedsRelist bool `json:"needsRelist"`

[duglin]

ReconciledGeneration or ObservedGeneration? I see Observed in Deployment.

I'm assuming we'll have code that will detect a request to modify NeedsRelist and aside from doing the "is it only +1" check you mentioned, if that passes, then that will force the Generation value to be different in some way from ReconciledGeneration, right?

trying to think thru the case of two requests to relist coming in at the same time. Both will send the new ordinal value and both will have the correct Version value but I suspect only one will win. The other will fail causing them to re-GET and resend with the next ordinal value of NeedsRelist (+1 above other request). I think that's ok. Plus if the failed requester checks the error and see it failed due to the +1 check and not the Version mismatch then it can know it failed because someone else had already asked for a relist.

[duglin]

@pmorie I am curious though, was there an issue with the /sync sub-resource type of solution?

[pmorie]

ReconciledGeneration or ObservedGeneration? I see Observed in Deployment.

I'm fine with either. ObservedGeneration has precedent, so that's probably better. I don't remember having a specific reason for ReconciledGeneration

[pmorie]

@pmorie I am curious though, was there an issue with the /sync sub-resource type of solution?

For the record, no issue with the approach other than a little extra code, but we seem to be converging on a subresource that bumps RelistRequests for broker starting here

[pmorie]

What's the benefit of having incremental counter as opposed to just bool?

If we have a bool, then the controller needs to unset the bool when it's done... which causes another reconcile because the spec changed. If we have an incremental counter, the controller doesn't have to do anything, and the resource behaves like other resources (spec change results in re-reconcile, controller doesn't have to mutate spec, etc).

[duglin]

ah, missed the causing another reconcile... thanks

[pmorie]

Actually I remember now why I suggested Reconciled instead of Observed - because the controller can't take all the steps it needs to necessarily in a single reconciliation to 'finish' actions for a particular version of the spec. Take for example an async provision. The controller might observe it and have to re-reconcile a couple times to complete the reconciliation. So I think the field should represent what has actually been successfully reconciled vs. what's merely been observed.

So, specific examples

synchronous provision

1. Instance is created
2. Controller calls provision at broker
3. Broker provisions synchronously and returns
4. Controller updates status.reconciledGeneration

asynchronous provision

1. Instance is created
2. Controller calls provision at broker
3. Broker begins an asynchronous provision and returns 202
4. Controller shouldn't update status.reconciledGeneration
5. Controller makes a condition (like it does today representing async op in progress)
6. Controller wakes up and calls last_operation endpoint on broker and gets success
7. now controller should update status.reconciledGeneration

Make sense? In the end, I guess the name doesn't matter as much as making sure the semantics of the field are clear to users and programmers.

[duglin]

In the end, I guess the name doesn't matter as much as making sure the semantics of the field are clear to users and programmers.

yup - and it was for consistency that I was thinking we'd want to dup what other kube resources have.

One question I have, and its related to what we we're talking about off-line, in your async case in step 7 what value does the controller use for state.reconciledGenerated? It can't just grab the latest value of "generation" it seems like it would need to cache the value at the time it started the reconciliation process.

[pmorie]

@duglin and I spent some time talking about asynchronous operations this weekend, and we realized that there is a need to store the generation that an asynchronous operation is for. This applies to instances only currently, but would apply to bindings (or, ahem ServiceInstanceCredential) as well if async binding is adopted in the OSB API.

[pmorie]

Closed by #1145 & #1151