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

feat: LB IP missing no longer an error #4066

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mflendrich
Copy link
Contributor

After this commit a LoadBalancer service that has no external IP addresses listed in the status is no longer a reconciliation error - the intended result is:

  • a change from the ugly error below to a more informative message to the user

    [kong-controller-75896f8d9b-4hwbq] time="2023-05-24T11:26:03Z" level=error msg="Reconciler error" error="waiting for addresses to be provisioned for publish service kong/kong-proxy-proxy" logger=controllers.Ingress.netv1 reconcileID=""f97e4634-0d72-4882-bdd9-9a5b2e5339b6""

  • not requeueing reconciliation of the ingress in the event of the LB service not being ready (yet)

We're changing the semtantics from "KIC failed to reconcile" to "KIC reconciled this publish service okay, it just happens to have zero public IP addresses".

Why are we making this change? To allow for a more user-friendly message than this:

After this commit a LoadBalancer service that has no external IP
addresses listed in the status is no longer a reconciliation error.

We're changing the semtantics from "KIC failed to reconcile" to
"KIC reconciled this publish service okay, it just happens to have
zero public IP addresses".

Why are we making this change? To allow for a more user-friendly
message than this:

    [kong-controller-75896f8d9b-4hwbq] time="2023-05-24T11:26:03Z" level=error msg="Reconciler error" error="waiting for addresses to be provisioned for publish service kong/kong-proxy-proxy" logger=controllers.Ingress.netv1 reconcileID="\"f97e4634-0d72-4882-bdd9-9a5b2e5339b6\""
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch coverage: 62.5% and no project coverage change.

Comparison is base (57e9e08) 59.9% compared to head (0712b6e) 60.0%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #4066   +/-   ##
=====================================
  Coverage   59.9%   60.0%           
=====================================
  Files        149     149           
  Lines      16491   16491           
=====================================
+ Hits        9884    9895   +11     
+ Misses      5975    5965   -10     
+ Partials     632     631    -1     
Impacted Files Coverage Δ
internal/manager/setup.go 40.2% <62.5%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmalek
Copy link
Member

pmalek commented Jun 27, 2023

@mflendrich do we want to merge this?

LoadBalancer service that has no external IP addresses listed in the status

Where did we encounter that? I can see the benefit of this change but I'm curious about where did this come up.

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

Successfully merging this pull request may close these issues.

2 participants