Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Remove unused data from persistedGVKs #1358

Closed
wants to merge 1 commit into from

Conversation

kevin85421
Copy link
Contributor

@kevin85421 kevin85421 commented Dec 9, 2023

Proposed changes

I am not sure whether my understanding is correct. Please correct me if I am wrong.

Both apiv1.Namespace and apiv1.Service are stored in persistedGVKs. However, setting either store [1] or predicate [2] to nil implies that a state change will not trigger a graph rebuild. In such cases, only Capturer can trigger the graph rebuild, and Capturer does not require information from either store (i.e. persistedGVKs) or predicate (i.e. stateChangedPredicates). Therefore, store and predicate should be either both set or both unset.

This PR sets store: nil for both apiv1.Namespace and apiv1.Service to make both store and predicate to be nil.

Screen Shot 2023-12-09 at 10 43 57 AM

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Both apiv1.Namespace and apiv1.Service are stored in persistedGVKs. However, setting either store or predicate to nil implies that a state change will not trigger a graph rebuild. In such cases, only Capturer can trigger the graph rebuild, and Capturer does not require information from either store or predicate. Therefore, store and predicate should be either both set or both unset. This PR sets `store: nil` for both apiv1.Namespace and apiv1.Service
Copy link

netlify bot commented Dec 9, 2023

Deploy Preview for nginx-gateway-fabric canceled.

Name Link
🔨 Latest commit 01c9dfa
🔍 Latest deploy log https://app.netlify.com/sites/nginx-gateway-fabric/deploys/6574b2820c6b6e000872409b

@kevin85421 kevin85421 marked this pull request as ready for review December 9, 2023 18:40
@kevin85421 kevin85421 requested a review from a team as a code owner December 9, 2023 18:40
@kevin85421
Copy link
Contributor Author

kevin85421 commented Dec 10, 2023

On second thought, I think one possible reason for storing apiv1.Namespace and apiv1.Service in persistedGVKs is to check generation for capturer, which is not currently being checked (#825). If this is correct, I will close this PR.

@kate-osborn
Copy link
Contributor

@kevin85421

The store updates the underlying clusterState which is used to build the graph.

Both Namespaces and Services are needed in the clusterState so the graph can bindRoutesToListeners and addBackendRefsToRouteRules.

We are planning on simplifying the changeTrackingUpdater by removing the relationshipCapturer, so we only have one source of truth (the latest graph).

See this ticket for more information: #824
Some of this work is already in progress: #1320

@kevin85421
Copy link
Contributor Author

@kate-osborn Thanks! I did not notice that ClusterState's maps are passed to the store via newObjectStoreMapAdapter(clusterStore.XXXXX).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants