Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Oct 25, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions deploy/crds/pravega.pravega.io_pravegaclusters_crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2210,6 +2210,21 @@ spec:
that a PersistentVolumeClaim called "pravega-longterm" is present
and it will use it as Tier 2
properties:
custom:
description: Custom Storage as a Tier2 backend
properties:
env:
additionalProperties:
type: string
type: object
options:
additionalProperties:
type: string
type: object
required:
- env
- options

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

type: object
ecs:
description: Ecs is used to configure a Dell EMC ECS system
as a Tier 2 backend
Expand Down
10 changes: 9 additions & 1 deletion pkg/apis/pravega/v1beta1/pravega.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,9 @@ type LongTermStorageSpec struct {

// Hdfs is used to configure an HDFS system as a Tier 2 backend
Hdfs *HDFSSpec `json:"hdfs,omitempty"`

// Custom Storage as a Tier2 backend
Custom *CustomSpec `json:"custom,omitempty"`
}

// AuthImpemenationSpec helps to inject plugins to contoller pod
Expand All @@ -471,7 +474,7 @@ type AuthHandlerSpec struct {
}

func (s *LongTermStorageSpec) withDefaults() (changed bool) {
if s.FileSystem == nil && s.Ecs == nil && s.Hdfs == nil {
if s.FileSystem == nil && s.Ecs == nil && s.Hdfs == nil && s.Custom == nil {
changed = true
fs := &FileSystemSpec{
PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{
Expand Down Expand Up @@ -511,3 +514,8 @@ type HDFSSpec struct {
// +optional
ReplicationFactor int32 `json:"replicationFactor"`
}

type CustomSpec struct {
Options map[string]string `json:"options"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Env map[string]string `json:"env"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
35 changes: 35 additions & 0 deletions pkg/apis/pravega/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions pkg/apis/pravega/v1beta1/zz_generated_deepcopy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,19 @@ var _ = Describe("PravegaCluster DeepCopy", func() {
},
}
p2.Spec.Pravega.LongTermStorage.Hdfs = p1.Spec.Pravega.LongTermStorage.Hdfs.DeepCopy()

p2.Spec.Pravega.LongTermStorage.Custom = p1.Spec.Pravega.LongTermStorage.Custom.DeepCopy()
p1.Spec.Pravega.LongTermStorage = &v1beta1.LongTermStorageSpec{
Custom: &v1beta1.CustomSpec{
Options: map[string]string{
"key": "dummy-value",
},
Env: map[string]string{
"AWS_KEY": "key",
},
},
}
p2.Spec.Pravega.LongTermStorage.Custom = p1.Spec.Pravega.LongTermStorage.Custom.DeepCopy()
})
It("value of str1 and str2 should be equal", func() {
Ω(str2).To(Equal(str1))
Expand Down
14 changes: 13 additions & 1 deletion pkg/controller/pravega/pravega_segmentstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for name, value := range p.Spec.Pravega.LongTermStorage.Custom.Options {
javaOpts = append(javaOpts, fmt.Sprintf("-D%v=%v", name, value))
}
}

sort.Strings(javaOpts)

authEnabledStr := fmt.Sprint(p.Spec.Authentication.IsEnabled())
Expand All @@ -309,7 +315,6 @@ func MakeSegmentstoreConfigMap(p *api.PravegaCluster) *corev1.ConfigMap {
for k, v := range getTier2StorageOptions(p.Spec.Pravega) {
configData[k] = v
}

return &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
Kind: "ConfigMap",
Expand Down Expand Up @@ -362,6 +367,13 @@ func getTier2StorageOptions(pravegaSpec *api.PravegaSpec) map[string]string {
}
}

if pravegaSpec.LongTermStorage.Custom != nil {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we remove the extra line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

return pravegaSpec.LongTermStorage.Custom.Env
}
}

return make(map[string]string)
}

Expand Down
170 changes: 170 additions & 0 deletions pkg/controller/pravega/pravega_segmentstore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,176 @@ var _ = Describe("PravegaSegmentstore", func() {
})
})

Context("SegmentStore", func() {

It("should create a headless service", func() {
_ = pravega.MakeSegmentStoreHeadlessService(p)
Ω(err).Should(BeNil())
})

It("should create a pod disruption budget", func() {
_ = pravega.MakeSegmentstorePodDisruptionBudget(p)
Ω(err).Should(BeNil())
})

It("should create a config-map and set the JVM options given by user", func() {
cm := pravega.MakeSegmentstoreConfigMap(p)
javaOpts := cm.Data["JAVA_OPTS"]
Ω(strings.Contains(javaOpts, "-Dpravegaservice.clusterName=default")).Should(BeTrue())
Ω(strings.Contains(javaOpts, "-XX:MaxDirectMemorySize=1g")).Should(BeTrue())
Ω(strings.Contains(javaOpts, "-XX:MaxRAMPercentage=50.0")).Should(BeTrue())
Ω(strings.Contains(javaOpts, "-Dpravegaservice.service.listener.port=12345")).Should(BeTrue())
Ω(err).Should(BeNil())
})
It("should create a stateful set", func() {
sts := pravega.MakeSegmentStoreStatefulSet(p)
mounthostpath0 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath
Ω(mounthostpath0).Should(Equal("/tmp/dumpfile/heap"))
mounthostpath1 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[1].MountPath
Ω(mounthostpath1).Should(Equal("/opt/pravega/logs"))
mounthostpath2 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[2].MountPath
Ω(mounthostpath2).Should(Equal("/tmp/dumpfile/heap"))
mounthostpath3 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[3].MountPath
Ω(mounthostpath3).Should(Equal("/opt/pravega/logs"))
mounthostpath4 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[4].MountPath
Ω(mounthostpath4).Should(Equal("/opt/pravega/conf/logback.xml"))
Ω(err).Should(BeNil())
})
It("should set external access service type to LoadBalancer", func() {
_ = pravega.MakeSegmentStoreExternalServices(p)
Ω(err).Should(BeNil())
})
})
Context("Create External service with external service type and access type empty", func() {
BeforeEach(func() {
p.Spec.Pravega.SegmentStoreExternalServiceType = ""
p.Spec.ExternalAccess.Type = ""
p.Spec.ExternalAccess.DomainName = "example"

})
It("should create external service with access type loadbalancer", func() {
svc := pravega.MakeSegmentStoreExternalServices(p)
Ω(svc[0].Spec.Type).To(Equal(corev1.ServiceTypeLoadBalancer))
Ω(err).Should(BeNil())
})
})
Context("Create External service with SegmentStoreExternalTrafficPolicy as cluster", func() {
BeforeEach(func() {
p.Spec.Pravega.SegmentStoreExternalTrafficPolicy = "cluster"
})
It("should create external service with ExternalTrafficPolicy type cluster", func() {
svc := pravega.MakeSegmentStoreExternalServices(p)
Ω(svc[0].Spec.ExternalTrafficPolicy).To(Equal(corev1.ServiceExternalTrafficPolicyTypeCluster))
})
})
Context("Create External service with LoadBalancerIP", func() {
BeforeEach(func() {
p.Spec.Pravega.SegmentStoreLoadBalancerIP = "10.240.12.18"
})
It("should create external service with LoadBalancerIP", func() {
svc := pravega.MakeSegmentStoreExternalServices(p)
Ω(svc[0].Spec.LoadBalancerIP).To(Equal("10.240.12.18"))
})
})
Context("Create External service with external service type empty", func() {
BeforeEach(func() {
m := make(map[string]string)
p.Spec.Pravega.SegmentStoreServiceAnnotations = m
p.Spec.Pravega.SegmentStoreExternalServiceType = ""
})
It("should create the service with external access type ClusterIP", func() {
svc := pravega.MakeSegmentStoreExternalServices(p)
Ω(svc[0].Spec.Type).To(Equal(corev1.ServiceTypeClusterIP))
Ω(err).Should(BeNil())
})
})
})
Context("With CustomStorage as Tier2", func() {
var (
customReq *corev1.ResourceRequirements
err error
)
BeforeEach(func() {
annotationsMap := map[string]string{
"service.beta.kubernetes.io/aws-load-balancer-type": "nlb",
}
customReq = &corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("2"),
corev1.ResourceMemory: resource.MustParse("4Gi"),
},
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("4"),
corev1.ResourceMemory: resource.MustParse("6Gi"),
},
}
p.Spec = v1beta1.ClusterSpec{
Version: "0.5.0",
ExternalAccess: &v1beta1.ExternalAccess{
Enabled: true,
Type: corev1.ServiceTypeClusterIP,
DomainName: "pravega.com.",
},
BookkeeperUri: v1beta1.DefaultBookkeeperUri,
Pravega: &v1beta1.PravegaSpec{
ControllerReplicas: 2,
SegmentStoreReplicas: 4,
ControllerServiceAccountName: "pravega-components",
SegmentStoreServiceAccountName: "pravega-components",
ControllerResources: customReq,
SegmentStoreResources: customReq,
ControllerServiceAnnotations: annotationsMap,
ControllerPodLabels: annotationsMap,
SegmentStoreServiceAnnotations: annotationsMap,
SegmentStorePodLabels: annotationsMap,
SegmentStoreExternalServiceType: corev1.ServiceTypeLoadBalancer,
SegmentStoreSecret: &v1beta1.SegmentStoreSecret{
Secret: "seg-secret",
MountPath: "/tmp/mount",
},
Image: &v1beta1.ImageSpec{
Repository: "bar/pravega",
},
ControllerJvmOptions: []string{"-XX:MaxDirectMemorySize=1g", "-XX:MaxRAMPercentage=50.0"},
SegmentStoreJVMOptions: []string{"-XX:MaxDirectMemorySize=1g", "-XX:MaxRAMPercentage=50.0"},
Options: map[string]string{
"dummy-key": "dummy-value",
"configMapVolumeMounts": "prvg-logback:logback.xml=/opt/pravega/conf/logback.xml",
"emptyDirVolumeMounts": "heap-dump=/tmp/dumpfile/heap,log=/opt/pravega/logs",
"hostPathVolumeMounts": "heap-dump=/tmp/dumpfile/heap,log=/opt/pravega/logs",
},
LongTermStorage: &v1beta1.LongTermStorageSpec{
Custom: &v1beta1.CustomSpec{
Options: map[string]string{
"key": "dummy-value",
},
Env: map[string]string{
"AWS_KEY": "key",
},
},
},
},
TLS: &v1beta1.TLSPolicy{
Static: &v1beta1.StaticTLS{
ControllerSecret: "controller-secret",
SegmentStoreSecret: "segmentstore-secret",
CaBundle: "ecs-cert",
},
},
Authentication: &v1beta1.AuthenticationParameters{
Enabled: true,
PasswordAuthSecret: "authentication-secret",
},
}
p.WithDefaults()
})

Context("First reconcile", func() {
It("shouldn't error", func() {
Ω(err).Should(BeNil())
})
})

Context("SegmentStore", func() {

It("should create a headless service", func() {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/resources/kubernetes_master_install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,5 @@ sudo chown $(id -u):$(id -g) $HOME/.kube/config
kubectl get nodes
kubectl apply -f https://docs.projectcalico.org/v3.14/manifests/calico.yaml
kubectl get nodes
sudo apt-get install binutils bison gcc -y
sudo apt-get install binutils bison gcc make -y
mkdir /export