Probe envoy proxy before marking Ingress ready #3500

sivanantha321 · 2024-03-06T12:18:17Z

What this PR does / why we need it:
We noticed random 404 erros in CI environment. This is due to the fact that we are marking ingress ready as soon as we created the top level virtual service but the the virtual service configuration is not propagated to the envoy proxies.

This PR probes the queue-proxy to check whether the virtual service is ready to receive traffic or not and marks the ingress condition accordingly.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #604

Type of changes
Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

Test A
Test B
Logs

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Checklist:

Have you added unit/e2e tests that prove your fix is effective or that this feature works?
Has code been commented, particularly in hard-to-understand areas?
Have you made corresponding changes to the documentation?

Release note:

Probe virtualservice before marking Ingress ready

oss-prow-bot · 2024-03-06T12:18:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sivanantha321
Once this PR has been reviewed and has the lgtm label, please assign yuzisun for approval by writing /assign @yuzisun in a comment. For more information see:The Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

terrytangyuan

Can you try run CI tests multiple times to see if it actually fixes the flakiness?

spolti · 2024-04-06T18:28:03Z

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go

+		if isIngressReady(isvc) {
+			// When the ingress has already been marked Ready for this generation,
+			// then it must have been successfully probed. This exception necessary for the case
+			// of global resyncs.


maybe improve the comment here?
if the ingress is ready, it will not be probed, the opposite you mentioned.

maybe just do if ! isIngressReady(isvc) and probe as needed. wouldn't it be better?

All the watched resources are periodically reconciled (by default for every 10 hours) by the controller irrespective of any change in the resource. In those scenarios it is unnecessary to probe the ingress, if it was already marked ready for the current generation (If it is marked true then it must be already successfully probed). I agree we can just use if ! isIngressReady(isvc) condition

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

spolti · 2024-04-22T14:57:34Z

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go

 )

 var (
 	log = logf.Log.WithName("IngressReconciler")
+	// probeTimeout defines the maximum amount of time a request will wait
+	probeTimeout                   = 1 * time.Second
+	Transport    http.RoundTripper = &http.Transport{


Will this configuration work well with plain text and ssl?

spolti · 2024-04-22T14:58:23Z

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go

+	} else {
+		log.V(1).Error(fmt.Errorf("failed to probe ingress"), "Failed to probe ingress", "url", target)
+	}
+	return isReady, nil


maybe move it do line 505?
can save creation of one var.

spolti · 2024-04-22T14:59:07Z

pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/suite_test.go

+	}
+}
+
+func TestMain(m *testing.M) {


maybe add a test for ssl?

spolti · 2024-04-22T15:00:35Z

test/e2e/common/utils.py

@@ -117,8 +116,6 @@ def predict_str(
    logging.info("Sending Header = %s", headers)
    logging.info("Sending url = %s", url)
    logging.info("Sending request data: %s", input_json)
-    # temporary sleep until this is fixed https://github.com/kserve/kserve/issues/604


Does this PR fixes the mentioned issue?

spolti · 2024-04-22T15:01:16Z

test/e2e/predictor/test_lightgbm.py

@@ -149,6 +150,10 @@ def test_lightgbm_v2_runtime_mlserver():
    )
    kserve_client.create(isvc)
    kserve_client.wait_isvc_ready(service_name, namespace=KSERVE_TEST_NAMESPACE)
+    # wait for model ready. Currently, wait_model_ready does not support path based routing
+    time.sleep(5)


is this intentional for was used for test?

oss-prow-bot bot added the do-not-merge/work-in-progress label Mar 6, 2024

oss-prow-bot bot requested review from cmaddalozzo and terrytangyuan March 6, 2024 12:18

sivanantha321 changed the title ~~Probe virtualservice before marking Ingress ready~~ Probe envoy proxy before marking Ingress ready Mar 6, 2024

sivanantha321 force-pushed the add-ingress-prober branch 3 times, most recently from b7a6410 to 7b8cfc3 Compare March 7, 2024 09:04

sivanantha321 marked this pull request as ready for review March 7, 2024 09:53

oss-prow-bot bot removed the do-not-merge/work-in-progress label Mar 7, 2024

oss-prow-bot bot requested review from andyi2it and ckadner March 7, 2024 09:53

terrytangyuan reviewed Mar 23, 2024

View reviewed changes

sivanantha321 force-pushed the add-ingress-prober branch 4 times, most recently from ec7d3e7 to 7471983 Compare April 6, 2024 15:56

spolti reviewed Apr 6, 2024

View reviewed changes

sivanantha321 force-pushed the add-ingress-prober branch from 7471983 to b804ed5 Compare April 9, 2024 06:38

sivanantha321 added 8 commits April 22, 2024 14:26

Add Ingress probe

7ddf8dc

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

Add tests

827a8d2

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

Fix mlserver fails due to model not ready

c5d25b7

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

Bump portforward package

8243d1c

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

Only display running pod logs in CI

fdb87fe

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

rebase master

5f307eb

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

debug

08ad051

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

resolve comment

b02a9c3

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

sivanantha321 force-pushed the add-ingress-prober branch from b804ed5 to 0b3ae85 Compare April 22, 2024 09:08

Make linter happy

cd34c81

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

sivanantha321 force-pushed the add-ingress-prober branch from 0b3ae85 to cd34c81 Compare April 22, 2024 09:28

spolti reviewed Apr 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Probe envoy proxy before marking Ingress ready #3500

Probe envoy proxy before marking Ingress ready #3500

sivanantha321 commented Mar 6, 2024 •

edited

oss-prow-bot bot commented Mar 6, 2024

terrytangyuan left a comment

spolti Apr 6, 2024

sivanantha321 Apr 8, 2024

spolti Apr 22, 2024

spolti Apr 22, 2024

spolti Apr 22, 2024

spolti Apr 22, 2024

spolti Apr 22, 2024

Probe envoy proxy before marking Ingress ready #3500

Are you sure you want to change the base?

Probe envoy proxy before marking Ingress ready #3500

Conversation

sivanantha321 commented Mar 6, 2024 • edited

oss-prow-bot bot commented Mar 6, 2024

terrytangyuan left a comment

Choose a reason for hiding this comment

spolti Apr 6, 2024

Choose a reason for hiding this comment

sivanantha321 Apr 8, 2024

Choose a reason for hiding this comment

spolti Apr 22, 2024

Choose a reason for hiding this comment

spolti Apr 22, 2024

Choose a reason for hiding this comment

spolti Apr 22, 2024

Choose a reason for hiding this comment

spolti Apr 22, 2024

Choose a reason for hiding this comment

spolti Apr 22, 2024

Choose a reason for hiding this comment

sivanantha321 commented Mar 6, 2024 •

edited