S3AttachmentStore #3779

chetanmeh · 2018-06-19T08:00:20Z

This PR introduces a S3AttachmentStore which is an AttachmentStore implementation for storing attachments in S3 API compatible object storages

It should be possible to use this with IBM Cloud Object Storage as it supports S3 API.

Description

S3AttachmentStore uses Alpakka S3 Connector for making calls to S3. The object key is constructed in following form

<namespace>/<docId>/<name>

Where

namespace - Entity type in lowercase. For now only whiskentity is used with attachments
- whiskentity
- whiskauth
- whiskactivation
docId - Document entity path
name - attachment name. It would be uuid

It supports following operations

readAttachment - Read is done in a streaming way using GET Object
attach - Stream is uploaded via multipart upload API where upload is done in chunks of 5 MB and then finally stitched together. This avoids buffering the whole stream on disk. So any upload would invoke 2+ remote calls to complete upload
deleteAttachments - This is done via List Objects v2 API to list object under prefix <namespace>/<docId> and then remove them via delete object api
deleteAttachment - This is done via single delete object api

Usage

To enable use of S3AttachmentStore following env variables need to be set

CONFIG_whisk_spi_AttachmentStoreProvider = whisk.core.database.s3.S3AttachmentStoreProvider
CONFIG_whisk_s3_bucket - Bucket name
AWS_ACCESS_KEY_ID - AWS Access Key ID. See Access Keys for details on key id and access key. Also see AWS Java SDK for details on how access keys are looked up
AWS_SECRET_ACCESS_KEY
AWS_REGION - . AWS Region name. See AWS Region Selection for details

See Alpakka S3 for more configuration details. In OpenWhisk setup the config are under whisk.s3.alpakka namespace (see s3-reference.conf for details)

Testing

All attachment related test are consolidated in S3AttachmentStoreBehaviorBase and then there are suites to run it against various options

S3AttachmentStoreMinioTests - Performs testing against a Minio S3 Proxy. These test only need docker to be present.
S3AttachmentStoreAwsTests - Performs testing against S3. It requires following env variables to be configured. These are configured in travis settingsnfor master repo and in my fork. So are executed there.
a. AWS_ACCESS_KEY_ID
b. AWS_SECRET_ACCESS_KEY
c. AWS_REGION

We can also use S3Mock which is built on top of akka stack but then it may later pose problem if our dependency version diverge. So using Minio for now

Dependencies

S3 support pulls in following dependencies

$ ./gradlew :core:controller:dependencies 
     \--- com.lightbend.akka:akka-stream-alpakka-s3_2.11:0.19
          +--- org.scala-lang:scala-library:2.11.12
          +--- com.typesafe.akka:akka-stream_2.11:2.5.12 (*)
          +--- com.typesafe.akka:akka-http_2.11:10.0.13 -> 10.1.1 (*)
          +--- com.typesafe.akka:akka-http-xml_2.11:10.0.13
          |    +--- org.scala-lang:scala-library:2.11.11 -> 2.11.12
          |    +--- com.typesafe.akka:akka-http_2.11:10.0.13 -> 10.1.1 (*)
          |    +--- com.typesafe.akka:akka-stream_2.11:2.4.20 -> 2.5.12 (*)
          |    \--- org.scala-lang.modules:scala-xml_2.11:1.0.6 (*)
          \--- com.amazonaws:aws-java-sdk-core:1.11.295
               +--- software.amazon.ion:ion-java:1.0.2
               \--- joda-time:joda-time:2.8.1

Add s3 support increases the size of controller.tar from 88 MB (92 jars) to 91 MB (97 jars). Following are version change and new jars being pulled in

14a15
> akka-http-xml_2.11-10.0.13.jar
22a24
> akka-stream-alpakka-s3_2.11-0.19.jar
30a33
> aws-java-sdk-core-1.11.295.jar
40a44
> ion-java-1.0.2.jar
52a57
> joda-time-2.8.1.jar
84c89
< scala-xml_2.11-1.0.5.jar
---
> scala-xml_2.11-1.0.6.jar

