Get rid of Linux specific scripts as much as possible

[johnSchnake]

What this PR does / why we need it:
If we are to support Windows nodes, we want to avoid using bash scripts
so that we don't have to duplicate the logic and translate it into a
powershell script.

Changes include:

• remove the run_master.sh script and instead rely on the Dockerfile
  to have the proper aggregator invocation. This way, regardless of the
  underlying OS, the image knows the right command to use.
• remove the script for running the worker in single-node mode. This
  only served to run Sonobuoy and then sleep for some amount of time to
  avoid restarting the container. Instead, a flag was added to the
  single-node command and golang handles the sleep functionality now. By
  default, it sleeps 0 seconds, consistent with the existing logic.
• Slight modifications to command structure so that the subcommands
  can use the cobra RunE method and logging can be done at the top level
  only. This helps avoid using os.Exit which hinders testability.

Which issue(s) this PR fixes
xref #732

Special notes for your reviewer:
We need to pause before merging this to make sure we recognize the compatibility issues this may cause (if any).

The CLI will not be specifying the command in the YAML anymore so:

• new CLI targeting an old Sonobuoy version will not specify it in the YAML so it will rely on the Dockerfile which will call the script. Since it is an old image, it will have the script and things should be fine (assuming that script is OK). The aggregator will generate the YAML for the workers so its logic should be consistent.
• old CLI with new Sonobuoy version will just have the command specified in the YAML so things should be fine for the aggregator. The new aggregator will the create the workers appropriately.

Am I missing a compatibility case of concern?

Release note:

N/A

[johnSchnake]

3600 is hardcoded but it is currently hardcoded in the script; no loss of flexibility. Though a constant would probably be a good idea.

[johnSchnake]

This command will be something that needs to work across OSs

In my POC this works fine on Windows nodes; the / doesnt bother things and it finds the right executable.

We can't rely on the Dockerfile since it defaults to running the aggregator and we wont know which OS to target unless we query and pass that around (and it would be very annoying to do so at first consideration)

[zubron]

Yay for getting rid of those scripts and making the plugin drivers consistent 🎉

[zubron]

I approved this, but want to try out/check some things locally. I think the code changes look good though. I'm don't think there are any compatibility issues but I will do some testing then re-approve.

[zubron]

Uh, I feel like this is another day when I'm forgetting about the order of when things are created.

You mentioned that:

The CLI will not be specifying the command in the YAML

but I don't think I understand what you mean by that. Which command are you referring to? If you mean the call to the bash script, I don't see any uses of the script in the YAML (assuming you mean the output from gen). We're already using the /sonobuoy command directly in aggregator container, and in the sidecar container for the job driver. Changing the daemonset driver to do that same thing seems fine to me.

[zubron]

I'm getting myself mixed up here 🙃

We want to rely on the Dockerfile to have the correct invocation so that it doesn't matter whether we're targetting Linux or Windows because the image will have the correct command. Although we were previously calling /run_master.sh in the image, we override it anyway in the manifest produced by gen - did you want to remove that?

[johnSchnake]

Although we were previously calling /run_master.sh in the image, we override it anyway in the manifest produced by gen - did you want to remove that?

Yes; so previously the docker image CMD was calling the run_master.sh script but that script also had some odd code in it too. So now I removed the run_master.sh script and updated the Dockerfile CMD to just invoke sonobuoy directly.

So when running the aggregator we don't need to specify a command at all; the default value will be correct.

When running as a sidecar to a job we were already setting the command to be `sonobuoy worker..; no change needed there.

When running as a sidecar to a daemonset we were running a bash script (so it wasnt portable to Windows). Since that script was just run Sonobuoy then sleep I just added the sleep functionality into the command.

Does that clarify the situations in which things are set or not and which things got modified in this PR?

[johnSchnake]

Which command are you referring to? If you mean the call to the bash script, I don't see any uses of the script in the YAML (assuming you mean the output from gen)

If you look at the master branch and run sonobuoy gen you explicitly set the command value for the aggregator pod:

apiVersion: v1
kind: Pod
metadata:
  labels:
    component: sonobuoy
    run: sonobuoy-master
    tier: analysis
  name: sonobuoy
  namespace: sonobuoy
spec:
  containers:
  - command:
    - /bin/bash
    - -c
    - /sonobuoy master --no-exit=true -v 3 --logtostderr
...

So I removed that part of the yaml and rely on the default from the image (the Dockerfile CMD value)

[zubron]

Does that clarify the situations in which things are set or not and which things got modified in this PR?

Yep, I'm totally on board with those changes - that all makes sense.

So I removed that part of the yaml and rely on the default from the image (the Dockerfile CMD value)

I ran this locally, and they are pretty much the same as there are no changes to manifest.go in this PR. Is that missing? I think that's what is confusing me here.

[johnSchnake]

Apparently, it is missing!

I didn't manually run gen again once I put up this PR and was mixing it up with my original, tinkering PR here: https://github.com/vmware-tanzu/sonobuoy/pull/1071/files#diff-ba4a4ef0b81ee6b7942cecf10da19388L124-L128

I'll add that change too and get tests green again. Thanks for the catch.

[johnSchnake]

So the one compatibility "issue" is just that a custom build of the master branch wont work properly since:

• it uses the sonobuoy:v0.17.1 image
• that docker image calls run_master.sh as its default CMD
• that script calls sonobuoy to run as an aggregator, but does not specify --no-exit so as soon as it gets results, it exits.

You just have to target a custom image or override the cmd in the gen output.

Once we release v0.17.2 this won't be an issue though.

[johnSchnake]

Clearly forgot to update the manifest.go since all those unit tests were green; now that I updated the template the "golden file" tests were incorrect. Updated those and rerunning.

[zubron]

So the one compatibility "issue" is just that a custom build of the master branch wont work properly

My hope is that this is something that should only affect those working on the project which might be a bit annoying in the meantime but we can try to target a release for later this week and get this change out.

[zubron]

You also mentioned this in the PR description:

new CLI targeting an old Sonobuoy version will not specify it in the YAML so it will rely on the Dockerfile which will call the script. Since it is an old image, it will have the script and things should be fine (assuming that script is OK). The aggregator will generate the YAML for the workers so its logic should be consistent.

Given that the script doesn't specify --no-exit, it won't work as intended so this combination will be broken for the users. I don't know how common it is to target an old image using a new CLI though. I can understand the case of using an existing manifest which uses an old image but that case will be fine as the command for the aggregator container is in that manifest.