base reconciler reconciles status

[eguzki]

What

Common status reconciler. The signature

func(ctx context.Context, objKey client.ObjectKey, obj client.Object, mutator StatusMutator) 

Then, it can be used as follows:

        newStatus := r.calculateStatus(rlp, specErr)
         r.ReconcileResourceStatus(                                                            
              ctx,                                                                                        
              client.ObjectKeyFromObject(rlp),                                                            
             &kuadrantv1beta2.RateLimitPolicy{},                                                         
             kuadrantv1beta2.RateLimitPolicyStatusMutator(newStatus),                            
       )

And the mutator looks like

  func RateLimitPolicyStatusMutator(desiredStatus *RateLimitPolicyStatus) reconcilers.StatusMutatorFunc {
        return func(obj client.Object) (bool, error) {                                                  
              existingRLP, ok := obj.(*RateLimitPolicy)                                                   
              if !ok {                                                                                    
                   return false, fmt.Errorf("unsupported object type %T", obj)                             
              }                                                                                           
                                                                                                                                                                                                                                                                                     
             if cmp.Equal(*desiredStatus, existingRLP.Status, opts) {                                    
                  return false, nil                                                                       
              }                                                                                           
                                                                                                     
              existingRLP.Status = *desiredStatus                                                         
                                                                                                     
              return true, nil
}

Work being done as part of #530

This is part of a series of PR's that will end with a new controller that will only reconcile limitador limits configuration.

[codecov]

Codecov Report

Attention: Patch coverage is 67.30769% with 17 lines in your changes missing coverage. Please review.

Project coverage is 82.86%. Comparing base (ece13e8) to head (f694367).
Report is 138 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #752      +/-   ##
==========================================
+ Coverage   80.20%   82.86%   +2.66%     
==========================================
  Files          64       77      +13     
  Lines        4492     6001    +1509     
==========================================
+ Hits         3603     4973    +1370     
- Misses        600      677      +77     
- Partials      289      351      +62     

Flag	Coverage Δ
bare-k8s-integration	4.53% <0.00%> (?)
controllers-integration	72.76% <67.30%> (?)
gatewayapi-integration	11.16% <0.00%> (?)
integration	?
istio-integration	56.28% <67.30%> (?)
unit	33.09% <3.84%> (+3.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	71.42% <ø> (ø)
api/v1beta2 (u)	90.81% <87.20%> (-0.62%)	⬇️
pkg/common (u)	88.13% <ø> (-0.70%)	⬇️
pkg/istio (u)	73.88% <ø> (-0.03%)	⬇️
pkg/log (u)	94.73% <ø> (ø)
pkg/reconcilers (u)	∅ <ø> (∅)
pkg/rlptools (u)	83.33% <ø> (+3.87%)	⬆️
controllers (i)	82.09% <84.28%> (+5.29%)	⬆️

Files	Coverage Δ
controllers/ratelimitpolicy_status.go	95.83% <90.90%> (+14.35%)	⬆️
api/v1beta2/ratelimitpolicy_types.go	88.31% <61.11%> (-0.78%)	⬇️
pkg/library/reconcilers/base_reconciler.go	65.17% <60.86%> (ø)

... and 39 files with indirect coverage changes

[Boomatang]

I am not sure about this silently failing if the wrong kind of StatusObject is passed in, it seems wrong. At the same time I don't think just logging an error message is enough.

[didierofrivia]

Maybe we could return error if as a default case (nothing matched) and we set the status to nil ? we could bubble up that error and decide given the setter expectations what to do

[eguzki]

before considering returning an error, any suggestion?

[Boomatang]

My mind is starting to go to extremes and I really don't know how well it suits. I am thinking that raising a panic might be the correct approach. If we return an error the reconcile ends up doing possibly twos things. One logs the error message, and two, reschedule the reconcile to try again. But if the wrong type is being past in, it was most likely an error caused by a developer in the code base where no matter how many times it is reschedule, reconcile, it will never pass.

I know causing the panic is extreme and not really done in operators, but I don't see how it would ever natural recover.

I do wonder what this would look like if the function is made more generalized and taken off the struct its self. But possible would have other issue, along with some of the same issue that I can currently visualize in my head.

[eguzki]

thanks for the ideas... let me think and try something else. I reckon the type assertion is a bad smell.

[eguzki]

@Boomatang

I have tried a different approach. Iteration number 2.

I do not like the panic approach because in production you kill the controller entirely, while returning an error and doing nothing only disables one feature.

This is not entirely clean approach either and there is still a chance to mistakenly mix types by the developer, so the status updater tries again and again returning error. But it is also true that it is the same mutator approach as the controllers use to reconcile resource specs (not status). It is being used by all the controllers and never hit any issue like that. So I think it should be good enough.

You might be interested in looking at the envoygateway controller implementation of the generic status updater: https://github.com/envoyproxy/gateway/blob/main/internal/provider/kubernetes/status_updater.go#L72-L121

[Boomatang]

There was the one documentation issue, but other than that every thing is working as I expected. This newer method seems far nicer than before.

[eguzki]

There was the one documentation issue, but other than that every thing is working as I expected. This newer method seems far nicer than before.

good catch. Addressed

[Boomatang]

Looks good to me