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

Expose API Server and Envoy endpoints and add a new Envoy condition #669

Merged
merged 5 commits into from
Aug 12, 2024
Merged

Expose API Server and Envoy endpoints and add a new Envoy condition #669

merged 5 commits into from
Aug 12, 2024

Conversation

rimolive
Copy link
Contributor

@rimolive rimolive commented Jul 5, 2024 •
edited
Loading

The issue resolved by this Pull Request:

Resolves RHOAIENG-7929

Description of your changes:

This PR will introduce a new condition to check if MLMD Envoy is ready (EnvoyReady) and add the DSPA endpoints (API Server and MLMD Envoy) to the status field.

Testing instructions

  • Build the operator pod, or use the one that GitHub will create
  • Deploy DSPO:
export IMAGE_NAME="" # Change to the image you built or the one GitHub generated
make deploy IMG=$IMAGE_NAME OPERATOR_NS=opendatahub # Ensure you have opendatahub namespace created and no resources created in it
  • Deploy the dspa_simple.yaml file
  • Run the following command (ensure you ran oc login first):
oc get DataSciencePipelinesApplication sample -o yaml | yq .status.components
  • You should receive an output like this:
apiServer:
  externalUrl: https://ds-pipeline-sample-dspa.apps.example.com
  url: https://ds-pipeline-sample.dspa.svc.cluster.local:8443
mlmdProxy:
  externalUrl: https://ds-pipeline-md-sample-dspa.apps.example.com
  url: http://ds-pipeline-md-sample.dspa.svc.cluster.local:9090
  • Ensure unit tests are passing. Run the following in the repo root path:
make unittests

Checklist

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@dsp-developers
Copy link
Contributor

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669
An OCP cluster where you are logged in as cluster admin is required.

To use this image run the following:

cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/669/head
git checkout -b pullrequest 21e6dfedf811fabf84f8d812edf727e53f528d6b
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-669"

More instructions here on how to deploy and test a Data Science Pipelines Application.

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@rimolive rimolive force-pushed the rmartine branch 2 times, most recently from 21e6dfe to 20c64e0 Compare July 10, 2024 21:25
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

2 similar comments
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@gregsheremeta
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 8, 2024
gmfrasca
gmfrasca reviewed Aug 8, 2024
View reviewed changes
config/internal/ml-metadata/metadata-envoy.deployment.yaml.tmpl Outdated
@@ -118,6 +119,7 @@ spec:
volumeMounts:
- mountPath: /etc/tls/private
name: proxy-tls
{{ end }}
Copy link
Member

@gmfrasca gmfrasca Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(code cleanliness) I think this is one indent off from its matching conditional above (6 vs 8 whitespaces)

gmfrasca
gmfrasca reviewed Aug 8, 2024
View reviewed changes
controllers/dspipeline_controller.go Outdated
return ctrl.Result{}, err
} else {
r.setStatus(ctx, "ds-pipeline-metadata-envoy-"+dspa.Name, config.MLMDProxyReady, dspa,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract "ds-pipeline-metadata-envoy-"+dspa.Name to a variable to avoid Magic Keywords in-code? (assuming the Envoy analog of params.XYZCompoenentName like the other components doesn't exist). Also, add spaces around the concating + for code cleanliness

gmfrasca
gmfrasca reviewed Aug 8, 2024
View reviewed changes
controllers/dspipeline_controller.go Outdated
log.Error(err, "Error retrieving MLMD Proxy Service endpoint")
}

mlmdProxyExternalUrl, err := util.GetRouteHostname(ctx, "ds-pipeline-md-"+dspa.Name, dspa.Namespace, r.Client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above: extract to parameter and/or add whitespace around +, etc

gmfrasca
gmfrasca reviewed Aug 8, 2024
View reviewed changes
controllers/dspipeline_controller.go Outdated
log.Error(err, "Error retrieving MLMD Proxy Route endpoint")
}

apiServerUrl, err := util.GetServiceHostname(ctx, "ds-pipeline-"+dspa.Name, dspa.Namespace, r.Client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above: extract to parameter and/or add whitespace around +, etc

gmfrasca
gmfrasca reviewed Aug 8, 2024
View reviewed changes
controllers/dspipeline_controller.go Outdated
log.Error(err, "Error retrieving API Server Service endpoint")
}

apiServerExternalUrl, err := util.GetRouteHostname(ctx, "ds-pipeline-"+dspa.Name, dspa.Namespace, r.Client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above: extract to parameter and/or add whitespace around +, etc

gmfrasca
gmfrasca reviewed Aug 8, 2024
View reviewed changes
tests/suite_test.go Outdated
@@ -194,6 +197,22 @@ func (suite *IntegrationTestSuite) SetupSuite() {
loggr.Info("Portforwarding service Successfully set up.")
}

func printConditions(ctx context.Context, dspa *v1alpha1.DataSciencePipelinesApplication, namespace string, client client.Client) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this is related to above, but we shouldn't have functional non-test code in *_test.go

@openshift-ci openshift-ci bot removed the lgtm label Aug 8, 2024
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@gregsheremeta
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 9, 2024
@openshift-ci openshift-ci bot removed the lgtm label Aug 12, 2024
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@rimolive
Fix issue with Envoy Deployment when optionally enabling route
6f28be9 
Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-669

@openshift-ci openshift-ci bot added the lgtm label Aug 12, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gmfrasca

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

@openshift-merge-bot openshift-merge-bot bot merged commit 94eb6c6 into opendatahub-io:main Aug 12, 2024
6 checks passed
@gregsheremeta
Copy link
Contributor

/lgtm
/approve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgarciao jgarciao jgarciao left review comments

@diegolovison diegolovison diegolovison left review comments

@gregsheremeta gregsheremeta gregsheremeta requested changes

@amadhusu amadhusu amadhusu left review comments

@DharmitD DharmitD DharmitD left review comments

@gmfrasca gmfrasca gmfrasca approved these changes

@HumairAK HumairAK Awaiting requested review from HumairAK

@VaniHaripriya VaniHaripriya Awaiting requested review from VaniHaripriya

Assignees

@gregsheremeta gregsheremeta

@gmfrasca gmfrasca

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@rimolive @dsp-developers @gregsheremeta @jgarciao @diegolovison @gmfrasca @amadhusu @DharmitD @openshift-merge-robot