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

Do not trigger DAG rebuild for irrelevant HTTProutes/TLSroutes #4912

Merged
merged 6 commits into from
Dec 16, 2022

Conversation

fangfpeng
Copy link
Contributor

@fangfpeng fangfpeng commented Dec 8, 2022

For HTTProute/TLSRoute, only if it references the relevant Gateway, it will trigger DAG rebuild.

Closes #4800.

Signed-off-by: fpeng fpeng@vmware.com

@fangfpeng
Copy link
Contributor Author

@skriss @sunjayBhatia Please help review

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #4912 (5b11814) into main (30320aa) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4912      +/-   ##
==========================================
+ Coverage   76.45%   76.47%   +0.01%     
==========================================
  Files         140      140              
  Lines       16711    16724      +13     
==========================================
+ Hits        12777    12790      +13     
  Misses       3684     3684              
  Partials      250      250              
Impacted Files Coverage Δ
internal/dag/cache.go 94.26% <100.00%> (+0.17%) ⬆️

@fangfpeng fangfpeng marked this pull request as ready for review December 9, 2022 19:15
@fangfpeng fangfpeng requested a review from a team as a code owner December 9, 2022 19:15
@fangfpeng fangfpeng requested review from tsaarni and sunjayBhatia and removed request for a team December 9, 2022 19:15
@sunjayBhatia sunjayBhatia requested a review from skriss December 13, 2022 14:18
@skriss skriss changed the title Donot trigger DAG rebuild for irrelevant HTTProutes/TLSroutes Do not trigger DAG rebuild for irrelevant HTTProutes/TLSroutes Dec 14, 2022
internal/dag/cache.go Outdated Show resolved Hide resolved
internal/dag/cache.go Outdated Show resolved Hide resolved
internal/dag/cache.go Outdated Show resolved Hide resolved
@skriss
Copy link
Member

skriss commented Dec 14, 2022

Now that we've merged the dev-gwapi-0.6 branch into main, we'll want to rebase this PR onto main and target main to get it merged. We can walk through this together on Friday.

@skriss skriss added the release-note/small A small change that needs one line of explanation in the release notes. label Dec 15, 2022
@skriss skriss changed the base branch from dev-gwapi-0.6 to main December 15, 2022 15:29
@skriss
Copy link
Member

skriss commented Dec 15, 2022

Retargeted the PR to main and now needs a rebase.

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Just one more minor comment, otherwise this looks pretty good once it's rebased.

internal/dag/cache.go Outdated Show resolved Hide resolved
fangfpeng and others added 4 commits December 15, 2022 14:38
For HTTProute/TLSRoute, only if it references the relevant Gateway, it
will trigger DAG rebuild.

Signed-off-by: fpeng <fpeng@vmware.com>
Do not insert irrelevant HTTPRoute/TLSRoute to cache.
Make test changes due to this change.

Also add postive test when removing HTTPRoute/TLSRoute.

Signed-off-by: fpeng <fpeng@vmware.com>
Do not insert irrelevant HTTPRoute/TLSRoute to cache.
Make test changes due to this change.

Also add postive test when removing HTTPRoute/TLSRoute.

Signed-off-by: fpeng <fpeng@vmware.com>
need to add route to the cache always so revert
some of the previous changes.

Updates projectcontour#4800

Signed-off-by: Fang Peng <fpeng@vmware.com>
@skriss
Copy link
Member

skriss commented Dec 15, 2022

One last thing to address: please add a file changelogs/unreleased/4912-fangfpeng-small.md, that includes a brief user-facing description of the change, something along the lines of "Don't trigger DAG rebuilds/xDS configuration updates for irrelevant HTTPRoutes and TLSRoutes".

Add change log and address review comment
regarding to reducing nesting.

Updates projectcontour#4800

Signed-off-by: Fang Peng <fpeng@vmware.com>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM!

Updates projectcontour#4800

Signed-off-by: Fang Peng <fpeng@vmware.com>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM, will leave for @sunjayBhatia to take another look

@sunjayBhatia sunjayBhatia merged commit a81e387 into projectcontour:main Dec 16, 2022
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
…ctcontour#4912)

For HTTProute/TLSRoute, only if it references the relevant Gateway, it
will trigger DAG rebuild.

Signed-off-by: Fang Peng <fpeng@vmware.com>
Signed-off-by: yy <yang.yang@daocloud.io>
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
…ctcontour#4912)

For HTTProute/TLSRoute, only if it references the relevant Gateway, it
will trigger DAG rebuild.

Signed-off-by: Fang Peng <fpeng@vmware.com>
Signed-off-by: yy <yang.yang@daocloud.io>
vmw-yingy pushed a commit to vmw-yingy/contour that referenced this pull request Feb 28, 2023
…ctcontour#4912)

For HTTProute/TLSRoute, only if it references the relevant Gateway, it
will trigger DAG rebuild.

Signed-off-by: Fang Peng <fpeng@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateway API: don't trigger DAG rebuilds for irrelevant objects
3 participants