[WIP] Add command args to container creation

[CodingParsley]

Description

It provides lightweight solution to run some easy startup commands before running druid script
For example, for debugging purpose if we want to check if an environment variable has been set before the druid script was run, we can set

startScript: sh
entryArg: echo ${ENV} >> file

The container will run sh -c "echo ${ENV} >> file && /bin/run-druid.sh {nodetype}

Another example use case is that if we need to source a file before running the druid script, it provides a lightweight solution to do that -
we can set

startScript: sh
entryArg: source ${filename}

And the container will run
sh -c "source ${filename} && /bin/run-druid.sh {nodetype}

If entryArg is not set, the container will act the same as before, thus preserving backward compatibility.
The PR also added variable druidScript to customize druid script path.

---

This PR has:

• been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
• 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.
• 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

• entryArg
• druidScript

[AdheipSingh]

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

[AdheipSingh]

@CodingParsley :)

[CodingParsley]

@AdheipSingh Thank you ❤️

[AdheipSingh]

@AdheipSingh Thank you ❤️

ill run this locally and test, cc @nishantmonu51 for approval to run CI.

just a question, does this PR work if you specify image for all nodeSpecs and also for individual nodeSpec ?

[AdheipSingh]

@xvrl can you approve this PR to run the CI. :)

[CodingParsley]

just a question, does this PR work if you specify image for all nodeSpecs and also for individual nodeSpec ?

Yes, this PR does not change the image of the container

[xvrl]

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.

[CodingParsley]

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

[xvrl]

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.

[CodingParsley]

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.

[xvrl]

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.

[CodingParsley]

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

[CodingParsley]

@xvrl Maybe another look? 🙏

[xvrl]

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

[AdheipSingh]

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

[xvrl]

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

[xvrl]

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

[CodingParsley]

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

[AdheipSingh]

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

[CodingParsley]

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