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

gcp-marketplace: Updating metadata deployment to use gRPC server #2083

Merged

Conversation

dushyanthsc
Copy link
Contributor

@dushyanthsc dushyanthsc commented Sep 10, 2019

Change to update gcp marketplace helm charts to use gRPC MLMD metadata server.


This change is Reviewable

@neuromage
Copy link
Contributor

/retest

command: ["/bin/metadata_store_server"]
args: ["--grpc_port=8080",
"--mysql_config_host=mysql",
"--mysql_config_database=metadb",
Copy link
Member

Choose a reason for hiding this comment

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

could you revert this change? the db name need to be parameterized

        {{ if .Values.managedstorage.databaseNamePrefix }}
        - --mlmd_db_name={{ .Values.managedstorage.databaseNamePrefix }}_metadata
        {{ else }}
        - --mlmd_db_name={{ .Release.Name | replace "-" "_" | replace "." "_" | trunc 50 }}_metadata
        {{ end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neuromage - This should be fine to use a custom database name correct? I remember you asking if we named the database as "metadb", not sure you asked because there is a dependency on the name somewhere or was it just for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per discussion, updated it.

@IronPan Is the syntax correct? I changed it add quotes, but let me know if its correct or needs modification.

Copy link
Member

Choose a reason for hiding this comment

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

Syntax LGTM
Remove "--mysql_config_database=metadb" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arrg, my bad. Fixed it.

@@ -4,7 +4,7 @@ images:
argoworkflowcontroller: gcr.io/ml-pipeline/workflow-controller:v2.3.0
cloudsqlproxy: gcr.io/cloudsql-docker/gce-proxy:1.14
frontend: gcr.io/ml-pipeline/frontend:0.1.27
metadataserver: gcr.io/kubeflow-images-public/metadata:v0.1.8
metadataserver: gcr.io/tfx-oss-public/ml_metadata_store_server:0.14.0
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dushyanthsc dushyanthsc force-pushed the dushyanthsc/gcp-marketplace-helm branch from 748ced9 to 7b7f584 Compare September 12, 2019 00:25
@dushyanthsc
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@dushyanthsc
Copy link
Contributor Author

/test kubeflow-pipeline-sample-test

command: ["/bin/metadata_store_server"]
args: ["--grpc_port=8080",
"--mysql_config_host=mysql",
"--mysql_config_database=metadb",
Copy link
Member

Choose a reason for hiding this comment

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

Syntax LGTM
Remove "--mysql_config_database=metadb" ?

spec:
containers:
- name: container
image: gcr.io/ml-pipeline/envoy:initial
Copy link
Member

Choose a reason for hiding this comment

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

image for marketplace deployment need to parameterized. please follow other deployments and adjust the code.

Ref: https://github.com/GoogleCloudPlatform/marketplace-k8s-app-tools/blob/master/docs/managed-updates.md#images

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it. Also looks like .release.cloudbuild.yaml also needs to be updated. I have added that as well.

@dushyanthsc dushyanthsc force-pushed the dushyanthsc/gcp-marketplace-helm branch 2 times, most recently from 267d686 to 9ef2877 Compare September 12, 2019 06:11
Change to update gcp marketplace helm charts to use gRPC MLMD metadata
server.
Copy link
Member

@IronPan IronPan left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@IronPan IronPan left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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

@k8s-ci-robot k8s-ci-robot merged commit 22eb915 into kubeflow:master Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants