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

Three-Phase Endpoint Probing #672

Merged
merged 16 commits into from
Apr 17, 2024
Merged

Three-Phase Endpoint Probing #672

merged 16 commits into from
Apr 17, 2024

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Apr 10, 2024 •
edited
Loading

Fixes: #18

I tried to break up the PR into readable commits for easier review.

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 10, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Apr 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2024
@knative-prow knative-prow bot requested review from cardil and skonto April 10, 2024 15:30
@knative-prow knative-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 10, 2024
@dprotaso dprotaso requested review from ReToCode and izabelacg and removed request for cardil, skonto, ReToCode and izabelacg April 10, 2024 15:31
@dprotaso dprotaso changed the title [WIP] Two-Phase Probing [WIP] Three-Phase Probing Apr 10, 2024
@codecov Codecov
Copy link

codecov bot commented Apr 10, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.47368% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 78.11%. Comparing base (cd525ad) to head (451df94).
Report is 19 commits behind head on main.

Files Patch % Lines
pkg/reconciler/ingress/reconcile_resources.go 88.54% 6 Missing and 5 partials ⚠️
pkg/reconciler/ingress/resources/httproute.go 93.51% 4 Missing and 3 partials ⚠️
pkg/reconciler/ingress/ingress.go 82.14% 2 Missing and 3 partials ⚠️
pkg/reconciler/ingress/controller.go 0.00% 2 Missing and 1 partial ⚠️
pkg/reconciler/ingress/lister.go 93.54% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main     #672       +/-   ##
===========================================
+ Coverage   66.97%   78.11%   +11.14%     
===========================================
  Files          15       16        +1     
  Lines         863     1106      +243     
===========================================
+ Hits          578      864      +286     
+ Misses        254      203       -51     
- Partials       31       39        +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dprotaso
Copy link
Contributor Author

I had to introduce another phase in the probing - when transition new backends to the main routing rules I was hoping to drop the probes at the same time. Unfortunately with Contour this still results in a spike with 503s.

Thus there is now a 'third phase' now - after new backend endpoint probes succeed we move them to the main routing rules. Start some new probes - when that succeeds is when we can drop the endpoint probes that we had originally.

@dprotaso dprotaso mentioned this pull request Apr 11, 2024
Merged
@dprotaso dprotaso mentioned this pull request Apr 11, 2024
Merged
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2024
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2024
@dprotaso
Copy link
Contributor Author

/test all

1 similar comment
@dprotaso
Copy link
Contributor Author

/test all

@dprotaso dprotaso changed the title [WIP] Three-Phase Probing Three-Phase Endpoint Probing Apr 16, 2024
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 16, 2024
@dprotaso
Copy link
Contributor Author

/assign @izabelacg @ReToCode @skonto

Hey folks - this is ready for a review - In a follow up PR I'll add some additional test cases.

Given all the conformance tests pass I think this is ready to merge so we can get a nightly into Serving for further testing.

@dprotaso dprotaso mentioned this pull request Apr 16, 2024
Merged
@izabelacg
Copy link
Member

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2024
dprotaso added 16 commits April 17, 2024 16:28
@dprotaso
Use the net-gateway-api's status prober
97df6c1
@dprotaso
add helpers for test fixtures
9429b77
@dprotaso
sort headers based on name
64a079b
@dprotaso
test case - new backend triggers endpoint probes
412ec38
@dprotaso
test case - steady state endpoints are not ready
0cdd957
@dprotaso
test case - transition to new backends
c98bb52
@dprotaso
test case - route probing succeeds - ingress is ready
b6a9b38
@dprotaso
test case - no backend transition when hash is different
d5c3139
@dprotaso
test-case - update endpoint probes when ingress has new backends
3e6aeec
@dprotaso
Update the status prober to work with dynamic paths
ddeca39 
Because probes on the HTTPRoute now have unique paths
we no longer use a vanilla prober from knative/networking.

The changes make the prober less coupled to Ingress and it requires
consumers to build up a list of backends URLs to probe.

The probe target lister now merges backends and URLs with their
corresponding proxy pod IPs and ports. The alternative
would be to look up HTTPRoutes in the informer cache but theres
no guarantee this is up to date.
@dprotaso
re-enable update tests, run codegen
1ed4e1f
@dprotaso
Add a third phase to the probing
1178c41 
We can't just drop the probing rules and move the backends into
the main rules. This still results in 503s with Contour.

Instead we now move the newer backends into the main rules and then
once that 'succeeds' we drop the probes.
@dprotaso
test case - add steady state reconcile while we are transition probing
208eaff
@dprotaso
test case - handle backend updates
05c2fac
@dprotaso
fix boilerplate
7a9f75b
@dprotaso
Add test coverage for resources functions
451df94
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2024
@dprotaso
Copy link
Contributor Author

@izabelacg rebased - you'll have to /lgtm again

@dprotaso
Copy link
Contributor Author

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2024
@izabelacg
Copy link
Member

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024
@knative-prow knative-prow bot merged commit e72ced6 into knative-extensions:main Apr 17, 2024
29 checks passed
@dprotaso dprotaso deleted the endpoint-probing branch April 17, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@skonto skonto

@izabelacg izabelacg

@ReToCode ReToCode

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Need a plan of the KIngress prober for HTTPRoute
5 participants
@dprotaso @izabelacg @skonto @ReToCode @knative-prow-robot