grafana / k6-operator

An operator for running distributed k6 tests.
Apache License 2.0
541 stars 147 forks source link

Include system env vars into Initializer job #376

Closed ausias-armesto closed 1 month ago

ausias-armesto commented 4 months ago

As part of my specific deployment, I needed to add the system environment variables into the Initializer Job.

yorugac commented 4 months ago

Hi @ausias-armesto, thanks for the PR. We've had and have issues on this topic: https://github.com/grafana/k6-operator/issues/207 https://github.com/grafana/k6-operator/issues/375

Could you please describe details of your use case over there first?

On the subject of the change itself: ATM, I don't think it's a good idea to hard-code this option to all test runs in k6-operator. I'd suggest to look for a workaround first. If there is no workaround, it'd be good to understand why.

ausias-armesto commented 4 months ago

Hi @ausias-armesto, thanks for the PR. We've had and have issues on this topic: #187 #375

Could you please describe details of your use case over there first?

On the subject of the change itself: ATM, I don't think it's a good idea to hard-code this option to all test runs in k6-operator. I'd suggest to look for a workaround first. If there is no workaround, it'd be good to understand why.

Hi @yorugac ,

Thanks for the reply. I totally understand that the change may have impact on other test runs and I didn't consider that.

My repo is public and you can have a look: https://github.com/hoprnet/hoprd-test/tree/main/k6 I think that the reason was because it was needed to built the options variable which was created dynamically based on input environment variables. So I could use the same script and customise the environment variables named: ENVIRONMENT_NAME, WORKLOAD_NAME, SCENARIO_ITERATIONS, SCENARIO_DURATION, etc to the type of load testing execution that I need at any time. Perhaps there is a better alternative to implement that kind of requirements.

This is the pipeline triggering the tests: https://github.com/hoprnet/hoprnet/blob/master/.github/workflows/load-tests.yaml

ausias-armesto commented 4 months ago

This is definitely related to #375

yorugac commented 4 months ago

In my prev comment: I meant issue https://github.com/grafana/k6-operator/issues/207 and not https://github.com/grafana/k6-operator/issues/187 :facepalm:

Thanks for sharing details @ausias-armesto! AFAIS at the moment, it should be working fine assuming workloads are part of the image :thinking: What is the error you're getting when you try to run this?

ausias-armesto commented 3 months ago

Hi @yorugac,

I've checked again with the current helm chart version 3.5.0 and it works fine without adding the system environment variables. I think that I tested with 3.3.0 and it failed. But it's good that it's not needed anymore.

Thanks.

ausias-armesto commented 2 months ago

Hi @yorugac I've come up with the reason why it was working. I added a default value to the environment variables, and then those values were taken by the initialiser job but the issue still exists. If the k6 script requires some environment variables to build the options then it requires that those environment variables are set with a default value at the code level

This snippet of code would fail.

const TEST_TYPE= __ENV.TEST_TYPE
var options
if(TEST_TYPE ="TEST_TYPE1") {
 options = ....
} else {
 options = ....
}
export options

But if the env variable has a default value like in the following snippet, then it works

const TEST_TYPE= __ENV.TEST_TYPE || "DEFAULT_TEST_TYPE"
var options
if(TEST_TYPE ="TEST_TYPE1") {
 options = ....
} else {
 options = ....
}
export options

Adding the parameter as implemented in the PR would solve the issue as well, but the developer needs to be aware of that restriction.

yorugac commented 1 month ago

@ausias-armesto thanks for the update!

const TEST_TYPE= __ENV.TEST_TYPE || "DEFAULT_TEST_TYPE"

This way of using env vars repeats very often actually. For instance, it's probably present in all examples in quickpizza. Like a "k6 good practice" thing.

Perhaps, it could be pointed out more, esp. in the main docs :thinking: I can also add an additional emphasis on it in k6-operator docs for env vars, but it sounds like it is a proper workaround without having to pass system env vars to the jobs.

yorugac commented 1 month ago

I've just added the emphasis about env var defintion in k6-operator doc.

As an additional source of recommendations, we recently published this blog too: https://grafana.com/blog/2024/04/30/organizing-your-grafana-k6-performance-testing-suite-best-practices-to-get-started/ It also contains definition of env var in k6 script with the default value. Hopefully, we'll have a better centralized documentation about such things one day.

I'm closing this PR after all, as the workaround exists and is basically a standard practice for k6 scripts. Thank you again @ausias-armesto, for looking into the issue!