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

anishakj · 2021-10-20T12:48:22Z

Signed-off-by: anishakj [email protected]

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

Signed-off-by: anishakj <[email protected]>

codecov-commenter · 2021-10-20T12:51:03Z

Codecov Report

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

@@            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%)`	⬆️
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.

Signed-off-by: anishakj <[email protected]>

nishant-yt · 2021-10-21T11:24:41Z

pkg/apis/pravega/v1beta1/pravega.go

@@ -511,3 +514,8 @@ type HDFSSpec struct {
 	// +optional
 	ReplicationFactor int32 `json:"replicationFactor"`
 }
+
+type CustomSpec struct {
+	Options map[string]string `json:"options"`


Suggested change

Options map[string]string `json:"options"`

Options map[string]string `json:"options,omitempty"`

@anishakj @nishant-yt Is there a use case when only one of options,env blocks would have to be supplied?

No, if custom option is mentioned both params needs to be provided

@anishakj looking at pravega_segmentstore.go code, the Options map here *can` be optional. Please, take the change suggested by @nishant-yt here and make it not required on the CRD.

nishant-yt · 2021-10-21T11:25:44Z

pkg/apis/pravega/v1beta1/pravega.go

+
+type CustomSpec struct {
+	Options map[string]string `json:"options"`
+	Env     map[string]string `json:"env"`


Suggested change

Env map[string]string `json:"env"`

Env map[string]string `json:"env,omitempty"`

This spec is actually needed to override the TIER2_STORAGE env variable (though it would be better to have a separate required parameter for exactly this value).

nishant-yt

Requested changes

jkhalack · 2021-10-21T15:58:00Z

@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 · 2021-10-21T16:00:11Z

@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 · 2021-10-21T16:01:14Z

@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 · 2021-10-21T16:53:16Z

@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 · 2021-10-22T12:05:41Z

@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.

jkhalack · 2021-10-22T12:10:00Z

pkg/controller/pravega/pravega_segmentstore.go

@@ -362,6 +367,13 @@ func getTier2StorageOptions(pravegaSpec *api.PravegaSpec) map[string]string {
 		}
 	}

