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

GatewayAPI: fix LB status address exception #6644

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

izturn
Copy link
Member

@izturn izturn commented Aug 27, 2024

Signed-off-by: Gang Liu [email protected]

When the user explicitly specifies an address, but the load balancer has not yet assigned this specified address to the gateway, the current logic here evaluates to false. After subsequently receiving the updated address status, the related update logic will not be triggered again (as currently only the generation is compared), so this situation is treated as true.

@izturn izturn requested a review from a team as a code owner August 27, 2024 09:48
@izturn izturn requested review from skriss and sunjayBhatia and removed request for a team August 27, 2024 09:48
@izturn izturn self-assigned this Aug 27, 2024
@sunjayBhatia sunjayBhatia requested review from a team, rajatvig and davinci26 and removed request for a team August 27, 2024 09:48
@izturn izturn added kind/bug Categorizes issue or PR as related to a bug. release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. labels Aug 27, 2024
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.78%. Comparing base (808864b) to head (73744fd).
Report is 52 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6644      +/-   ##
==========================================
+ Coverage   81.76%   81.78%   +0.01%     
==========================================
  Files         133      133              
  Lines       15944    15944              
==========================================
+ Hits        13037    13040       +3     
+ Misses       2614     2611       -3     
  Partials      293      293              
Files with missing lines Coverage Δ
internal/dag/gatewayapi_processor.go 94.08% <100.00%> (+0.21%) ⬆️

Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 12, 2024
@izturn izturn removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 12, 2024
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 30, 2024
Copy link

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 20, 2024
@tsaarni
Copy link
Member

tsaarni commented Oct 20, 2024

Hi @izturn,

I'm not fully familiar with this case but could this change result in the condition not being displayed in cases where the status is never updated, even when it should be? If so, should the comparison logic for Gateway also take the status into account, similarly as here, to re-trigger the update?

@tsaarni tsaarni removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 20, 2024
Copy link

github-actions bot commented Nov 4, 2024

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 4, 2024
@izturn izturn removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 4, 2024
@izturn
Copy link
Member Author

izturn commented Nov 4, 2024

Hi @izturn,

I'm not fully familiar with this case but could this change result in the condition not being displayed in cases where the status is never updated, even when it should be? If so, should the comparison logic for Gateway also take the status into account, similarly as here, to re-trigger the update?

Apologize for the delayed response, TBH, my fix only solved the problem we discovered, and we did not conduct a comprehensive evaluation of its impact. Based on your suggestions, I will conduct an evaluation later.
Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants