search

kube-burner / kube-burner-ocp

OpenShift integrations and workloads for kube-burner
https://kube-burner.github.io/kube-burner-ocp/
Apache License 2.0
4 stars 14 forks source link

Making ocp-wrapper compatable with kube-burner v1.9.6 #56

Closed vishnuchalla closed 2 months ago

vishnuchalla commented 3 months ago

Type of change

Description

Making code changes compatible with latest kb version .i.e. v1.9.6

Checklist before requesting a review

josecastillolema commented 3 months ago

From the web-burner workload perspective lgtm

afcollins commented 3 months ago

Thanks for doing this!

I believe with these changes, the documentation is incorrect.

While we are touching that section, the command oc sa new-token is deprecated. When you run it, it says to use oc create token instead. We should also include that change as part of the docs change.

Also, by hard-coding metricsDirectory in the job config, we are reverting some functionality to generate the local dir with the uuid in the file name. We should preserve that functionality so we are not overwriting the same dir on repeated runs.

      metricsDirectory: reporting-metrics
afcollins commented 3 months ago

I looked and the appending uuid seems to be removed upstream in kube-burner#611 . I guess I missed that there, but it should be fixed before we merge this PR.

vishnuchalla commented 3 months ago

Thanks for doing this!

I believe with these changes, the documentation is incorrect.

While we are touching that section, the command oc sa new-token is deprecated. When you run it, it says to use oc create token instead. We should also include that change as part of the docs change.

Also, by hard-coding metricsDirectory in the job config, we are reverting some functionality to generate the local dir with the uuid in the file name. We should preserve that functionality so we are not overwriting the same dir on repeated runs.

      metricsDirectory: reporting-metrics

Thanks for all the inputs regarding docs @afcollins. Updated them accordingly. Regarding metrics directory, I idea of this change is to leave it up to the user to provide a list of non-colliding metric directory names for their runs in local. Let me know if you have any concerns around this idea.

cc: @rsevilla87

rsevilla87 commented 3 months ago

Thanks for doing this! I believe with these changes, the documentation is incorrect. While we are touching that section, the command oc sa new-token is deprecated. When you run it, it says to use oc create token instead. We should also include that change as part of the docs change. Also, by hard-coding metricsDirectory in the job config, we are reverting some functionality to generate the local dir with the uuid in the file name. We should preserve that functionality so we are not overwriting the same dir on repeated runs.

      metricsDirectory: reporting-metrics

Thanks for all the inputs regarding docs @afcollins. Updated them accordingly. Regarding metrics directory, I idea of this change is to leave it up to the user to provide a list of non-colliding metric directory names for their runs in local. Let me know if you have any concerns around this idea.

cc: @rsevilla87

correct, that's the idea, but it doesn't work well though, all metrics get indexed to reporting-metrics, but that's due to a bug in the go-commons library...

Regarding the issue mentioned by @afcollins, we could conifgure metricsDirectory with something like the below to fix it.

      metricsDirectory: reporting-metrics-{{.UUID}}
vishnuchalla commented 3 months ago

Thanks for doing this! I believe with these changes, the documentation is incorrect. While we are touching that section, the command oc sa new-token is deprecated. When you run it, it says to use oc create token instead. We should also include that change as part of the docs change. Also, by hard-coding metricsDirectory in the job config, we are reverting some functionality to generate the local dir with the uuid in the file name. We should preserve that functionality so we are not overwriting the same dir on repeated runs.

      metricsDirectory: reporting-metrics

Thanks for all the inputs regarding docs @afcollins. Updated them accordingly. Regarding metrics directory, I idea of this change is to leave it up to the user to provide a list of non-colliding metric directory names for their runs in local. Let me know if you have any concerns around this idea. cc: @rsevilla87

correct, that's the idea, but it doesn't work well though, all metrics get indexed to reporting-metrics, but that's due to a bug in the go-commons library...

Regarding the issue mentioned by @afcollins, we could conifgure metricsDirectory with something like the below to fix it.

      metricsDirectory: reporting-metrics-{{.UUID}}

Appending UUID at the end looks good to me, if we do not want any manual intervention at all.

rsevilla87 commented 2 months ago

Hi, I created a PR in go-commons to fix/improve the indexers code to allow creating multiple indexers of the same kind https://github.com/cloud-bulldozer/go-commons/pull/45