Below was the original dependency list which was trimmed down to previous one based on the assumption alpakka s3 client uses akk http stack. So commons http and jackson can be excluded. All tests pass with these dependencies removed

     \--- com.lightbend.akka:akka-stream-alpakka-s3_2.11:0.19
          +--- org.scala-lang:scala-library:2.11.12
          +--- com.typesafe.akka:akka-stream_2.11:2.5.12 (*)
          +--- com.typesafe.akka:akka-http_2.11:10.0.13 -> 10.1.1 (*)
          +--- com.typesafe.akka:akka-http-xml_2.11:10.0.13
          |    +--- org.scala-lang:scala-library:2.11.11 -> 2.11.12
          |    +--- com.typesafe.akka:akka-http_2.11:10.0.13 -> 10.1.1 (*)
          |    +--- com.typesafe.akka:akka-stream_2.11:2.4.20 -> 2.5.12 (*)
          |    \--- org.scala-lang.modules:scala-xml_2.11:1.0.6 (*)
          \--- com.amazonaws:aws-java-sdk-core:1.11.295
               +--- commons-logging:commons-logging:1.1.3 -> 1.2
               +--- org.apache.httpcomponents:httpclient:4.5.5 (*)
               +--- software.amazon.ion:ion-java:1.0.2
               +--- com.fasterxml.jackson.core:jackson-databind:2.6.7.1 -> 2.7.7 (*)
               +--- com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.6.7
               |    \--- com.fasterxml.jackson.core:jackson-core:2.6.7 -> 2.7.7
               \--- joda-time:joda-time:2.8.1

TODO

