sharing artifacts between pipeline tasks using bucket #444

nader-ziada · 2019-01-28T19:59:23Z

Fixes #384

Proposed changes

Using gcp buckets only for now
Bucket information configured using a config map
Refactor of pvc implementation for the same feature to use same interface
Artifact bucket and artifact pvc return the container spec to execute the upload and download steps
e2e test will only run if there is an environment variable KANIKO_SECRET_CONFIG_FILE with the path to a service json file that has enough access to create a bucket

Still to be implemented

The recommendation is to use a retention policy to get the files deleted after a fixed small number of hours which is more efficient then deleting a folder with a large number of files

shashwathi · 2019-01-28T20:57:09Z

/test pull-knative-build-pipeline-build-tests

nader-ziada · 2019-01-28T21:43:40Z

/test pull-knative-build-pipeline-unit-tests

nader-ziada · 2019-01-28T21:46:53Z

/test pull-knative-build-pipeline-integration-tests

nader-ziada · 2019-01-28T23:21:20Z

/test pull-knative-build-pipeline-integration-tests

nader-ziada · 2019-01-29T02:00:56Z

/retest

docs/developers/README.md

docs/using.md

pkg/apis/pipeline/v1alpha1/artifact_bucket.go

pkg/apis/pipeline/v1alpha1/artifact_pvc.go

pkg/artifacts/artifacts_storage.go

shashwathi

Awesome work @pivotal-nader-ziada . Great PR.
We did some initial review and left some minor comments. I will take further look at this again tomorrow. 👍

pkg/apis/pipeline/v1alpha1/artifact_bucket.go

pkg/apis/pipeline/v1alpha1/artifact_pvc.go

pkg/apis/pipeline/v1alpha1/taskrun_types.go

pkg/artifacts/artifacts_storage.go

shashwathi · 2019-01-29T21:41:53Z

pkg/artifacts/artifacts_storage.go

