[RHOAIENG-16851] - Rawdeployment bug fixes #462

VedantMahabaleshwarkar · 2025-01-08T10:30:35Z

What this PR does / why we need it:

Fix bug regarding inconsistent schemas in isvc.status.url and isvc.status.address.url for rawdeployment
Remove component URL temporarily (follow up JIRA to add them back correctly : https://issues.redhat.com/browse/RHOAIENG-17765)

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 # https://issues.redhat.com/browse/RHOAIENG-16851

Type of changes
Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
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.

    kserve:
      defaultDeploymentMode: RawDeployment
      devFlags:
        manifests:
          - contextDir: config
            sourcePath: overlays/odh
            uri: 'https://github.com/VedantMahabaleshwarkar/kserve/tarball/devflags'
          - contextDir: config
            sourcePath: ''
            uri: 'https://github.com/VedantMahabaleshwarkar/odh-model-controller/tarball/devflags'
      managementState: Managed
      serving:
        ingressGateway:
          certificate:
            type: OpenshiftDefaultIngress
        managementState: Managed
        name: knative-serving

Install DSC with devflags as given above.
Modify the inferenceservice-config ConfigMap in the ODH application namespace (opendatahub) as follows
-- Add annotation opendatahub.io/managed: false
-- Modify

service: |-
    {
        "serviceClusterIPNone": true
    }

to

service: |-
    {
        "serviceClusterIPNone": false
    }

Resources used to test : https://gist.github.com/VedantMahabaleshwarkar/b84d876cab99d2f1db708757b26f5c63
-- this is in line with previous tests done here
-- results for this test (to address all the yellows in the previous round of testing) : https://gist.github.com/VedantMahabaleshwarkar/fdc6b4bf4226a0ffdf7b76219a2365f1
-- addressing the red (failure to perform inference against unprotected exposed isvc)

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:

Re-running failed tests

/rerun-all - rerun all failed workflows.
/rerun-workflow <workflow name> - rerun a specific failed workflow. Only one workflow name can be specified. Multiple /rerun-workflow commands are allowed per comment.

spolti · 2025-01-08T13:52:35Z

what are these other commits?

VedantMahabaleshwarkar · 2025-01-08T18:22:40Z

@spolti created this PR from the source branch for #459 (i.e https://github.com/spolti/kserve/tree/sync23). The reason for doing that is so that when #459 is merged, this PR will not have to rebase. Also, I needed the commits related to serviceIPNone from the sync PR.
Once the other PR is merged this PR should only show my commits on top

israel-hdez · 2025-01-10T16:36:26Z

@VedantMahabaleshwarkar The sync PR is merged. A rebase is needed, anyway, to remove the foreign commits.

spolti · 2025-01-13T19:20:09Z

What is the issue / PR that this PR is fixing?
Can you please add it in the commit message and in the description?

VedantMahabaleshwarkar · 2025-01-13T19:24:15Z

What is the issue / PR that this PR is fixing?
Can you please add it in the commit message and in the description?

@spolti It is already mentioned in the description, but adding it here again : https://issues.redhat.com/browse/RHOAIENG-16851

spolti · 2025-01-13T19:35:37Z

I meant the commit msg, see, my point is after it is merged, it does not have info about the jira/issue was fixed.

… for rawdeployment Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

VedantMahabaleshwarkar · 2025-01-13T21:20:51Z

@spolti added jira number to all commit messages

…roxy if a transformer-container is present Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

pkg/apis/serving/v1beta1/inference_service_status.go

pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go

spolti · 2025-01-14T17:49:38Z

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

@@ -80,9 +80,15 @@ func createRawURL(client client.Client, isvc *v1beta1.InferenceService, authEnab
 			return nil, err
 		}
 	} else {
+		var scheme string
+		if authEnabled {


not sure about it, why are you enforcing auth only with SSL?

doing this, the code looks cleaner.

var scheme string url = &apis.URL{ Host: getRawServiceHost(isvc, client), Scheme: "http", Scheme: scheme, Path: "", } if authEnabled { url.Scheme = "https" }

Another question, as ssl is being enforced, how the certificates are configured?

certs : https://github.com/opendatahub-io/kserve/blob/master/pkg/controller/v1beta1/inferenceservice/reconcilers/service/service_reconciler.go#L176

All reported ISVC status schemes are according to what is expected in each case according to #419 (comment)

also, code change is included in latest commit

spolti · 2025-01-14T17:53:00Z

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

 	if authEnabled {
 		internalHost += ":" + strconv.Itoa(constants.OauthProxyPort)
+		scheme = "https"
+	} else {
+		scheme = "http"


same here:

scheme = "http" if authEnabled { internalHost += ":" + strconv.Itoa(constants.OauthProxyPort) scheme = "https" }

considering, the logic you wrote is the correct behavior for both cases.

addressed in latest commit

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

VedantMahabaleshwarkar · 2025-01-14T20:12:49Z

/retest

openshift-ci · 2025-01-14T20:49:51Z

@VedantMahabaleshwarkar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-slow	`3fc65e2`	link	true	`/test e2e-slow`
ci/prow/e2e-fast	`3fc65e2`	link	true	`/test e2e-fast`
ci/prow/e2e-raw	`3fc65e2`	link	true	`/test e2e-raw`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

israel-hdez

This code is working great 💯

Suggested follow-up: when using auth, the internal endpoint has the form https://{something}.svc.cluster.local:8443. It will be much nicer/better if the Service would be mapping port 80 -> 8443 to prevent the explicit port in the URL.

/lgtm

spolti

@VedantMahabaleshwarkar can you please create a jira for the follow-up suggested by @israel-hdez ?

openshift-ci · 2025-01-15T16:53:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, spolti, VedantMahabaleshwarkar

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:

~~OWNERS~~ [VedantMahabaleshwarkar,israel-hdez,spolti]

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

* [RHOAIENG-16851] fix scheme bugs in status.url and status.address.url for rawdeployment Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] Remove component url temporarily Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] Use transformer spec to set upstream port in oauth-proxy if a transformer-container is present Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] address feedback Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> --------- Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> (cherry picked from commit 13b5166)