+	if pravegaSpec.LongTermStorage.Custom != nil {


There seems to be an indentation issue within getTier2StorageOptions() function.
if pravegaSpec.LongTermStorage.Custom != nil has a different indentation as compared to the above three if blocks.

jkhalack · 2021-10-22T12:16:09Z

pkg/controller/pravega/pravega_segmentstore.go

@@ -284,6 +284,12 @@ func MakeSegmentstoreConfigMap(p *api.PravegaCluster) *corev1.ConfigMap {
 		javaOpts = append(javaOpts, fmt.Sprintf("-D%v=%v", name, value))
 	}

+	if p.Spec.Pravega.LongTermStorage.Custom != nil {


Hmm... There is nothing saying that the storage options cannot be included by the customer into p.Spec.Pravega.Options (the current practice is to include both controller and segment store java options there, and if needed only in the segment store, the option is ignored by the controller).
So p.Spec.Pravega.LongTermStorage.Custom.Options can be optional.

jkhalack · 2021-10-22T12:18:45Z

pkg/apis/pravega/v1beta1/pravega.go

@@ -511,3 +514,8 @@ type HDFSSpec struct {
 	// +optional
 	ReplicationFactor int32 `json:"replicationFactor"`
 }
+
+type CustomSpec struct {
+	Options map[string]string `json:"options"`


@anishakj looking at pravega_segmentstore.go code, the Options map here *can` be optional. Please, take the change suggested by @nishant-yt here and make it not required on the CRD.

jkhalack · 2021-10-22T12:20:42Z

pkg/apis/pravega/v1beta1/pravega.go

+
+type CustomSpec struct {
+	Options map[string]string `json:"options"`
+	Env     map[string]string `json:"env"`


This spec is actually needed to override the TIER2_STORAGE env variable (though it would be better to have a separate required parameter for exactly this value).

jkhalack · 2021-10-22T12:22:12Z

deploy/crds/pravega.pravega.io_pravegaclusters_crd.yaml

+                            type: object
+                        required:
+                        - env
+                        - options


Please, remove the options block from the list of required parameters.

Signed-off-by: anishakj <[email protected]>

jkhalack

LGTM

SrishT · 2021-10-25T12:33:30Z

doc/longtermstorage.md

@@ -238,3 +238,34 @@ spec:
      root: /example
      replicationFactor: 3
 ```
+
+### Use Custom Storage as LongTermStorage


Could we add the heading of this section to the contents in the beginning of the doc too?

SrishT · 2021-10-25T12:38:08Z

pkg/controller/pravega/pravega_segmentstore.go

@@ -362,6 +367,13 @@ func getTier2StorageOptions(pravegaSpec *api.PravegaSpec) map[string]string {
 		}
 	}

+	if pravegaSpec.LongTermStorage.Custom != nil {
+		if pravegaSpec.LongTermStorage.Custom.Env != nil {
+


nit: Can we remove the extra line here?

Signed-off-by: anishakj <[email protected]>

SrishT

LGTM

nishant-yt

LGTM

Added support for custom storage option in longtermstorage

37776cc

Signed-off-by: anishakj <[email protected]>

Increasing code coverage

968259a

Signed-off-by: anishakj <[email protected]>

anishakj marked this pull request as ready for review October 20, 2021 14:58

anishakj requested review from SrishT, jkhalack and nishant-yt October 20, 2021 14:58

sachin-j-joshi mentioned this pull request Oct 20, 2021

LTS: Enable use of Custom binding in system tests pravega/pravega#6386

Closed

nishant-yt reviewed Oct 21, 2021

View reviewed changes

jkhalack approved these changes Oct 21, 2021

View reviewed changes

jkhalack reviewed Oct 22, 2021

View reviewed changes

jkhalack suggested changes Oct 22, 2021

View reviewed changes

anishakj added 4 commits October 25, 2021 02:00

Addressing review comments

637518c

Signed-off-by: anishakj <[email protected]>

Fixing go fmt error

8ca7a7a

Signed-off-by: anishakj <[email protected]>

modifying test crd

aa86771

Signed-off-by: anishakj <[email protected]>

Installing make

3807f45

Signed-off-by: anishakj <[email protected]>

jkhalack approved these changes Oct 25, 2021

View reviewed changes

SrishT requested changes Oct 25, 2021

View reviewed changes

addressing review comments

437b7fd

Signed-off-by: anishakj <[email protected]>

SrishT approved these changes Oct 25, 2021

View reviewed changes

nishant-yt approved these changes Oct 25, 2021

View reviewed changes

anishakj merged commit f833dc5 into master Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anishakj commented Oct 20, 2021 •

edited

Loading

codecov-commenter commented Oct 20, 2021 •

edited

Loading

nishant-yt Oct 21, 2021

jkhalack Oct 21, 2021

anishakj Oct 21, 2021

jkhalack Oct 22, 2021

anishakj Oct 25, 2021

nishant-yt Oct 21, 2021

jkhalack Oct 22, 2021

nishant-yt left a comment

jkhalack commented Oct 21, 2021

anishakj commented Oct 21, 2021

jkhalack commented Oct 21, 2021

anishakj commented Oct 21, 2021

jkhalack commented Oct 22, 2021

jkhalack Oct 22, 2021

anishakj Oct 25, 2021

jkhalack Oct 22, 2021

jkhalack Oct 22, 2021

jkhalack Oct 22, 2021

jkhalack Oct 22, 2021

anishakj Oct 25, 2021

jkhalack left a comment

SrishT Oct 25, 2021

anishakj Oct 25, 2021

SrishT Oct 25, 2021

anishakj Oct 25, 2021

SrishT left a comment

nishant-yt left a comment

	Options map[string]string `json:"options"`
	Options map[string]string `json:"options,omitempty"`

	Env map[string]string `json:"env"`
	Env map[string]string `json:"env,omitempty"`

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

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

Conversation

anishakj commented Oct 20, 2021 • edited Loading

Change log description

Purpose of the change

What the code does

How to verify it

codecov-commenter commented Oct 20, 2021 • edited Loading

Codecov Report

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

nishant-yt left a comment

Choose a reason for hiding this comment

jkhalack commented Oct 21, 2021

anishakj commented Oct 21, 2021

jkhalack commented Oct 21, 2021

anishakj commented Oct 21, 2021

jkhalack commented Oct 22, 2021

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

jkhalack 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

SrishT left a comment

Choose a reason for hiding this comment

nishant-yt left a comment

Choose a reason for hiding this comment

anishakj commented Oct 20, 2021 •

edited

Loading

codecov-commenter commented Oct 20, 2021 •

edited

Loading