pravega / pravega-operator

Pravega Kubernetes Operator
Apache License 2.0
41 stars 38 forks source link

Issue 489: Added support for custom storage option in long term storage #585

Closed anishakj closed 2 years ago

anishakj commented 2 years ago

Signed-off-by: anishakj anisha.kj@dell.com

Change log description

Adding a new field for custom storage in longtermstoragespec

Purpose of the change

Fixes #489

What the code does

Currently, operator supports HDFS, ECS and filesystem as longtermstorage options, added support for custom storage so that users can provide required options and env variables based on the storage type selected.

We can also pass env varaibles via segmentStoreEnvVars. But in case,if we do not set longtermstorage type, it can cause conflicts

How to verify it

Example of helm charts with s3 as custom storage

storage:
  longtermStorage:
    type: custom
    custom:
      options:
        pravegaservice.storage.layout: "CHUNKED_STORAGE"
        pravegaservice.storage.impl.name: "S3"
        s3.bucket: bucketname
        s3.prefix: "10-11-1"
        s3.connect.config.uri.override: "false"
        s3.connect.config.uri: uri
        s3.connect.config.access.key: keyname
        s3.connect.config.secret.key: keyvalue
      env:
        TIER2_STORAGE: "S3"
        AWS_ACCESS_KEY_ID: "key"
        AWS_SECRET_ACCESS_KEY: "secret"

Verified that able to install pravega with S3 as custom storage, also ran system tests

codecov-commenter commented 2 years ago

Codecov Report

Merging #585 (437b7fd) into master (7af939a) will increase coverage by 0.17%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
+ Coverage   74.17%   74.35%   +0.17%     
==========================================
  Files          16       16              
  Lines        4229     4258      +29     
==========================================
+ Hits         3137     3166      +29     
  Misses        962      962              
  Partials      130      130              
Impacted Files Coverage Δ
pkg/apis/pravega/v1beta1/pravega.go 98.34% <100.00%> (ø)
pkg/apis/pravega/v1beta1/zz_generated.deepcopy.go 99.47% <100.00%> (+0.03%) :arrow_up:
pkg/controller/pravega/pravega_segmentstore.go 100.00% <100.00%> (ø)

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 7af939a...437b7fd. Read the comment docs.

jkhalack commented 2 years ago

@anishakj Could you add some (commented out) example on how this issue might be used to the helm chart (https://github.com/pravega/pravega-operator/blob/master/charts/pravega/values.yaml#L107), please?

anishakj commented 2 years ago

@anishakj Could you add some (commented out) example on how this issue might be used to the helm chart (https://github.com/pravega/pravega-operator/blob/master/charts/pravega/values.yaml#L107), please?

have to create separate PR for that in Pravega/charts repo. Charts repo here is deprecated

jkhalack commented 2 years ago

@anishakj Could you add some (commented out) example on how this issue might be used to the helm chart (https://github.com/pravega/pravega-operator/blob/master/charts/pravega/values.yaml#L107), please?

have to create separate PR for that in Pravega/charts repo. Charts repo here is deprecated

Would you at least add it to the PR description?

anishakj commented 2 years ago

@anishakj Could you add some (commented out) example on how this issue might be used to the helm chart (https://github.com/pravega/pravega-operator/blob/master/charts/pravega/values.yaml#L107), please?

have to create separate PR for that in Pravega/charts repo. Charts repo here is deprecated

Would you at least add it to the PR description?

Values need to be used in helm chart are added in PR description.

jkhalack commented 2 years ago

@anishakj would you please amend the PR description to elaborate on the use of TIER2_STORAGE env variable and why we cannot use the pre-existing segmentStoreEnvVars field and add the custom options under the common options block on PravegaCluster resource.

Apparently, the existing code would set TIER2_STORAGE to one of the three hard-coded values (FILESYSTEM, EXTENDEDS3, or HDFS) along with adding some extra options and env variables. The primary purpose of this PR (if I understand it correctly) is to not set any of the above three storage option and allow the user to configure something different, for example set TIER2_STORAGE to S3 when deploying on AWS.