* add oauth-proxy to rawdeployments if odh auth label is present (#419) * add oauth-proxy to rawdeployments if odh auth label is present * remove ingress modifications * bug fix * consume oauth proxy params from configmap * fix oauth proxy sar and minor bugs * revert some unneeded changes * add oauth proxy flag to prevent login page redirect on invalid request * address feedback * update to newer oauth proxy image * minor fix * fix unit test * more feedback * cookie secret * test and other fixes * fix lint issues * address latest feedback * missed import sort * address more feedback * bug fix * fix lint error (cherry picked from commit d987799) Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * introduce service configuration at configmap level (kserve#3672) (cherry picked from commit 23c0396) Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-17229] - Routing and Headless Service Support in KServe Raw Mode Deployment chore: Follow up: remove the hardcoded clsuterIP setting and add the service configuration. Signed-off-by: Spolti <[email protected]> (cherry picked from commit 33b1600) * [RHOAIENG-16851] - Rawdeployment bug fixes (#462) * [RHOAIENG-16851] fix scheme bugs in status.url and status.address.url for rawdeployment Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] Remove component url temporarily Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] Use transformer spec to set upstream port in oauth-proxy if a transformer-container is present Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> * [RHOAIENG-16851] address feedback Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> --------- Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> (cherry picked from commit 13b5166) * go.mod fixes Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> --------- Signed-off-by: Vedant Mahabaleshwarkar <[email protected]> Co-authored-by: Filippe Spolti <[email protected]>

VedantMahabaleshwarkar changed the title ~~J 16851 new~~ Rawdeployment bug fixes Jan 8, 2025

openshift-ci bot added the do-not-merge/work-in-progress label Jan 8, 2025

VedantMahabaleshwarkar requested review from israel-hdez and spolti January 8, 2025 10:54

VedantMahabaleshwarkar force-pushed the j-16851-new branch from 42438c5 to a98d801 Compare January 13, 2025 16:42

openshift-ci bot added the approved label Jan 13, 2025

VedantMahabaleshwarkar marked this pull request as ready for review January 13, 2025 16:42

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 13, 2025

openshift-ci bot requested a review from terrytangyuan January 13, 2025 16:42

VedantMahabaleshwarkar changed the title ~~Rawdeployment bug fixes~~ [RHOAIENG-16851] - Rawdeployment bug fixes Jan 13, 2025

VedantMahabaleshwarkar added 2 commits January 13, 2025 16:20

[RHOAIENG-16851] fix scheme bugs in status.url and status.address.url…

8c95dac

… for rawdeployment Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

[RHOAIENG-16851] Remove component url temporarily

ef6b4d6

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

VedantMahabaleshwarkar force-pushed the j-16851-new branch from a98d801 to ef6b4d6 Compare January 13, 2025 21:20

[RHOAIENG-16851] Use transformer spec to set upstream port in oauth-p…

be3f5f3

…roxy if a transformer-container is present Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

israel-hdez mentioned this pull request Jan 13, 2025

[RHOAIENG-16851] rawdeployment route bug fixes opendatahub-io/odh-model-controller#341

Merged

3 tasks

spolti reviewed Jan 14, 2025

View reviewed changes

pkg/apis/serving/v1beta1/inference_service_status.go Show resolved Hide resolved

spolti reviewed Jan 14, 2025

View reviewed changes

pkg/controller/v1beta1/inferenceservice/reconcilers/deployment/deployment_reconciler.go Show resolved Hide resolved

spolti reviewed Jan 14, 2025

View reviewed changes

[RHOAIENG-16851] address feedback

3fc65e2

Signed-off-by: Vedant Mahabaleshwarkar <[email protected]>

israel-hdez approved these changes Jan 14, 2025

View reviewed changes

openshift-ci bot assigned israel-hdez Jan 14, 2025

openshift-ci bot added the lgtm label Jan 14, 2025

spolti approved these changes Jan 15, 2025

View reviewed changes

VedantMahabaleshwarkar merged commit 13b5166 into opendatahub-io:master Jan 15, 2025
26 of 30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHOAIENG-16851] - Rawdeployment bug fixes #462

