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.

Sign up for GitHub

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

Jump to bottom

[WIP] Add command args to container creation #268

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[WIP] Add command args to container creation #268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3070bca
Add command args to container creation (#6)
CodingParsley Jan 7, 2022
8f28155
add test
CodingParsley Oct 31, 2022
322751f
Update controllers/druid/testdata/druid-test-cr-start-command.yaml
CodingParsley Nov 2, 2022
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
9 changes: 8 additions & 1 deletion apis/druid/v1alpha1/druid_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,16 @@ type DruidSpec struct {
// +optional
DeleteOrphanPvc bool `json:"deleteOrphanPvc"`

// Required: path to druid start script to be run on container start
// Required: Command to be run on container start
StartScript string `json:"startScript"`

// Optional: bash/sh entry arg. Set startScript to `sh` or `bash` to customize entryArg
// For example, the container can run `sh -c "${EntryArg} && ${DruidScript} {nodeType}"`
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is fully backwards compatible. Since we are now wrapping this in a shell, any signals sent to the process running the container will be sent to the shell and not to the JVM. The druid startup script ensures that the JVM will replace the shell, but in this case I do not think that will happen.

Is there a reason why the changes cannot be done inside of the druid startup script? It might be useful to provide an example use-case to clarify that.

Copy link
Author

Choose a reason for hiding this comment

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

It just gives the user an option to run druid startup script in shell, but the default is still
https://github.com/druid-io/druid-operator/pull/268/files#diff-6b7cea906298d4262b867f6e81f41caa89c2daeb67247bd95169e93ed80a00b4R1146

Copy link
Member

Choose a reason for hiding this comment

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

understood, but having it behave differently is not a good idea, and the nuances of using one or the other are too subtle in my opinion.

Copy link
Author

Choose a reason for hiding this comment

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

Its true the same can be achieved by adapting druid.sh. Adding custom arguments just provides a more lightweight way to update the container startup command than updating druid.sh and building a new image.

Copy link
Member

Choose a reason for hiding this comment

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

understood, but can we include a concrete use-case in the description that would warrant this feature specifically in operator? it would help understand the motivation for this change.

Copy link
Author

@CodingParsley CodingParsley Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@xvrl I added this feature to allow us to source environment variables for our druid clusters

EntryArg string `json:"entryArg,omitempty"`

// Optional: Customized druid shell script path. If not set, the default would be "bin/run-druid.sh"
DruidScript string `json:"druidScript,omitempty"`

Comment on lines +97 to +103
Copy link
Contributor

@AdheipSingh AdheipSingh Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

pls add unit this in unit tests tests.
also you can update the docs and examples folder with this feature.
https://github.com/druid-io/druid-operator/tree/master/controllers/druid/testdata

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@CodingParsley :)

// Required here or at nodeSpec level
Image string `json:"image,omitempty"`

Expand Down
4 changes: 4 additions & 0 deletions chart/templates/crds/druid.apache.org_druids.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3979,6 +3979,10 @@ spec:
type: array
startScript:
type: string
entryArg:
type: string
druidScript:
type: string
startUpProbe:
properties:
exec:
Expand Down
19 changes: 18 additions & 1 deletion controllers/druid/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"regexp"
"sort"
"strings"

autoscalev2beta2 "k8s.io/api/autoscaling/v2beta2"
networkingv1 "k8s.io/api/networking/v1"
Expand Down Expand Up @@ -1138,6 +1139,21 @@ func getVolume(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid, nodeSpecUniq
return volumesHolder
}

func getCommand(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []string {
if m.Spec.StartScript != "" && m.Spec.EntryArg != "" {
return []string{m.Spec.StartScript}
}
return []string{firstNonEmptyStr(m.Spec.StartScript, "bin/run-druid.sh"), nodeSpec.NodeType}
}

func getEntryArg(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid) []string {
if m.Spec.EntryArg != "" {
bashCommands := strings.Join([]string{m.Spec.EntryArg, "&&", firstNonEmptyStr(m.Spec.DruidScript, "bin/run-druid.sh"), nodeSpec.NodeType}, " ")
return []string{"-c", bashCommands}
}
return nil
}

func getEnv(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid, configMapSHA string) []v1.EnvVar {
envHolder := firstNonNilValue(nodeSpec.Env, m.Spec.Env).([]v1.EnvVar)
// enables to do the trick to force redeployment in case of configmap changes.
Expand Down Expand Up @@ -1308,7 +1324,8 @@ func makePodSpec(nodeSpec *v1alpha1.DruidNodeSpec, m *v1alpha1.Druid, nodeSpecUn
v1.Container{
Image: firstNonEmptyStr(nodeSpec.Image, m.Spec.Image),
Name: fmt.Sprintf("%s", nodeSpecUniqueStr),
Command: []string{firstNonEmptyStr(m.Spec.StartScript, "bin/run-druid.sh"), nodeSpec.NodeType},
Command: getCommand(nodeSpec, m),
Args: getEntryArg(nodeSpec, m),
Comment on lines +1327 to +1328
Copy link
Member

Choose a reason for hiding this comment

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

instead of adding this complex custom logic, would it make more sense to let the user explicitly override Command and Args directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

In case of an override for a Command or an Arg how can it be done directly ? Override in the CR ? or some way of templating to override it out

Copy link
Member

@xvrl xvrl Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yes, I meant to let the user override those in the CR, rather than creating our own variables with special meanings

Copy link
Member

Choose a reason for hiding this comment

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

Right now we have startScript, druidScript, and entryArg. The way these get combined is quite confusing, and it's not clear why we need 3.

If I specify druidScript, but neither startScript nor entryArg, then druidScript is ignored and we run the default bin/run-druid.sh so druidScript doesn't seem to control the Druid script in all cases unlike what intuition might suggest

This is quite confusing logic for someone new to this (especially since there is very little documentation).

Copy link
Author

Choose a reason for hiding this comment

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

I tried to do that initially. The problem is that, we have to add the node type after the druid.sh script for each node dynamically, otherewise the user needs to specify separate container commands and arguments for each node.
Therefore, we can not just let them overwrite the commands & args freely

Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite confusing logic for someone new to this (especially since there is very little documentation).

Yeah it can be confusing. I am not a user of this feature/requirement so don't have any comments on the use case.
Regardless i leave it for your team, i personally feel that configuration management is a total abstraction, operator CR should be a minimal viable spec to support what is needed to support overrides, templating etc. :)

Also @CodingParsley how about just adding StartScript to each nodeSpec also, currently is just present on the cluster scope ( its common to all nodesSpecs ).
https://github.com/druid-io/druid-operator/blob/master/apis/druid/v1alpha1/druid_types.go#L95

Copy link
Author

@CodingParsley CodingParsley Nov 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

just adding StartScript to each nodeSpec also, currently is just present on the cluster scope ( its common to all nodesSpecs )

Sounds like a solution! Will rewrite the PR

ImagePullPolicy: v1.PullPolicy(firstNonEmptyStr(string(nodeSpec.ImagePullPolicy), string(m.Spec.ImagePullPolicy))),
Ports: nodeSpec.Ports,
Resources: nodeSpec.Resources,
Expand Down
15 changes: 15 additions & 0 deletions controllers/druid/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,21 @@ func TestMakeStatefulSetForBroker(t *testing.T) {
assertEquals(expected, actual, t)
}

func TestMakeStatefulSetForBrokerWithStartCommands(t *testing.T) {
clusterSpec := readDruidClusterSpecFromFile(t, "testdata/druid-test-cr-start-command.yaml")

nodeSpecUniqueStr := makeNodeSpecificUniqueString(clusterSpec, "brokers")
nodeSpec := clusterSpec.Spec.Nodes["brokers"]

actual, _ := makeStatefulSet(&nodeSpec, clusterSpec, makeLabelsForNodeSpec(&nodeSpec, clusterSpec, clusterSpec.Name, nodeSpecUniqueStr), nodeSpecUniqueStr, "blah", nodeSpecUniqueStr)
addHashToObject(actual)

expected := new(appsv1.StatefulSet)
readAndUnmarshallResource("testdata/broker-statefulset-start-command.yaml", &expected, t)

assertEquals(expected, actual, t)
}

func TestMakeStatefulSetForBrokerWithSidecar(t *testing.T) {
clusterSpec := readSampleDruidClusterSpecWithSidecar(t)

Expand Down
94 changes: 94 additions & 0 deletions controllers/druid/testdata/broker-statefulset-start-command.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
apiVersion: apps/v1
kind: StatefulSet
metadata:
name: druid-druid-test-brokers
namespace: test-namespace
labels:
app: druid
druid_cr: druid-test
nodeSpecUniqueStr: druid-druid-test-brokers
component: broker
annotations:
druidOpResourceHash: jDYG8FQEDYnyd6LHB++phICQiD0=
spec:
podManagementPolicy: Parallel
replicas: 2
selector:
matchLabels:
app: druid
druid_cr: druid-test
nodeSpecUniqueStr: druid-druid-test-brokers
component: broker
serviceName: druid-druid-test-brokers
template:
metadata:
labels:
app: druid
druid_cr: druid-test
nodeSpecUniqueStr: druid-druid-test-brokers
component: broker
annotations:
key1: value1
key2: value2
spec:
tolerations: []
affinity: {}
containers:
- command:
- sh
args:
- -c
- echo 'Hello World' && druid-test.sh broker
image: himanshu01/druid:druid-0.12.0-1
name: druid-druid-test-brokers
env:
- name : configMapSHA
value : blah
ports:
- containerPort: 8083
name: random
readinessProbe:
httpGet:
path: /status
port: 8080
livenessProbe:
httpGet:
path: /status
port: 8080
resources:
limits:
cpu: "4"
memory: 2Gi
requests:
cpu: "4"
memory: 2Gi
volumeMounts:
- mountPath: /druid/conf/druid/_common
readOnly: true
name: common-config-volume
- mountPath: /druid/conf/druid/broker
readOnly: true
name: nodetype-config-volume
- mountPath: /druid/data
readOnly: true
name: data-volume
securityContext:
fsGroup: 107
runAsUser: 106
volumes:
- configMap:
name: druid-test-druid-common-config
name: common-config-volume
- configMap:
name: druid-druid-test-brokers-config
name: nodetype-config-volume
volumeClaimTemplates:
- metadata:
name: data-volume
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 2Gi
storageClassName: gp2
Loading