Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for volume and volume mounts for runners #249

Merged
merged 5 commits into from
Aug 2, 2023

Conversation

andrei-trandafir
Copy link
Contributor

@andrei-trandafir andrei-trandafir commented Jul 19, 2023

Currently there is no way to add different volume mounts to the runner pod, other than the default one that is created from the script spec. This PR adds support volumes and volume mounts.

The scenario we're aiming to utilise these in is when we're fetching the k6 script(s) and other artifacts from an external source (i.e. S3) through init containers, and copying them to the main containers using a Volume of type EmptyDir.

Closes #250

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrei-trandafir I have a change request: could you please remove changes to Makefile and instead add a new file, smth like e2e/test-initcontainer-volumes.yaml, which uses this new fields?

Otherwise, the changes seem OK to me and thanks for adding the Go test 🙂

@andrei-trandafir
Copy link
Contributor Author

Thanks for the review @yorugac , i've reverted the Makefile changes and added a new sample k6 file in e2e.

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrei-trandafir thanks for updates! The yaml test needs a very small update: it fails at the moment, please see the comment.

And one more thing. When trying to deploy from your branch as is, I hit an interesting error about byte limit:

The CustomResourceDefinition "k6s.k6.io" is invalid: metadata.annotations: Too long: must have at most 262144 bytes

It seems it can be resolved by adding parameter crd:maxDescLen=0 in Makefile. But since you didn't add this parameter as part of PR, I wonder have you encountered this error during your testing?

e2e/test-initcontainer-volumes.yaml Outdated Show resolved Hide resolved
Co-authored-by: Olha Yevtushenko <yorugac@users.noreply.github.com>
@andrei-trandafir
Copy link
Contributor Author

andrei-trandafir commented Aug 2, 2023

@yorugac thanks for spotting the issue. I probably copied the wrong file 😅

Regards the CRD install, I believe I encountered the issue once and didn't keep the solution in the Makefile in the end.
The way I solved it was by adding the following flag to the kubectl apply in the deploy command in the Makefile:

--server-side=true

This avoids using the "last-applied-configuration" annotation from what I understand.

@yorugac
Copy link
Collaborator

yorugac commented Aug 2, 2023

OK, I see. Actually, --server-side=true seems to work only on clean slate, i.e. without existing k6-operator deployment. Otherwise, I get these errors:

error: Apply failed with 9 conflicts: conflicts with "kubectl-client-side-apply" using apiextensions.k8s.io/v1beta1:
- .spec.validation.openAPIV3Schema.properties.spec.properties.initializer.properties.initContainers.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.initializer.properties.volumeMounts.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.initializer.properties.volumes.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.runner.properties.initContainers.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.runner.properties.volumeMounts.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.runner.properties.volumes.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.starter.properties.initContainers.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.starter.properties.volumeMounts.items
- .spec.validation.openAPIV3Schema.properties.spec.properties.starter.properties.volumes.items

But also, for 'cleaner' solution I think it's better to change make manifests with crd:maxDescLen=0 and not make deploy with --server-side=true: so that there's an impact only on manifest generation during development and not on the bundle generation during deployments and releases. So, could you please add that crd:maxDescLen=0 to make manifests as well? Assuming that option works in your setup as well 🙂

@andrei-trandafir
Copy link
Contributor Author

Added and tested the changes. Everything seems in order 👍🏼
Thanks @yorugac

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Seems to work AFAIS 👍
Thanks for the changes 😄

@yorugac yorugac merged commit b5125f4 into grafana:main Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Volume and VolumeMount support for runners
2 participants