[RHOAIENG-16851] - Rawdeployment bug fixes #462

VedantMahabaleshwarkar commented Jan 8, 2025 •

edited

Loading

spolti commented Jan 8, 2025

VedantMahabaleshwarkar commented Jan 8, 2025

israel-hdez commented Jan 10, 2025

spolti commented Jan 13, 2025

VedantMahabaleshwarkar commented Jan 13, 2025

spolti commented Jan 13, 2025

VedantMahabaleshwarkar commented Jan 13, 2025

spolti Jan 14, 2025

spolti Jan 14, 2025

spolti Jan 14, 2025

VedantMahabaleshwarkar Jan 14, 2025

VedantMahabaleshwarkar Jan 14, 2025

spolti Jan 14, 2025

VedantMahabaleshwarkar Jan 14, 2025

VedantMahabaleshwarkar commented Jan 14, 2025

openshift-ci bot commented Jan 14, 2025

israel-hdez left a comment

spolti left a comment

openshift-ci bot commented Jan 15, 2025

[RHOAIENG-16851] - Rawdeployment bug fixes #462

[RHOAIENG-16851] - Rawdeployment bug fixes #462

Conversation

VedantMahabaleshwarkar commented Jan 8, 2025 • edited Loading

spolti commented Jan 8, 2025

VedantMahabaleshwarkar commented Jan 8, 2025

israel-hdez commented Jan 10, 2025

spolti commented Jan 13, 2025

VedantMahabaleshwarkar commented Jan 13, 2025

spolti commented Jan 13, 2025

VedantMahabaleshwarkar commented Jan 13, 2025

spolti Jan 14, 2025

Choose a reason for hiding this comment

spolti Jan 14, 2025

Choose a reason for hiding this comment

spolti Jan 14, 2025

Choose a reason for hiding this comment

VedantMahabaleshwarkar Jan 14, 2025

Choose a reason for hiding this comment

VedantMahabaleshwarkar Jan 14, 2025

Choose a reason for hiding this comment

spolti Jan 14, 2025

Choose a reason for hiding this comment

VedantMahabaleshwarkar Jan 14, 2025

Choose a reason for hiding this comment

VedantMahabaleshwarkar commented Jan 14, 2025

openshift-ci bot commented Jan 14, 2025

israel-hdez left a comment

Choose a reason for hiding this comment

spolti left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 15, 2025

VedantMahabaleshwarkar commented Jan 8, 2025 •

edited

Loading