KDP Reconciler does not set observedGeneration on the managed resource

[sdowell]

What would you like to be added: I would like for the KDP reconciler to have built-in support for setting the observedGeneration on the status of the managed resource.

Why is this needed: This makes it easier to determine when a KDP controller has reconciled the current spec of the managed resource.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tomasaschan]

I agree this would be useful, and shouldn't be terribly hard to implement. I guess the "hard" part is to determine whether the declarative resource definition has status.observedGeneration defined in its schema, to know whether to attempt to set it or not; without that (or an explicit opt-in, which is probably easier to implement), this can never be non-breaking.

[tomasaschan]

/remove-lifecycle stale

[tomasaschan]

Thinking a bit more about this, it is already possible to do this with the current features of KDP, by utilizing the BuildStatus API of the status builder:

// in your setup code, where you pass all the other options to set up the declarative reconciler

// We use a StatusBuilder to avoid having to implement all of declarative.Status; we only need BuildStatus
// https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/blob/master/pkg/patterns/declarative/status.go#L80
declarative.WithStatus(&declarative.StatusBuilder{
  BuildStatusImpl: &myCustomStatus{},
})


type myCustomStatus struct {}

func (*myCustomStatus) BuildStatus(ctx context.Context, statusInfo *StatusInfo) error {
    // here, statusInfo has a property Subject which is the declarative object
    // cast that to your custom resource type, so you can mutate the status at will
    subj, ok := statusInfo.Subject.(*MyCustomResource)
    if !ok {
        return errors.New("expected a %T, but got a %T", &MyCustomResource{}, statusInfo.Subject)
    }
    
    // mutations to status here will be saved to the API server by the KDP reconciler
    subj.Status.ObservedGeneration = subj.ObjectMeta.Generation

    return nil
}

This is admittedly quite a bit of ceremony in order to do something very common, but if you also want to do other things (e.g. calculate a set of conditions) you probably already do most of it, and the line that actually sets observed generation is the only new code needed.

It might be interesting to try to simplify this, by e.g. providing a helper feature that removes the need for all the boilerplate, but we'd have to design that a bit carefully to ensure it plays well with the existing status features.

@justinsb Do you have any thoughts on this? In particular, could we make it easier to build helpers for things like this by finally actually removing the old-and-deprecated methods on StatusBuilder (in particular, Reconciled)? (Yes, that would be breaking, but at least in a very compile-time discoverable way.)