Fix pipeline cannot run bug when using marketplace managed storage #2341

Bobgy · 2019-10-09T13:53:08Z

Pipeline couldn't run because workflow-controller isn't configured to write output artifacts to the correct gcs bucket.

I also made gcs bucket a directly configurable parameter for marketplace deployment, because:

When users need to backup / delete gcp resources, they should be aware which bucket KFP was using.

Pros:

Clarity on which gcp resources KFP is using

Cons:

One extra field to configure when using marketplace

I think pros outweigh cons. WDYT?

/assign @numerology
/cc @jessiezcc
/cc @rmgogogo
/kind bug

Reminder note: when we bring back managed storage configs for marketplace, we need to add an extra field for typing gcs bucket name.

This change is

…oes not run pipelines successfully

Bobgy · 2019-10-09T13:53:38Z

@numerology Can you help me review?

numerology · 2019-10-09T17:35:52Z

@numerology Can you help me review?

Sure. Can you add the triaging issue for this bug. I am a bit of lack of context here. Is this related with the managed storage issue?

numerology · 2019-10-09T17:44:21Z

Shall we also update schema.yaml to reflect this change in our deploy page? Or you just want to enable such deployment through mpdev?

jessiezcc · 2019-10-09T17:48:14Z

/lgtm

numerology · 2019-10-09T17:48:22Z

/lgtm
Otherwise

Bobgy · 2019-10-10T01:36:54Z

Shall we also update schema.yaml to reflect this change in our deploy page? Or you just want to enable such deployment through mpdev?

Just mpdev for this PR.
I'll do a separate PR for schema.yaml if we actually decide to bring back GCS + CloudSQL options.

Bobgy · 2019-10-10T01:38:12Z

Hi @IronPan @neuromage, can you review/approve?

rmgogogo · 2019-10-10T07:14:36Z

manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/pipeline.yaml

-              value: '{{ printf "%s-%s" .Values.managedstorage.cloudsqlInstanceConnectionName .Values.managedstorage.databaseNamePrefix | replace ":" "-" | trunc 50 }}'
-              {{ else }}
-              value: '{{ printf "%s-%s" .Values.managedstorage.cloudsqlInstanceConnectionName .Release.Name | replace ":" "-" | trunc 50 }}'
-              {{ end }}


I would prefer split the change into two PR. You have two in this PR
(1) fix the issue (argo bucket not respect managed storage)
(2) customize gcsBucketname
For (2), I remember Yang mentioned the reason is that he want to sync the DB name and Object storage name.
Need his LGTM in case (2) introduces other issue we didn't aware.

If Yang is OK with (2), then this PR requires touch schema.yaml to let UI feed in the gcsBucketName.

Thanks! These are valid concerns.

I can also do just (1), but I didn't find a cleaner way than just duplicating the same {{if}} {{else}} in both places and make sure the complex logic for determining bucket name is the same.

It will be very helpful if you know other ways that can let me put the logic of determining bucket name in one place in helm chart, and then reference it in those sub charts that use it.

For (2), I wonder why we want to sync DB name and Object storage name?

for (2), Yang mentioned with me some reason before. One reason I remember is that when user reinstall KFP but based on existing storage, it can save user's efforts to remember the pair (DB name and Storage buckage name) considering DB contains index to buckage. If user didn't well remember the pair or mis-match the pair, the error is pretty hard to be caught and understood by user.

for (1), I think a better solution is that we don't handle the logic inside helm but do it in application layer which is easier to share code logics while keep yaml be simple. It might require some corresponding code changes I didn't look into yet.

thanks for the explanation.

for (2), Yang mentioned with me some reason before. One reason I remember is that when user reinstall KFP but based on existing storage, it can save user's efforts to remember the pair (DB name and Storage buckage name) considering DB contains index to buckage. If user didn't well remember the pair or mis-match the pair, the error is pretty hard to be caught and understood by user.