+}
+
+// NewArtifactBucketConfigFromConfigMap creates a Bucket from the supplied ConfigMap
+func NewArtifactBucketConfigFromConfigMap(configMap *corev1.ConfigMap) (*v1alpha1.ArtifactBucket, error) {


There is no path that returns error in this function so this function declaration can be updated to

func NewArtifactBucketConfigFromConfigMap(configMap *corev1.ConfigMap) *v1alpha1.ArtifactBucket {

it has to implement interface definition in pkg

pkg/artifacts/artifacts_storage.go

nader-ziada · 2019-01-30T18:01:30Z

@shashwathi @dlorenc comments addressed, Thank you for the review

shashwathi

Just some minor comments in test. Rest looks great. 👍

shashwathi · 2019-01-30T20:51:18Z

pkg/reconciler/v1alpha1/pipelinerun/config/store.go

+}
+
+func NewStore(logger configmap.Logger) *Store {
+	store := &Store{


I think we can return directly without assigning to store variable here.

shashwathi · 2019-01-30T21:43:24Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

+	}
+}
+
+func TestAddStepsToBuild_WithBucketFromConfigMap(t *testing.T) {


In this test we can simplify by only testing build spec instead of build. I don't see build metadata being changed here. Also wantErr is not used in this test so we can remove that too

test updated

shashwathi · 2019-01-30T22:05:55Z

test/README.md

@@ -115,6 +115,8 @@ permissions inside the TaskRun to run the Kaniko e2e test and GCS taskrun test.
  `KANIKO_SECRET_CONFIG_FILE` is used to generate Kubernetes secret to access
  GCS bucket. This e2e test requires valid service account configuration json
  but it does not require any role binding.
+- In Storage artifact bucket, GCP service account JSON key file at path
+  `KANIKO_SECRET_CONFIG_FILE` is used to create/delete a bucket.


I think we need to refactor/reword this paragraph. It might not be in the scope of this PR. We can leave TODO here. We need to break down "GCP serviceaccount JSON key file" . One suggestion would be the following

In the Storage artifact bucket test, the JSON key file for the GCP service account stored at path KANIKO_SECRET_CONFIG_FILE is used to create/delete a GCS bucket.

now that we're using KANIKO_SECRET_CONFIG_FILE for the bucket test as well, would it make sense to rename KANIKO_SECRET_CONFIG_FILE to something more general?

renamed to GCP_SERVICE_ACCOUNT_KEY_PATH and reworded the readme

shashwathi · 2019-01-30T22:14:11Z

test/artifact_bucket_test.go

+		v1alpha1.BucketServiceAccountSecretName: bucketSecretName,
+		v1alpha1.BucketServiceAccountSecretKey:  bucketSecretKey,
+	}
+	c.KubeClient.UpdateConfigMap(systemNamespace, v1alpha1.BucketConfigName, configMapData)


Just a small doubt. Are we updating config map after e2e test execution to its original state? Sorry if I missed this code

adding login to reset config map to original values

knative-prow-robot · 2019-01-30T22:14:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, shashwathi

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~~ [pivotal-nader-ziada,shashwathi]

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

bobcatfish

Looks great!!! Just some minor comments from me and a couple questions:

Just curious, did you notice in your experiments, is it faster to use GCS instead of PVCs?
looks like there's a bit of coverage missing in gcs_resource.go that might be worth adding?

bobcatfish · 2019-01-31T00:37:33Z

docs/using.md

+- bucket.service.account.secret.name: the name of the secret that will contain the credentials for the service account
+  with access to the bucket
+- bucket.service.account.secret.key: the key in the secret with the required service account json
+The bucket is configured with a retention policy of 24 hours after which files will be deleted


could we put the docs on how to configure this ConfigMap with these other docs on how to configure the entrypoint configmap?

might even make sense to put in a separate doc e.g. install.md (could be a starting point for #385 :D)

I can do the install.md in a separate pr :D

kk sounds great @pivotal-nader-ziada :D

bobcatfish · 2019-01-31T00:38:07Z

docs/using.md

+- location: the address of the bucket (for example gs://mybucket)
+- bucket.service.account.secret.name: the name of the secret that will contain the credentials for the service account
+  with access to the bucket
+- bucket.service.account.secret.key: the key in the secret with the required service account json


might be a bit easier to read with `` around the values, e.g.:

bucket.service.account.secret.key: the key in the secret with the required service account json

bobcatfish · 2019-01-31T00:39:23Z

pkg/apis/pipeline/v1alpha1/artifact_bucket.go

+limitations under the License.
+*/
+
+package v1alpha1


what do you think about putting the implementations of the artifacts interface into package artifacts ?

I originally had the code in the artifacts package, but had to move it to be able to use existing variables (such as gsutilImage) which are defined in the api package. I would get a circular dependency. This will probably need a bigger refactor which I didn't want to do here since this PR is big already

kk no worries, hopefully we'll refactor it later :D

bobcatfish · 2019-01-31T00:39:51Z

pkg/artifacts/artifacts_storage.go

+	GetSecretsVolumes() []corev1.Volume
+	GetType() string
+	StorageBasePath(pr *v1alpha1.PipelineRun) string
+}


nice!!!!!! i like the interface :D

bobcatfish · 2019-01-31T00:42:35Z

test/README.md

@@ -115,6 +115,8 @@ permissions inside the TaskRun to run the Kaniko e2e test and GCS taskrun test.
  `KANIKO_SECRET_CONFIG_FILE` is used to generate Kubernetes secret to access
  GCS bucket. This e2e test requires valid service account configuration json
  but it does not require any role binding.
+- In Storage artifact bucket, GCP service account JSON key file at path
+  `KANIKO_SECRET_CONFIG_FILE` is used to create/delete a bucket.


now that we're using KANIKO_SECRET_CONFIG_FILE for the bucket test as well, would it make sense to rename KANIKO_SECRET_CONFIG_FILE to something more general?

bobcatfish · 2019-01-31T00:43:00Z

test/artifact_bucket_test.go

+// TestStorageBucketPipelineRun is an integration test that will verify a pipeline
+// can use a bucket for temporary storage of artifacts shared between tasks
+func TestStorageBucketPipelineRun(t *testing.T) {
+	configFilePath := os.Getenv("KANIKO_SECRET_CONFIG_FILE")


quick question: do we know if KANIKO_SECRET_CONFIG_FILE is defined in our automated tests?

the user that is used to run the e2e tests doesn't have high permissions, so it would be able to execute the gcs and artifact_bucket tests anyways

- using gcp buckets only for now - bucket information configured using a config map - refactor of pvc implementation for the same feature to use same interface - artifact bucket and artifact pvc return the container spec to execute the upload and download steps - issue tektoncd#384

- use getType on artifactStorageInterface instead of isPVC - refactor go files - add unit tests

shashwathi

Thank you for addressing comments. It looks good from my end.

shashwathi · 2019-01-31T14:23:19Z

/assign @bobcatfish

nader-ziada · 2019-01-31T14:48:10Z

@bobcatfish On GKE, the pvc is pretty fast, even faster if its a huge number of files

- addressed comments from pr tektoncd#444 review

knative-metrics-robot · 2019-01-31T14:51:42Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/artifact_bucket.go	Do not exist	83.3%
pkg/apis/pipeline/v1alpha1/artifact_pvc.go	Do not exist	71.4%
pkg/apis/pipeline/v1alpha1/gcs_resource.go	87.0%	82.9%	-4.1
pkg/apis/pipeline/v1alpha1/pipelinerun_types.go	100.0%	90.0%	-10.0
pkg/apis/pipeline/v1alpha1/secret_volume_mount.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha1/taskrun_types.go	53.8%	64.7%	10.9
pkg/artifacts/artifacts_storage.go	Do not exist	87.0%
pkg/reconciler/v1alpha1/pipelinerun/config/store.go	Do not exist	87.5%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	82.6%	84.8%	2.2
pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go	93.3%	92.9%	-0.5
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go	94.8%	93.8%	-1.1

bobcatfish · 2019-01-31T18:31:23Z

test/README.md

@@ -136,7 +139,7 @@ gcloud projects add-iam-policy-binding $PROJECT_ID --member serviceAccount:$EMAI
 # create the JSON key
 gcloud iam service-accounts keys create config.json --iam-account $EMAIL

-export KANIKO_SECRET_CONFIG_FILE="$PWD/config.json"
+export GCP_SERVICE_ACCOUNT_KEY_PATH="$PWD/config.json"


nice change :D

bobcatfish · 2019-01-31T18:31:54Z

whew, awesome work @pivotal-nader-ziada !! Looks great :D

/lgtm
/meow space

knative-prow-robot · 2019-01-31T18:31:55Z

@bobcatfish:

In response to this:

whew, awesome work @pivotal-nader-ziada !! Looks great :D

/lgtm
/meow space

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.

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 28, 2019

knative-prow-robot requested review from aaron-prindle and steuhs January 28, 2019 19:59

nader-ziada force-pushed the bucket branch from 0918ba8 to 7007b48 Compare January 28, 2019 20:01

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jan 28, 2019

nader-ziada force-pushed the bucket branch from b6be295 to 3948aa5 Compare January 29, 2019 00:12

nader-ziada force-pushed the bucket branch 4 times, most recently from 73b06a1 to 0d2385f Compare January 29, 2019 19:08

dlorenc reviewed Jan 29, 2019

View reviewed changes

shashwathi reviewed Jan 29, 2019

View reviewed changes

nader-ziada force-pushed the bucket branch from 0d2385f to cfdc80b Compare January 30, 2019 17:56

nader-ziada force-pushed the bucket branch from cfdc80b to 37bf029 Compare January 30, 2019 20:59

shashwathi approved these changes Jan 30, 2019

View reviewed changes

bobcatfish reviewed Jan 31, 2019

View reviewed changes

nader-ziada added 2 commits January 31, 2019 08:34

refactor code for artifact storage

54713f7

- use getType on artifactStorageInterface instead of isPVC - refactor go files - add unit tests

shashwathi reviewed Jan 31, 2019

View reviewed changes

knative-prow-robot assigned bobcatfish Jan 31, 2019

refactor and enhancements to artifact bucket code

d618177

- addressed comments from pr tektoncd#444 review

nader-ziada force-pushed the bucket branch from 37bf029 to d618177 Compare January 31, 2019 14:49

bobcatfish reviewed Jan 31, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2019

knative-prow-robot merged commit 3cd318b into tektoncd:master Jan 31, 2019

nader-ziada deleted the bucket branch February 13, 2019 14:02

sharing artifacts between pipeline tasks using bucket #444

sharing artifacts between pipeline tasks using bucket #444

Conversation

nader-ziada commented Jan 28, 2019

Proposed changes

Still to be implemented

shashwathi commented Jan 28, 2019

nader-ziada commented Jan 28, 2019

nader-ziada commented Jan 28, 2019

nader-ziada commented Jan 28, 2019

nader-ziada commented Jan 29, 2019

shashwathi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Jan 30, 2019

shashwathi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 30, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada Jan 31, 2019 • edited Loading

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

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

shashwathi left a comment

Choose a reason for hiding this comment

shashwathi commented Jan 31, 2019

nader-ziada commented Jan 31, 2019

knative-metrics-robot commented Jan 31, 2019

Choose a reason for hiding this comment

bobcatfish commented Jan 31, 2019

knative-prow-robot commented Jan 31, 2019

nader-ziada Jan 31, 2019 •

edited

Loading