Support Deployments and StatefulSet #52

AdheipSingh · 2020-05-30T08:36:15Z

Fixes #49 .

Description

As discussed in the issue Support StatefulSet only For Historicals,MM #49, this PR shall make sure operator supports both deployments as well as statefulsets, giving users more flexibility.

In the NodeSpec have added a key kind which can take inputs for deployments or statefulsets.
This key is a Required param and is validated by the operator.
The makeStatefulSet func has been split up so that the underlying podSpec remains common. Hence less code to maintain, plus in case we add more features to the podSpec it remains common to both deployments and statefulsets.
Status has been updated to update deployments.
Update is same as earlier, incase of rollingDeploy true, we have incremental updates.

This PR has:

[x ] been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
[x ] been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
[x ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added documentation for new or modified features or behaviors.

Key changed/added files in this PR

types.go
handler.go
tiny-cluster.yaml

himanshug · 2020-06-19T00:40:20Z

pkg/apis/druid/v1alpha1/druid_types.go

@@ -141,6 +141,9 @@ type DruidNodeSpec struct {
 	// Required: Port used by Druid Process
 	DruidPort int32 `json:"druid.port"`

+	// Required: Define kind for node. Deployment or Statefulsets are supported
+	Kind string `json:"kind"`


can we make it optional and default to StatefulSet to keep things backward compatible and also to not necessarily having to specify this.

sure, makes sense to have backward compatible. Ill do the changes :)

AdheipSingh · 2020-07-15T17:46:11Z

@himanshug any updates ?

himanshug · 2020-07-16T07:02:11Z

@AdheipSingh sorry, I haven't had a chance to look at it till now, will review it this week for sure. I am hoping to get some more time to spend on druid-operator in future :)

AdheipSingh · 2020-07-17T02:12:22Z

@AdheipSingh sorry, I haven't had a chance to look at it till now, will review it this week for sure. I am hoping to get some more time to spend on druid-operator in future :)

no hurries, just wanted to check :)
Thanks

himanshug

Looks good overall, few small comments. Thanks

himanshug · 2020-07-20T23:46:30Z

pkg/apis/druid/v1alpha1/druid_types.go

+	// Optional: maxSurge for deployment object
+	MaxSurge *int32 `json:"maxSurge"`
+
+	// Optional: MaxUnavailable for deployment object
+	MaxUnavailable *int32 `json:"maxUnavailable"`
+


nit

Suggested change

// Optional: maxSurge for deployment object

MaxSurge *int32 `json:"maxSurge"`

// Optional: MaxUnavailable for deployment object

MaxUnavailable *int32 `json:"maxUnavailable"`

// Optional: maxSurge for deployment object, only applicable if kind=Deployment

MaxSurge *int32 `json:"maxSurge"`

// Optional: MaxUnavailable for deployment object, only applicable if kind=Deployment

MaxUnavailable *int32 `json:"maxUnavailable"`

himanshug · 2020-07-21T16:52:57Z

pkg/controller/druid/handler.go