The arguments sound reasonable. I think for clarity we should think about a step during marketplace install that we introduce to users which db/buckets are created for them.

Bobgy · 2019-10-10T09:39:37Z

/hold
because of @rmgogogo 's concerns

IronPan · 2019-10-10T20:09:01Z

manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/argo.yaml

@@ -93,7 +93,7 @@ data:
    artifactRepository:
    {
        s3: {
-            bucket: mlpipeline,
+            bucket: '{{ if .Values.managedstorage.enabled }}{{ .Values.managedstorage.gcsBucketName }}{{ else }}mlpipeline{{ end }}',


Good catch. Thanks!

IronPan · 2019-10-10T20:16:36Z

manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/pipeline.yaml

@@ -624,11 +624,7 @@ spec:
            # Following environment variables are only needed when using Cloud SQL and GCS.
            {{ if .Values.managedstorage.enabled }}
            - name: OBJECTSTORECONFIG_BUCKETNAME
-              {{ if .Values.managedstorage.databaseNamePrefix }}


I think you already aware, before this change, we don't allow user to specify the gcs bucket name.
The gcs bucket name is auto inferred from the cloud sql instance name. The reason for that is so the cloudsql and GCS data can alway be in sync. When user reinstall, the data can be recovered correctly.

Allowing user to specify gcs bucket name might break that assumption and causing unexpected behavior.

Thanks for the explanation. I agree with the concern. will change to not letting it be configurable.

Thanks. A fast fix is to copy the bucket-gen code here to argo configmap.(although it would lead to code dup. anyway to dedup the code would be sweet)

What do you think about my approach to avoid code dupe?

I'm OK if it works

Bobgy · 2019-10-12T07:49:41Z

I found a way to avoid the code duplication.
Documentation about tpl feature in helm chart: https://helm.sh/docs/developing_charts/#using-the-tpl-function

Tests on my new change

pipelines/manifests/gcp_marketplace$ helm template chart/kubeflow-pipelines/ --set-string managedstorage.enabled=true,managedstorage.cloudsqlInstanceConnectionName=cloudsql-connection-name | grep -C 3 -i bucket
    artifactRepository:
    {
        s3: {
            bucket: 'cloudsql-connection-name-release-name',
            keyPrefix: artifacts,
            endpoint: minio-service.default:9000,
            insecure: true,
--
              value: 'pipeline-runner'
            # Following environment variables are only needed when using Cloud SQL and GCS.
            
            - name: OBJECTSTORECONFIG_BUCKETNAME
              value: 'cloudsql-connection-name-release-name'
            - name: DBCONFIG_DBNAME

pipelines/manifests/gcp_marketplace$ helm template chart/kubeflow-pipelines/ --set-string managedstorage.enabled=true,managedstorage.cloudsqlInstanceConnectionName=cloudsql-connection-name,managedstorage.databaseNamePrefix=db-prefix | grep -C 3 -i bucket
    artifactRepository:
    {
        s3: {
            bucket: 'cloudsql-connection-name-db-prefix',
            keyPrefix: artifacts,
            endpoint: minio-service.default:9000,
            insecure: true,
--
              value: 'pipeline-runner'
            # Following environment variables are only needed when using Cloud SQL and GCS.
            
            - name: OBJECTSTORECONFIG_BUCKETNAME
              value: 'cloudsql-connection-name-db-prefix'
            - name: DBCONFIG_DBNAME

… code duplication

rmgogogo

/lgtm

IronPan · 2019-10-15T17:04:00Z

/hold cancel
/approve

k8s-ci-robot · 2019-10-15T17:04:07Z

[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:

~~OWNERS~~ [IronPan]

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

Bobgy · 2019-10-16T03:05:34Z

/retest

* Generate Inferencegraph apis (kubeflow#2226) Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * Generate python models for inferencegraph (kubeflow#2226) * Add inferencegraph models to python sdk * Update python sdk README Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * Add inferencegraph apis to python sdk (kubeflow#2226) * Add create inferencegraph method * Add delete inferencegraph method Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * Add inferencegraph apis to python sdk (kubeflow#2226) * Add inferencegraph ready method * Add wait for inferencegraph ready method * Add get inferencegraph method Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * Add e2e test for inferencegraph (kubeflow#2226) Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * Lint check fix Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * Fixed test errors Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

Make gcs bucket name configurable + fix marketplace managed storage d…

5cc7823

…oes not run pipelines successfully

k8s-ci-robot assigned numerology Oct 9, 2019

k8s-ci-robot added the kind/bug label Oct 9, 2019

k8s-ci-robot requested review from jessiezcc and rmgogogo October 9, 2019 13:53

k8s-ci-robot added the size/XS label Oct 9, 2019

k8s-ci-robot assigned jessiezcc Oct 9, 2019

k8s-ci-robot added the lgtm label Oct 9, 2019

rmgogogo reviewed Oct 10, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold label Oct 10, 2019

numerology assigned IronPan Oct 10, 2019

IronPan reviewed Oct 10, 2019

View reviewed changes

gcsBucketName is computed from cloudsqlInstanceConnectionName + avoid…

05d470a

… code duplication

k8s-ci-robot added size/S and removed lgtm size/XS labels Oct 12, 2019

Bobgy force-pushed the fix_gcp_managed_storage branch from 19b1d25 to 05d470a Compare October 12, 2019 08:10

rmgogogo reviewed Oct 15, 2019

View reviewed changes

k8s-ci-robot assigned rmgogogo Oct 15, 2019

k8s-ci-robot added the lgtm label Oct 15, 2019

k8s-ci-robot removed the do-not-merge/hold label Oct 15, 2019

k8s-ci-robot added the approved label Oct 15, 2019

k8s-ci-robot merged commit 49a50df into kubeflow:master Oct 16, 2019

Bobgy deleted the fix_gcp_managed_storage branch October 16, 2019 03:49

Bobgy added the area/deployment/marketplace label Feb 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pipeline cannot run bug when using marketplace managed storage #2341

Fix pipeline cannot run bug when using marketplace managed storage #2341

Bobgy commented Oct 9, 2019 •

edited by numerology

Loading

Bobgy commented Oct 9, 2019

numerology commented Oct 9, 2019

numerology commented Oct 9, 2019

jessiezcc commented Oct 9, 2019

numerology commented Oct 9, 2019

Bobgy commented Oct 10, 2019

Bobgy commented Oct 10, 2019

rmgogogo Oct 10, 2019

Bobgy Oct 10, 2019

rmgogogo Oct 10, 2019

Bobgy Oct 10, 2019

Bobgy commented Oct 10, 2019

IronPan Oct 10, 2019

IronPan Oct 10, 2019

Bobgy Oct 11, 2019

IronPan Oct 11, 2019

Bobgy Oct 12, 2019

rmgogogo Oct 15, 2019

Bobgy commented Oct 12, 2019 •

edited

Loading

rmgogogo left a comment

IronPan commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

Bobgy commented Oct 16, 2019

Fix pipeline cannot run bug when using marketplace managed storage #2341

Fix pipeline cannot run bug when using marketplace managed storage #2341

Conversation

Bobgy commented Oct 9, 2019 • edited by numerology Loading

Bobgy commented Oct 9, 2019

numerology commented Oct 9, 2019

numerology commented Oct 9, 2019

jessiezcc commented Oct 9, 2019

numerology commented Oct 9, 2019

Bobgy commented Oct 10, 2019

Bobgy commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy commented Oct 12, 2019 • edited Loading

rmgogogo left a comment

Choose a reason for hiding this comment

IronPan commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

Bobgy commented Oct 16, 2019

Bobgy commented Oct 9, 2019 •

edited by numerology

Loading

Bobgy commented Oct 12, 2019 •

edited

Loading