Add initializer spec

[13013SwagR]

Description

This PR adds the independent management of the initializer pod spec to the K6 object spec.

Motivation and Context

The initializer pod has different requirements than the runner pods.
In this case it doesn't need the Istio sidecar, but I would like to have it for the runner.
fixes #172

How Has This Been Tested?

I only adapted the current unit test to make it pass, wanted to see the reception of PR first. I can make a more complete unit test suite if required.
I ran a manual test in my own k8s setup with Istio.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

[CLAassistant]

All committers have signed the CLA.

[yorugac]

Thanks, @13013SwagR 🙂
Initializer was deliberately set to use the runner's spec because they're both running k6, unlike the starter. I.e. one needs an image with k6 for both initializer and runner while starter doesn't depend on k6, at least at the moment. So initializer was set to use runner's spec in order to minimize entities and keep configurations simple. Normally, a user of the operator shouldn't even bother with initializer, unless there are errors in the script and initializer terminates with an error. I think if we are to change this and put initializer into a separate spec, we should be more clear on why we're doing that.

So one case is with dapr, described in #164, but I'm not fully clear dapr usage really depends on a separate spec for initializer or if it was resolved with container specification fix. I'd like to confirm this first. Sadly, I didn't have time to test this myself yet.

Are there other cases?

PS this discussion is more fit for an issue and not a PR I guess 🙈

[13013SwagR]

After some thinking, I am not sure the custom initializer configuration is required...
It seems to overlap with the Starter Job, but it is unclear to what is the purpose of this pod.
My issue seemed to be pretty much the same as #164, a sidecar not shutting down, the scuttle feature should do the job to fix that, but it currently breaks the whole system by polluting the initializer logs which makes the controller fail to parse the inspect response.

In conclusion, I think the initializer Job should really be just an init container on the Starter Pod which solve my issue and the dapr one for the initializer.

[mcandeia]

So one case is with dapr, described in #164, but I'm not fully clear dapr usage really depends on a separate spec for initializer or if it was resolved with container specification fix. I'd like to confirm this first. Sadly, I didn't have time to test this myself yet.

As I described there, "the problem" here is related on how kubernetes understand that a given job is completed.

Since dapr is a sidecar (i.e one more container within the same pod) and the annotations for the runners and initializers are the same, the sidecar will be present in both, runners and initializers. The big difference is because when using runners I'm able to call the dapr shutdown using its api (see one example at dapr) but as the initializers are totally out of my control I can't call shutdown, meaning that from the kubernetes point of view, the initializer job as a whole still pending even when the initializer container actually was successfully executed (because daprd still running). Furthermore, the operator leverages the kubernetes status to make sure the job is completed, see here.

Conclusion: The operator will not work when the initializer job has sidecar containers because the job isn't considered completed until all the containers are exited with code 0.

[yorugac]

Thank you for the clarifications, @mcandeia!

Furthermore, the operator leverages the kubernetes status to make sure the job is completed, see here.

This part is unlikely to change even with resolution of #138. So yes, some alternative is clearly needed.

@13013SwagR,

I think the initializer Job should really be just an init container on the Starter Pod which solve my issue and the dapr one for the initializer.

This pod is required for the k6 Cloud output support. It can't be merged with the starter: these are clear separate stages that must be executed sequentially.

I've tested this with Envoy / scuttle: there's actually another bug which was also reported recently in #179 - I added the fix in PR #182.
To sum up, enabled Envoy will work with disableLogging: true. Otherwise, initializer output will be polluted with sidecar's logs and cannot be processed. This is "expected behaviour" at the moment. This is true regardless of initializer job being separate or not.

As for request to make initializer job separate, we have three cases so far:

1. dapr sidecar shutdown
2. disabling Istio for initializer only but keeping it for runners
3. downloading the script in init container of initializer (also connected to init container #118)

Given that, I think it makes sense to make initializer job separate at this point of time. More complex logic would be to make initializer job run only for specific cases but that requires some more thinking, esp. given the limitation with the logs (described above with Envoy).

[yorugac]

@13013SwagR, this PR could use a rebase 🙂

Otherwise, the code changes themselves seem to be fine; but the main drawback of this change is that it forces people to specify initializer spec where they didn't have to before. This is true for basically all fields. For instance, if one was passing an imagePullSecrets in the runner spec - now they might need to also pass it to the initializer spec. Ideally, we should re-use runner spec when initializer spec is absent.

I've added a workaround to achieve that: could you please try to incorporate it in your PR? It could use some more testing but by my initial tests, the workaround should work.

[yorugac]

Hello @13013SwagR, This issue with initializer job got a higher internal priority lately. No pressure but could you please let me know if you'll be able to work on this PR in the coming weeks? Thank you!

[13013SwagR]

Hey sorry for the late response, will update in the next days.

[13013SwagR]

Hey @yorugac,
Did the rebase and implemented your suggestions
Let me know if there's anything else
Thanks

[yorugac]

Thank you @13013SwagR, for coming back to this!

It looks OK to me; will be merging in 👍