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.

Sign up for GitHub

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

Jump to bottom

EndpointSlice resources tracked by Graph #1432

Merged
merged 41 commits into from
Jan 24, 2024
Merged

EndpointSlice resources tracked by Graph #1432

merged 41 commits into from
Jan 24, 2024

Conversation

bjee19
Copy link
Contributor

@bjee19 bjee19 commented Dec 29, 2023 •
edited
Loading

Proposed changes

Problem: EndpointSlice resources are currently being tracked by the relationship capturer, but to reduce the number of components we would like to track the resources in the Graph.

Solution: Enable Graph to track EndpointSlice resources and Services. In addition, this PR refactors backend_refs to store the NamespacedName of a Service and a ServicePort instead of the actual Service. This is done to track non-existing referenced services. Also, refactors and removes the alwaysTruePredicate introduced in this PR. Removes RelationshipCapturer.

Testing: Wrote new unit tests and refactored existing ones.

Closes #1264

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

@github-actions github-actions bot added the tech-debt Short-term pain, long-term benefit label Dec 29, 2023
@bjee19
Copy link
Contributor Author

bjee19 commented Dec 30, 2023

Had a few questions / checks on the logic of the changes I would like to have looked at before completing the changes and adding tests, Thanks!

kate-osborn
kate-osborn reviewed Jan 2, 2024
View reviewed changes
internal/mode/static/state/graph/graph.go Show resolved Hide resolved
internal/mode/static/state/graph/service.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Show resolved Hide resolved
internal/mode/static/state/change_processor.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Outdated Show resolved Hide resolved
@kate-osborn kate-osborn mentioned this pull request Jan 3, 2024
Merged
6 tasks
kate-osborn
kate-osborn requested changes Jan 10, 2024
View reviewed changes
internal/mode/static/state/changed_predicate.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Outdated Show resolved Hide resolved
internal/mode/static/state/resolver/resolver.go Show resolved Hide resolved
internal/mode/static/state/resolver/resolver.go Show resolved Hide resolved
internal/mode/static/state/resolver/resolver.go Outdated Show resolved Hide resolved
@bjee19 bjee19 mentioned this pull request Jan 11, 2024
Closed
@bjee19 bjee19 marked this pull request as ready for review January 12, 2024 23:58
@bjee19 bjee19 requested a review from a team as a code owner January 12, 2024 23:58
@bjee19 bjee19 changed the title Draft: EndpointSlice resources tracked by Graph EndpointSlice resources tracked by Graph Jan 12, 2024
pleshakov
pleshakov reviewed Jan 16, 2024
View reviewed changes
internal/mode/static/state/graph/backend_refs.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/service.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/service.go Outdated Show resolved Hide resolved
internal/mode/static/state/store.go Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/backend_refs.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/resolver/resolver.go Outdated Show resolved Hide resolved
@bjee19 bjee19 force-pushed the debt/endpointslice-resources-tracked-by-graph branch 2 times, most recently from dbb211a to 583738a Compare January 18, 2024 23:13
kate-osborn
kate-osborn requested changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice job, @bjee19! This makes our code much simpler 🚀

internal/mode/static/state/change_processor_test.go Show resolved Hide resolved
internal/mode/static/state/changed_predicate.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/service.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/service.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/service_test.go Show resolved Hide resolved
internal/mode/static/state/resolver/service_resolver_test.go Show resolved Hide resolved
internal/mode/static/state/store.go Outdated Show resolved Hide resolved
internal/mode/static/state/store.go Show resolved Hide resolved
@bjee19 bjee19 requested a review from pleshakov January 23, 2024 19:12
pleshakov
pleshakov approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

bjee19 added 27 commits January 23, 2024 15:37
@bjee19
Remove capturer
5109dcf
@bjee19
Add service tests
27fd279
@bjee19
Add backendRef tests
318302e
@bjee19
Refactor part of change processor tests to work correctly
af48643
@bjee19
Remove tests from change_processor
8e29ddd
@bjee19
Fix function layout in change processor tests
d890a78
@bjee19
Change function layout in change processor tests
1e3d23b
@bjee19
Adjust comments in backendRef
35b548a
@bjee19
Remove FIXME
7852824
@bjee19
Add assert on Process call
22227f8
@bjee19
Refactor comments and logic in backendRefs
060d356
@bjee19
Remove unnecessary comments
feef30a
@bjee19
Remove mentions of capturer
c7e6a39
@bjee19
Refactor tests to not use pass/fail in name
bd0dad7
@bjee19
Remove some comments
9bdc838
@bjee19
Rename to ReferencedServices
32307fb
@bjee19
Remove unnecessary checks in service
58d9c5c
@bjee19
Move comment
a86a0d0
@bjee19
Rebase and remove FIXME
88e8d24
@bjee19
Add feedback from review
8513f27
@bjee19
Add persists to multiObjectStore
4efe47e
@bjee19
Remove alwaysTrue predicate and resolve FIXME
1580fe1
@bjee19
Add back some change_processor tests
a42e2d2
@bjee19
Remove persistedGVKs from changeTrackingUpdater
cebcc7a
@bjee19
Change function documentation
b8d9a89
@bjee19
Add namespace resources to multi k8s api resource change tests
3146c18
@bjee19
Add secret resources to change processor tests
635178b
@bjee19 bjee19 force-pushed the debt/endpointslice-resources-tracked-by-graph branch from c129995 to 635178b Compare January 23, 2024 23:37
@bjee19 bjee19 merged commit 73f7918 into nginx:main Jan 24, 2024
27 checks passed
@bjee19 bjee19 deleted the debt/endpointslice-resources-tracked-by-graph branch May 7, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjberman sjberman sjberman left review comments

@kate-osborn kate-osborn kate-osborn approved these changes

@pleshakov pleshakov pleshakov approved these changes

Assignees
No one assigned
Labels
tech-debt Short-term pain, long-term benefit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EndpointSlice resources are tracked by Graph
4 participants
@bjee19 @pleshakov @sjberman @kate-osborn