Check resource generation when processing updates of some resources to skip config regeneration

[pleshakov]

When processing updates to cluster resources, for some resources, we check their generation, so that we don't trigger state change (graph rebuild) if the generation didn't change. This is a performance optimization so that we don't rebuild the graph and as a result do not regenerate NGINX config and reload it.

However, for resources don't trigger state change on upsert, but for which the relationship capturer or triggerStateChange func determines if the resource triggers a change, we don't check the resource generation.

FIXME https://github.com./nginxinc/nginx-kubernetes-gateway/blob/36d5df4f5b047eadcbc2b814f19c216a054c2d6c/internal/state/store.go#L214

We can also filter resource events at the controller level by adding the controller-runtime GenerationChangedPredicate. This would eliminate the need to check the generation in the changeTrackingUpdater and reduce the number of events we process. Adding this predicate may not make sense for all the resources since the generation of a resource is only incremented for spec changes. Changes to metadata -- such as labels and annotations -- do not increment the generation.

Acceptance criteria:

• check generation for those cases above

[kevin85421]

However, for resources don't trigger state change on upsert, but for which the relationship capturer or triggerStateChange func determines if the resource triggers a change, we don't check the resource generation.

• Do you mean to add the generation check as pseudo code below in both Upsert and Delete functions? Thanks!

func (s *changeTrackingUpdater) Upsert(obj client.Object) {
	s.assertSupportedGVK(s.extractGVK(obj))

	changingUpsert := s.upsert(obj)
        if generation doesn't change {
	    relationshipExisted := s.capturer.Exists(obj, client.ObjectKeyFromObject(obj))
    
	    s.capturer.Capture(obj)
    
	    relationshipExists := s.capturer.Exists(obj, client.ObjectKeyFromObject(obj))
    
	    // FIXME(pleshakov): Check generation in all cases to minimize the number of Graph regeneration.
	    // s.changed can be true even if the generation of the object did not change, because
	    // capturer and triggerStateChange don't take the generation into account.
	    // See https://github.com./nginxinc/nginx-gateway-fabric/issues/825
        }
	s.changed = s.changed || changingUpsert || .....
}

[pleshakov]

Hi @kevin85421

The code in question was refactored recently. Now, we configure generation check here as a predicate:

• https://github.com./nginxinc/nginx-gateway-fabric/blob/bcd8c5eaeec0eff7bf114f912676f1b3958495a4/internal/mode/static/state/change_processor.go#L125-L172

However, the controller runtime framework also supports generation check at the reconciler (controller) level (before changed resources are propagated to the ChangeProcessor) using https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/predicate#GenerationChangedPredicate . For example, we do that for EndpointSlices here https://github.com./nginxinc/nginx-gateway-fabric/blob/bcd8c5eaeec0eff7bf114f912676f1b3958495a4/internal/mode/static/manager.go#L309 when we configure the controller for EndpointSlices

I think what makes sense now is to:

• Move generation check from ChangeProcessor to Controllers, for the types we already check generation
• Figure out which types miss generation check and add it.

@kate-osborn thoughts?

[kevin85421]

Thanks, @pleshakov! I have already drafted PR #1422, which includes changes for the requirement "Move generation check from ChangeProcessor to Controllers, for the types we already check generation". It would be great if I could get some early feedback before starting work on the second requirement and adding tests.