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

feat: add use of tolerations for starter and runner #142

Merged
merged 3 commits into from
Sep 2, 2022

Conversation

spoukke
Copy link
Contributor

@spoukke spoukke commented Aug 10, 2022

This PR aims to add support for Tolerations for the starter and the different runner pods.

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.

Hi @spoukke, thanks for the PR! Could you please update manifests as well? We have make manifests to help with that.

@yorugac
Copy link
Collaborator

yorugac commented Aug 14, 2022

And one more thing: I think tolerations should be added to all pods in the operator, i.e. to the initializer pod as well.

@spoukke
Copy link
Contributor Author

spoukke commented Aug 18, 2022

Hi @yorugac ! I re-generated the manifests.

From what I see in the codebase, the Initializer is of type *batchv1.Job so it will directly inherit the filed Tolerations from corev1.PodTemplate.

Also, I was wondering why you did not use the corev1.PoTemplate for the Runner et Starter fields instead of recreating your own struct. If you do this, you will directly have all the fields form a Pods, and the operator will automatically follow the updates from the Kubernetes API.

@spoukke spoukke force-pushed the feat/spoukke/add-tolerations branch from 707710e to 7f55435 Compare August 23, 2022 13:07
@spoukke
Copy link
Contributor Author

spoukke commented Aug 23, 2022

Hello @yorugac ! I juste understood what you wnted regarding the initializer. Now, the runner tolerations are mapped to the intializer one.

Let me know if there is anything else

@spoukke spoukke requested a review from yorugac August 23, 2022 13:12
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.

@spoukke thank you for the update! LGTM, merging this in now.

I've tested this a bit and in case a pod cannot be scheduled controller will be stuck at initialization. It's not related to this PR, just something that is more noticeable with usage of taints 😞 So one more TODO point to the #138.

And I missed this:

Also, I was wondering why you did not use the corev1.PoTemplate for the Runner et Starter fields instead of recreating your own struct. If you do this, you will directly have all the fields form a Pods, and the operator will automatically follow the updates from the Kubernetes API.

Good question! I cannot say for certain but I suspect it was simply easier to implement it this way. Looking it up, it seems like PodTemplate didn't always have a stable behaviour in Kubernetes API? But I think given the way fields are being added, it's worth to look into it in greater detail, to understand the pros & cons. Thanks for feedback and pointer!

@yorugac yorugac merged commit c13609d into grafana:main Sep 2, 2022
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.

None yet

2 participants