Add GCS support and tests #1105

mszacillo · 2020-09-27T15:24:31Z

What this PR does / why we need it:
This PR adds GCS Protocol support for downloading models.

Overview:
I've added GCS as an accepted protocol to provider.go, along with the necessary model download logic using gcs which resides in gcs.go. For unit testing, I needed to find a way to mock the gcs client. The best resource I could find was googleapis/google-cloud-go-testing, but when I tried adding this library as a dependency, there was a conflict that prevented the project from pulling it. Because of this, I added in some interface classes, which I use to adapt the gcs client to a general type, which can be used for mocking. Test coverage for the gcs class is currently at 75%, but this can be increased with a couple more tests.

Note: There were conflicts when importing google.cloud.com/go/storage alongside the storage package within agent. Due to this, I renamed the storage class to kfstorage (but I'm happy to alter this name to something else more fitting if needed). Also noticed that there are a bunch of conflicts with #1055 - so I can resolve those after that PR merges. :)

Testing

For testing, I deployed the puller pod manually and checked the logs to make sure that the models were being successfully downloaded from my GCS bucket. I did this using the following yaml:

apiVersion: v1
kind: Pod
metadata:
  name: puller
spec:
  serviceAccountName: default
  restartPolicy: Never
  containers:
  - name: job
    image: mszacillo/korepo:gcssupporttest
    imagePullPolicy: Always
    resources:
      limits:
        cpu: "1"
        memory: 1Gi
      requests:
        cpu: "1"
        memory: 1Gi
    command:
    - /agent
    args:
    - -config-dir
    - /mnt/configs
    volumeMounts:
    - name: config-volume
      mountPath: /mnt/configs
    - name: models-volume
      mountPath: /mnt/models
    - name: gcs-serviceaccount-volume
      mountPath: /gcssecret
  volumes:
    - name: config-volume
      configMap:
        name: models-config
    - name: models-volume
      emptyDir: {}
    - name: gcs-serviceaccount-volume
      configMap:
        name: gcs-config
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: models-config
data:
  models.json: |
    [
      {
        "modelName": "model1",
        "modelSpec": {
          "storageUri": "gs://mskfservingmodels/iris.py",
          "framework": "sklearn",
          "memory": "1G"
        }
      }
    ]
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: gcs-config
data:
  gcloud-application-credentials.json: |
     { *** credential info removed *** }

After the puller pod comes up, I can see the following logs print which show that the download was successful, however, I will also run a full e2e test with the kfserving setup. Below you can see a failure when trying to connect to localhost:8080, this is because I wasn't proxyporting the ingress gateway.

{"level":"info","ts":1606770594.7795548,"logger":"modelAgent","msg":"Initializing model agent with","config-dir":"/mnt/configs","model-dir":"/mnt/models"}
{"level":"info","ts":1606770594.7796307,"logger":"modelAgent","msg":"Setting credential file for GCS."}
{"level":"info","ts":1606770594.7796636,"logger":"modelAgent","msg":"Initializing gcs client, using existing GOOGLE_APPLICATION_CREDENTIALS variable."}
{"level":"info","ts":1606770595.8202243,"logger":"Syncer","msg":"Syncing model directory..","modelDir":"/mnt/models"}
&{mskfservingmodels [{project-owners-810000861461  OWNER   0xc00052d9c0} {project-editors-810000861461  OWNER   0xc00052da00} {project-viewers-810000861461  READER   0xc00052da20}] {false 0001-01-01 00:00:00 +0000 UTC} {false 0001-01-01 00:00:00 +0000 UTC} [{project-owners-810000861461  OWNER   0xc00052da40} {project-editors-810000861461  OWNER   0xc00052da80} {project-viewers-810000861461  READER   0xc00052daa0}] false   US 1 STANDARD 2020-11-30 18:28:08.213 +0000 UTC false map[] false {[]} <nil> [] <nil> <nil> <nil> CAE= multi-region}
{"level":"info","ts":1606770595.8207524,"logger":"modelWatcher","msg":"adding model","modelName":"model1"}
{"level":"info","ts":1606770595.8211873,"logger":"Watcher","msg":"Start to watch model config event"}
{"level":"info","ts":1606770595.8213172,"logger":"Watcher","msg":"Watching","modelConfig":"/mnt/data/models.json"}
{"level":"info","ts":1606770595.8212771,"logger":"modelProcessor","msg":"worker is started for","model":"model1"}
{"level":"info","ts":1606770595.8214014,"logger":"modelProcessor","msg":"Downloading model","storageUri":"gs://mskfservingmodels/iris.py"}
{"level":"info","ts":1606770595.8214495,"logger":"Downloader","msg":"Downloading to model dir","modelUri":"gs://mskfservingmodels/iris.py","modelDir":"/mnt/models"}
{"level":"info","ts":1606770595.8214915,"logger":"Downloader","msg":"Success file does not exist"}
{"level":"info","ts":1606770595.82158,"logger":"modelAgent","msg":"Downloading model ","modelName":"model1","storageUri":"gs://mskfservingmodels/iris.py","modelDir":"/mnt/models"}
{"level":"info","ts":1606770596.0174067,"logger":"modelAgent","msg":"Getting file /mnt/models/model1/iris.py"}
{"level":"info","ts":1606770596.2042253,"logger":"modelWatcher","msg":"Downloaded model."}
{"level":"error","ts":1606770596.213174,"logger":"modelProcessor","msg":"Failed to Load model","modelName":"model1","error":"Post http://localhost:8080/v2/repository/models/model1/load: dial tcp [::1]:8080: connect: connection refused","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/pkg/mod/github.com/go-logr/zapr@v0.2.0/zapr.go:132\ngithub.com/kubeflow/kfserving/pkg/agent.(*Puller).modelProcessor\n\t/go/src/github.com/kubeflow/kfserving/pkg/agent/puller.go:143"}
{"level":"info","ts":1606770596.2133465,"logger":"modelOnComplete","msg":"completion event for model","modelName":"model1","inFlight":0}

Release note:

Adding in GCS as an accepted storage provider

kubeflow-bot · 2020-09-27T15:24:36Z

This change is

k8s-ci-robot · 2020-09-27T15:24:42Z

Hi @mszacillo. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

google-cla · 2020-09-27T15:24:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-27T15:28:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-09-27T15:28:32Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mszacillo · 2020-09-27T15:31:44Z

@googlebot I fixed it.

google-cla · 2020-09-27T15:31:51Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

ifilonenko · 2020-09-27T15:47:02Z

/ok-to-test

yuzisun · 2020-09-29T00:43:07Z

@mszacillo This is great! Can you help rebase the master and sign the Google CLA?

mszacillo · 2020-09-29T13:50:55Z

@yuzisun Yes! I can take care of rebasing the branch - for the CLA I'm just waiting to hear back for approval from legal. Should be able to get that done in the next couple days.

ifilonenko · 2020-09-30T17:49:12Z

has this been tested outside of unit tests? i.e. an e2e example of a pod with the appropriate labels pulling down from gcs? Can the results of that test be put in the PR description please

mszacillo · 2020-10-01T14:23:33Z

Good point, I can do some manual verifications and post the results on the PR description.

pkg/agent/test/mockapi/storageinterface.go

yuzisun · 2020-11-08T00:31:25Z

pkg/apis/serving/v1beta1/trained_model.go

@@ -65,3 +65,31 @@ type ModelSpec struct {
 	// +optional
 	Memory resource.Quantity `json:"memory,omitempty"`
 }
+
+type TrainedModelYaml struct {


Why we do need the yaml version?

yuzisun · 2020-11-08T00:35:04Z

cmd/agent/main.go

-			Downloader: s3manager.NewDownloaderWithClient(sessionClient, func(d *s3manager.Downloader) {
-			}),
+		downloader.Providers[kfstorage.GCS] = &kfstorage.GCSProvider{
+			Client: mockapi.AdaptClient(client),


here should use the real client?

The real client is defined by storage.NewClient(ctx) on line 54. This line instead adapts the client to fit the mock client interface which is used to allow unit testing. At first I had set the client type for the GCS provider as storage.Client, but this was causing type errors when I went and mocked the client for unit tests - so I used a common interface between the mocked version and the real client.

I might be missing a better way to do this, so I'll look into it further.

yuzisun · 2020-11-08T00:39:23Z

pkg/agent/downloader.go

@@ -77,14 +80,41 @@ func (d *Downloader) download(modelName string, storageUri string) error {
 	return nil
 }

+func (d* Downloader) GetProvider() (string, error) {
+	storageUri := ""
+	matches, _ := filepath.Glob(d.ModelDir + "/*.yaml")


not sure if I understand this, the storage uri should be passed on ModelSpec and does not need to be parsed from the yaml.

My understanding was that we'd configure the storage provider before any of the models are pulled, however I think I was over complicating things. We can just add each supported provider to the Provider map and then upon an add event, the correct provider will be parsed from the ModelSpec like you said. I'll go ahead and remove this logic.

+1 to removing this logic

ifilonenko

a few nits but thanks for writing up a much need storage protocol!

ifilonenko · 2020-11-10T00:56:23Z

cmd/agent/main.go

+		panic(err)
+	}
+
+	if strings.Contains(storageUri, string(kfstorage.S3)) {


This is not the appropriate place to do this. This main method is setting up watcher and initializing all the available providers (by checking os.LookupEnv() to see if the agent is configured properly. in essence, this just means you need to do:

if endpoint, ok := os.LookupEnv(bcscredential.GCSEndpointUrl); ok {

or whatever to setup the gcs client, if needed. The logic of whether or not the download should happen from gcs or s3 is configured elsewhere.

ifilonenko · 2020-11-10T00:56:58Z

pkg/agent/downloader.go

@@ -77,14 +80,41 @@ func (d *Downloader) download(modelName string, storageUri string) error {
 	return nil
 }

+func (d* Downloader) GetProvider() (string, error) {
+	storageUri := ""
+	matches, _ := filepath.Glob(d.ModelDir + "/*.yaml")


+1 to removing this logic

ifilonenko · 2020-11-10T00:57:41Z

pkg/agent/kfstorage/gcs.go

+)
+
+type GCSProvider struct {
+	Client mockapi.Client


mockapi? this should be the physical client

ifilonenko · 2020-11-10T01:03:16Z

pkg/agent/kfstorage/gcs.go

+		)
+	}
+	defer rc.Close()
+	data, err := ioutil.ReadAll(rc)


Is there a way to send this directly to disk? i.e. zero copy transfer or some more efficient way to write to disk directly? or is there BatchDownloader that takes a FileWriter? and what is rc here?

ifilonenko · 2020-11-10T01:03:57Z

pkg/apis/serving/v1beta1/trained_model.go

@@ -65,3 +65,31 @@ type ModelSpec struct {
 	// +optional
 	Memory resource.Quantity `json:"memory,omitempty"`
 }
+
+type TrainedModelYaml struct {


cmd/agent/Dockerfile

cmd/agent/main.go

yuzisun · 2020-11-24T17:37:38Z

pkg/agent/watcher_test.go

@@ -332,4 +297,39 @@ var _ = Describe("Watcher", func() {
 			})
 		})
 	})
+
+	Describe("Use GCS Downloader", func() {


Nice test! could you add a test for failing to download ?

yuzisun · 2020-12-04T03:36:34Z

@mszacillo This looks great! Can you help resolve the conflicts and we should be good to merge!

yuzliu · 2020-12-08T00:58:20Z

@mszacillo I think there must be a bug in the gcs downloader. I tested using gs://kfserving-samples/models/sklearn/iris The logs shows that it downloaded the model but when I exec into the pod the model was not downloaded.

{"level":"info","ts":1607388819.7210934,"logger":"modelProcessor","msg":"Downloading model","storageUri":"gs://kfserving-samples/models/sklearn/iris"}
{"level":"info","ts":1607388819.7213714,"logger":"Downloader","msg":"Downloading to model dir","modelUri":"gs://kfserving-samples/models/sklearn/iris","modelDir":"/mnt/models"}
{"level":"info","ts":1607388819.7258606,"logger":"modelOnComplete","msg":"completion event for model","modelName":"model3","inFlight":0}

Could you please try

kubectl exec -it <pod_name> -- /bin/bash
cd /mnt/models

And verify that models are downloaded from gcs?

mszacillo · 2020-12-08T19:09:17Z

Could you please try
kubectl exec -it <pod_name> -- /bin/bash
cd /mnt/models
And verify that models are downloaded from gcs?

@yuzliu I added some debug logs to double check that the file is getting downloaded successfully - seems like the download works, the file gets written to successfully, and that the file exists after the download:

{"level":"info","ts":1607454256.6356146,"logger":"modelAgent","msg":"Downloading model ","modelName":"model1","storageUri":"gs://mskfservingmodels/iris.py","modelDir":"/mnt/models"}
{"level":"info","ts":1607454257.6121614,"logger":"modelAgent","msg":"Object iris.py exists."}
{"level":"info","ts":1607454257.7909303,"logger":"modelAgent","msg":"Created filename /mnt/models/model1/iris.py"}
{"level":"info","ts":1607454258.070437,"logger":"modelAgent","msg":"Wrote to file /mnt/models/model1/iris.py"}
{"level":"info","ts":1607454258.0704966,"logger":"modelAgent","msg":"File /mnt/models/model1/iris.py exists."}
{"level":"info","ts":1607454258.070524,"logger":"modelWatcher","msg":"Downloaded model."}

This is however with only the puller pod deployed by itself. Next I'll try creating a trainedmodel.

mszacillo · 2020-12-13T19:08:55Z

pkg/agent/storage/gcs.go

+func (g *GCSObjectDownloader) Download(client stiface.Client, it stiface.ObjectIterator) error {
+	var errs []error
+	// flag to help determine if query prefix returned an empty iterator
+	var foundObject = false


I added in a flag to determine if the returned iterator is empty (meaning that the storage uri prefix used to search for objects returned nothing). If the flag is still false after iteration, that means no objects were found and we should log a warning for the user. This was done since the object iterator does not have a way to check size. That said I'm not entirely happy with this solution - any suggestions?

yuzisun · 2021-01-14T14:38:31Z

/retest

ifilonenko · 2021-01-14T18:54:53Z

/retest

ifilonenko · 2021-01-14T21:34:53Z

/retest

aws-kf-ci-bot · 2021-01-14T21:34:57Z

@ifilonenko: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

/test kubeflow-kfserving-presubmit

Use /test all to run all jobs.

In response to this:

/retest this PR also might need to be rebased

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/test-infra repository.

ifilonenko · 2021-01-14T21:36:51Z

/retest

Adding in testing Removing mockapi and using google-cloud-go-testing instead, removing unnecessary methods, cleaning up code Changing back dockerfile name Rebasing on master Fixing import statement Reverting kfstorage rename to storage, changing gcs import Fixing import statement Changing import in test Combining tests into watcher_test, putting mocks into a testutils package Removing unnecessary suite run, renaming testutils to mocks Adding more test cases, accounting for lack of model name in passed in storageURI Changing iterator retrieval logic Rebasing and cleaning code

…ing test

yuzisun · 2021-01-15T21:39:52Z

/retest

yuzisun · 2021-01-15T22:47:20Z

@mszacillo Thanks for your awesome contribution!

/lgtm
/approve

k8s-ci-robot · 2021-01-15T22:47:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mszacillo, yuzisun

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~~ [yuzisun]

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

* Adding in GCS support Adding in testing Removing mockapi and using google-cloud-go-testing instead, removing unnecessary methods, cleaning up code Changing back dockerfile name Rebasing on master Fixing import statement Reverting kfstorage rename to storage, changing gcs import Fixing import statement Changing import in test Combining tests into watcher_test, putting mocks into a testutils package Removing unnecessary suite run, renaming testutils to mocks Adding more test cases, accounting for lack of model name in passed in storageURI Changing iterator retrieval logic Rebasing and cleaning code * Returning a warning if queried object doesn't exist in bucket, resolving test * Rebasing, removing unused import, refactoring * Adding missing parameter to NewWatcher call

k8s-ci-robot requested review from Tomcli and yuzliu September 27, 2020 15:24

k8s-ci-robot added the needs-ok-to-test label Sep 27, 2020

k8s-ci-robot added the size/XL label Sep 27, 2020

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Sep 27, 2020

mszacillo force-pushed the gcs-support branch from 3bd97ca to 68022fa Compare September 30, 2020 00:18

mszacillo force-pushed the gcs-support branch from cf177b1 to ddc2ffb Compare October 13, 2020 18:50

k8s-ci-robot added size/XXL and removed size/XL labels Nov 6, 2020

yuzisun reviewed Nov 8, 2020

View reviewed changes

pkg/agent/test/mockapi/storageinterface.go Outdated Show resolved Hide resolved

yuzisun reviewed Nov 8, 2020

View reviewed changes

ifilonenko suggested changes Nov 10, 2020

View reviewed changes

k8s-ci-robot added size/XL and removed size/XXL labels Nov 10, 2020

yuzliu reviewed Nov 10, 2020

View reviewed changes

cmd/agent/Dockerfile Outdated Show resolved Hide resolved

yuzliu reviewed Nov 10, 2020

View reviewed changes

cmd/agent/main.go Outdated Show resolved Hide resolved

mszacillo force-pushed the gcs-support branch from bdaa00d to fdfa9d7 Compare November 10, 2020 04:21

yuzisun reviewed Nov 24, 2020

View reviewed changes

k8s-ci-robot added size/XXL and removed size/XL labels Dec 1, 2020

mszacillo force-pushed the gcs-support branch from d8427f4 to 4ffadbb Compare December 10, 2020 15:56

k8s-ci-robot added size/XL and removed size/XXL labels Dec 10, 2020

mszacillo commented Dec 13, 2020

View reviewed changes

mszacillo force-pushed the gcs-support branch 2 times, most recently from d42b53e to 4183643 Compare January 14, 2021 05:08

michael.szacillo added 4 commits January 15, 2021 09:13

Returning a warning if queried object doesn't exist in bucket, resolv…

8360dcd

…ing test

Rebasing, removing unused import, refactoring

6cbe8a4

Adding missing parameter to NewWatcher call

085c459

mszacillo force-pushed the gcs-support branch from 4183643 to 085c459 Compare January 15, 2021 16:15

k8s-ci-robot assigned yuzisun Jan 15, 2021

k8s-ci-robot added the lgtm label Jan 15, 2021

k8s-ci-robot added the approved label Jan 15, 2021

k8s-ci-robot merged commit b8b3584 into kserve:master Jan 15, 2021

Add GCS support and tests #1105

Add GCS support and tests #1105

Conversation

mszacillo commented Sep 27, 2020 • edited

kubeflow-bot commented Sep 27, 2020

k8s-ci-robot commented Sep 27, 2020

google-cla bot commented Sep 27, 2020

google-cla bot commented Sep 27, 2020

google-cla bot commented Sep 27, 2020

mszacillo commented Sep 27, 2020

google-cla bot commented Sep 27, 2020

ifilonenko commented Sep 27, 2020

yuzisun commented Sep 29, 2020

mszacillo commented Sep 29, 2020

ifilonenko commented Sep 30, 2020

mszacillo commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mszacillo Nov 9, 2020 • edited

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

ifilonenko left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzisun commented Dec 4, 2020

yuzliu commented Dec 8, 2020 • edited

mszacillo commented Dec 8, 2020

Choose a reason for hiding this comment

yuzisun commented Jan 14, 2021

ifilonenko commented Jan 14, 2021

ifilonenko commented Jan 14, 2021 • edited

aws-kf-ci-bot commented Jan 14, 2021

ifilonenko commented Jan 14, 2021

yuzisun commented Jan 15, 2021

yuzisun commented Jan 15, 2021

k8s-ci-robot commented Jan 15, 2021

mszacillo commented Sep 27, 2020 •

edited

mszacillo Nov 9, 2020 •

edited

yuzliu commented Dec 8, 2020 •

edited

ifilonenko commented Jan 14, 2021 •

edited