- Add test against actual S3 Bucket
- Fix attachmentScheme name from s3s -> s3 (see Uri.from fails for scheme ending in digit akka/akka-http#2080)

Related issue and scope

I opened an issue to propose and discuss this change (Introduce a AttachmentStore SPI #3450)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

codecov-io · 2018-06-19T08:42:05Z

Codecov Report

Merging #3779 into master will decrease coverage by 4.68%.
The diff coverage is 84.48%.

@@            Coverage Diff            @@
##           master   #3779      +/-   ##
=========================================
- Coverage   75.78%   71.1%   -4.69%     
=========================================
  Files         145     146       +1     
  Lines        6897    6953      +56     
  Branches      418     418              
=========================================
- Hits         5227    4944     -283     
- Misses       1670    2009     +339

Impacted Files	Coverage Δ
...n/scala/whisk/core/database/CouchDbRestStore.scala	`73.23% <0%> (ø)`	⬆️
.../scala/whisk/core/database/AttachmentSupport.scala	`100% <100%> (ø)`	⬆️
.../scala/src/main/scala/whisk/core/WhiskConfig.scala	`92.06% <100%> (+0.06%)`	⬆️
...ala/whisk/core/database/s3/S3AttachmentStore.scala	`85.18% <85.18%> (ø)`
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.08%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-81.82%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`0% <0%> (-58.83%)`	⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala	`92% <0%> (-4%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e96c1bb...5966452. Read the comment docs.

chetanmeh · 2018-06-19T10:11:38Z

Would need some suggestion on where to document steps for configuring S3AttachmentStore

chetanmeh · 2018-06-21T09:47:07Z

This PR is now ready for review

chetanmeh · 2018-06-28T00:29:07Z

Needs a PG

cbickel · 2018-06-28T19:03:32Z

PG of commit aef88cf is 🔵

aws sdk pulls in quite a few dependencies which are not used with akka s3 based usage as it uses its own http client instead of commons http client. So those can be safely excluded

rabbah · 2018-07-26T21:12:46Z

common/scala/src/main/scala/whisk/core/database/s3/S3AttachmentStore.scala

+      failure => s"[ATT_DELETE] '$prefix' internal error, doc: '$docId', failure: '${failure.getMessage}'")
+  }
+
+  override def shutdown(): Unit = {}


nothing to shutdown?

It uses Akka cached Http connection pool which is bound to ActorSystem lifecycle so need not be explicitly closed. It may be possible to close the pool but comments here indicates that it may pose problem so not attempting them

This PR introduces a S3AttachmentStore which is an AttachmentStore implementation for storing attachments in S3 API compatible object storages.

chetanmeh added the wip label Jun 19, 2018

chetanmeh force-pushed the attachment-store-s3 branch from 1b815d5 to f7aec89 Compare June 21, 2018 09:29

chetanmeh mentioned this pull request Jun 21, 2018

Introduce separate Akka dispatchers for CouchDB and Kafka Clients (#2954) #3515

Merged

21 tasks

chetanmeh removed the wip label Jun 21, 2018

chetanmeh force-pushed the attachment-store-s3 branch from 418dcf3 to 1792a88 Compare June 25, 2018 17:42

chetanmeh force-pushed the attachment-store-s3 branch from 187e6a7 to aef88cf Compare June 28, 2018 13:38

chetanmeh force-pushed the attachment-store-s3 branch from aef88cf to 304d0be Compare July 6, 2018 07:48

chetanmeh added 14 commits July 17, 2018 18:42

Initial implementation

aaa7cd7

Remove unused variables

9b231ec

Add Minio based tests

11d676b

add new line

e4727ab

Move config to whisk.db.s3

64443b4

Exclude unused dependencies

e881ac4

aws sdk pulls in quite a few dependencies which are not used with akka s3 based usage as it uses its own http client instead of commons http client. So those can be safely excluded

retry create bucket

e6b8e48

Remove unused DBUtils trait

712bad9

Use version 2 of list api

a7b185e

Consolidate S3 tests in a base trait

30cfe72

Add test against S3

402044c

Adapt Uri construction to workaround akka http bug

1f53bb9

Change config namespace to whisk.s3 from whisk.db.s3

bdfe88d

Use DockerCommand from ActionContainer

5966452

chetanmeh force-pushed the attachment-store-s3 branch from 304d0be to 5966452 Compare July 17, 2018 13:13

markusthoemmes added the artifactstore label Jul 23, 2018

markusthoemmes requested a review from dubee July 23, 2018 10:35

markusthoemmes assigned dubee Jul 23, 2018

markusthoemmes added the review Review for this PR has been requested and yet needs to be done. label Jul 23, 2018

rabbah approved these changes Jul 26, 2018

View reviewed changes

rabbah merged commit 4fea23b into apache:master Jul 27, 2018

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

S3AttachmentStore (apache#3779)

55b21ca

This PR introduces a S3AttachmentStore which is an AttachmentStore implementation for storing attachments in S3 API compatible object storages.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3AttachmentStore #3779

S3AttachmentStore #3779

chetanmeh commented Jun 19, 2018 •

edited

Loading

codecov-io commented Jun 19, 2018 •

edited

Loading

chetanmeh commented Jun 19, 2018

chetanmeh commented Jun 21, 2018

chetanmeh commented Jun 28, 2018

cbickel commented Jun 28, 2018

rabbah Jul 26, 2018

chetanmeh Jul 27, 2018 •

edited

Loading

S3AttachmentStore #3779

S3AttachmentStore #3779

Conversation

chetanmeh commented Jun 19, 2018 • edited Loading

Description

Usage

Testing

Dependencies

TODO

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

codecov-io commented Jun 19, 2018 • edited Loading

Codecov Report

chetanmeh commented Jun 19, 2018

chetanmeh commented Jun 21, 2018

chetanmeh commented Jun 28, 2018

cbickel commented Jun 28, 2018

rabbah Jul 26, 2018

Choose a reason for hiding this comment

chetanmeh Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

chetanmeh commented Jun 19, 2018 •

edited

Loading

codecov-io commented Jun 19, 2018 •

edited

Loading

chetanmeh Jul 27, 2018 •

edited

Loading