-			// Check StatefulSet rolling update status, if in-progress then stop here
-			done, err := isStsFullyDeployed(sdk, nodeSpecUniqueStr, m)
-			if !done {
+		if nodeSpec.Kind == "Deployment" {


two "if" clause above should be combined as ...

if nodeSpec.Kind == "Deployment" { // do stuff for Deployment } else { // do stuff for StatefulSet }

himanshug · 2020-07-21T16:56:13Z

pkg/controller/druid/handler.go

+		sendEvent(sdk, drd, v1.EventTypeWarning, "GET_FAIL", e.Error())
+		return false, e
+	} else {
+		if deploy.Status.ReadyReplicas != deploy.Status.Replicas {


have you tested same concept works for Deployment resource? what I wrote for StatefulSet in isStsFullyDeployed(..) was only tested for sts.

yes, i have been running this in my cluster, this is checking if desired replicas have come back in ready state. Behaves same as sts during upgrades.

himanshug · 2020-07-21T18:57:58Z

pkg/controller/druid/handler.go

@@ -560,31 +617,43 @@ func getServiceName(nameTemplate, nodeSpecUniqueStr string) string {
 	}
 }

-func makeStatefulSet(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid, ls map[string]string, nodeSpecUniqueStr, configMapSHA, serviceName string) (*appsv1.StatefulSet, error) {
-	templateHolder := []v1.PersistentVolumeClaim{}
+func templateHolder(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []v1.PersistentVolumeClaim {


nit: like other similar methods, this could be a getXxx

Suggested change

func templateHolder(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []v1.PersistentVolumeClaim {

func getPersistentVolumeClaims(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []v1.PersistentVolumeClaim {

himanshug · 2020-07-21T18:58:20Z

pkg/controller/druid/handler.go

-	volumeMountHolder := []v1.VolumeMount{
+}
+
+func volumeMountHolder(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []v1.VolumeMount {


nit: like other similar methods, this could be a getXxx

Suggested change

func volumeMountHolder(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []v1.VolumeMount {

func getVolumeMounts(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []v1.VolumeMount {

pkg/apis/druid/v1alpha1/druid_types.go

nishantmonu51

LGTM, 👍

nishantmonu51 · 2020-07-30T08:37:00Z

pkg/controller/druid/handler.go

-			done, err := isStsFullyDeployed(sdk, nodeSpecUniqueStr, m)
-			if !done {
+				// Check Deployment rolling update status, if in-progress then stop here
+				done, err := isDeployFullyDeployed(sdk, nodeSpecUniqueStr, m)


nit: rename to isDeploymentFullyDeployed

AdheipSingh and others added 5 commits May 22, 2020 03:11

inital version of deployments,sts

89cb934

deploy updated working

27587ae

support both deployments and sts

98a0bd6

Merge branch 'master' into deployment

36663c4

fix PSC in podSpec

070b9ef

himanshug reviewed Jun 19, 2020

View reviewed changes

AdheipSingh and others added 3 commits June 20, 2020 03:43

Fix: Kind to be optional

8725752

Merge branch 'master' into deployment

bb345d7

identation, remove required from types

25f45d8

AdheipSingh requested a review from himanshug July 5, 2020 10:30

himanshug reviewed Jul 21, 2020

View reviewed changes

code review changes

f68f3af

AdheipSingh requested a review from himanshug July 22, 2020 03:52

This was referenced Jul 26, 2020

KubernetesTaskRunner for running druid tasks as kubernetes jobs apache/druid#8801

Open

readOnly for ConfigMaps #48

Merged

AdheipSingh and others added 3 commits July 30, 2020 12:54

fix CI, remove labels

593287c

Update druid-test-cr.yaml

fc4b452

Merge branch 'master' into deployment

758d889

nishantmonu51 approved these changes Jul 30, 2020

View reviewed changes

Update handler.go

22a0cb6

himanshug merged commit 33f4d38 into druid-io:master Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Deployments and StatefulSet #52

Support Deployments and StatefulSet #52

AdheipSingh commented May 30, 2020

himanshug Jun 19, 2020

AdheipSingh Jun 19, 2020

AdheipSingh commented Jul 15, 2020

himanshug commented Jul 16, 2020

AdheipSingh commented Jul 17, 2020

himanshug left a comment

himanshug Jul 20, 2020

himanshug Jul 21, 2020

himanshug Jul 21, 2020

AdheipSingh Jul 22, 2020 •

edited

Loading

himanshug Jul 21, 2020

himanshug Jul 21, 2020

nishantmonu51 left a comment

nishantmonu51 Jul 30, 2020

AdheipSingh Jul 30, 2020

	func templateHolder(nodeSpec v1alpha1.DruidNodeSpec, m v1alpha1.Druid) []v1.PersistentVolumeClaim {
	func getPersistentVolumeClaims(nodeSpec v1alpha1.DruidNodeSpec, m v1alpha1.Druid) []v1.PersistentVolumeClaim {

	func volumeMountHolder(nodeSpec v1alpha1.DruidNodeSpec, m v1alpha1.Druid) []v1.VolumeMount {
	func getVolumeMounts(nodeSpec v1alpha1.DruidNodeSpec, m v1alpha1.Druid) []v1.VolumeMount {

Support Deployments and StatefulSet #52

Support Deployments and StatefulSet #52

Conversation

AdheipSingh commented May 30, 2020

Description

Key changed/added files in this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdheipSingh commented Jul 15, 2020

himanshug commented Jul 16, 2020

AdheipSingh commented Jul 17, 2020

himanshug 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

AdheipSingh Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishantmonu51 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdheipSingh Jul 22, 2020 •

edited

Loading