Closed conneryn closed 2 years ago
Hi @conneryn, thanks for supporting the project and taking the time on providing such detailed explanation!
I really like the idea; it'd bring a lot of value for users that want to have more control over the objects created by the controller.
I would be happy to start on a PR to support this, if you are happy with the strategy.
Of course! Feel free to start working on a draft and let's use this ticket to work on it. Keep me posted with your progress, happy to help with any additional question or guidance.
Is it ok If I assign you this ticket? If you cannot find the time I can put it on my queue; your call.
Great, yeah I'll take a stab at an initial PR, but will definitely appreciate your feedback/suggestions once I've got the first pass ready!
Awesome! Looking forward to hear from you
I finally had a bit of time to get an initial draft together.
The PR adds a template
property to the OnionService spec. This template can be used to set priorityClassName
, resources
, topologySpreadConstraints
, affinity
, tolerations
, nodeSelector
, schedulerName
, and runtimeClassName
of the onion-service pods.
Outstanding Items:
A few thoughts/questions:
ServicePodSpec
) to define the template properties that can be set. We could use the existing PodTemplateSpec instead, but it requires "containers" to be set (which we don't want), so we would need to work around that. Here's a project currently doing something similar here and here, if we think this is a better approach.template.spec.template
property in an OnionBalancedService will set the template for the backend OnionService(s). However, the OnionBalancedService's daemon pod is not affected. To support this, we could add a separate property to the OnionBalancedService spec (ex: daemonPodTemplate
), but it may be a little confusing as the word template
is already being used... any thoughts on what a good property name or placement could be?
For example:
apiVersion: tor.k8s.torproject.org/v1alpha3
kind: OnionBalancedService
metadata:
name: onionbalancedservice-sample
spec:
backends: 2
version: 3
daemonPodTemplate:
spec:
priorityClassName: critical
template:
spec:
template:
spec:
priorityClassName: critical
rules:
//...
Hi @conneryn! Thank you very much for your awesome work and notes. I've poked around the PR, let me answer some of your questions:
I finally had a bit of time to get an initial draft together.
The PR adds a
template
property to the OnionService spec. This template can be used to setpriorityClassName
,resources
,topologySpreadConstraints
,affinity
,tolerations
,nodeSelector
,schedulerName
, andruntimeClassName
of the onion-service pods.
Really nice!
Outstanding Items:
* [ ] Handle OnionBalancedService daemon pods.
Checked in question 2
A few thoughts/questions:
1. Currently, I am using an explicit/new object (`ServicePodSpec`) to define the template properties that can be set. We _could_ use the existing [PodTemplateSpec](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#podtemplatespec-v1-core) instead, **but** it requires "containers" to be set (which we don't want), so we would need to work around that. Here's a project currently doing something similar [here](https://github.com/dippynark/cluster-api-provider-kubernetes/blob/731d0dd6e972b325fc1a3a35931cf6724b17d7bf/hack/remove-containers-requirement.sh) and [here](https://github.com/dippynark/cluster-api-provider-kubernetes/blob/731d0dd6e972b325fc1a3a35931cf6724b17d7bf/api/v1alpha3/kubernetesmachine_webhook.go#L42), if we think this is a better approach.
I like dippynark's cluster-api-provider-kubernetes, for the time being is a nice workaround. In the future we could append tor-controller's container definition to whatever the user want to use).
2. Setting the `template.spec.template` property in an OnionBalancedService will set the template for the backend OnionService(s). However, the OnionBalancedService's daemon pod is not affected. To support this, we could add a separate property to the OnionBalancedService spec (ex: `daemonPodTemplate`), but it may be a little confusing as the word `template` is already being used... any thoughts on what a good property name or placement could be? For example: ``` apiVersion: tor.k8s.torproject.org/v1alpha3 kind: OnionBalancedService metadata: name: onionbalancedservice-sample spec: backends: 2 version: 3 daemonPodTemplate: spec: priorityClassName: critical template: spec: template: spec: priorityClassName: critical rules: //... ```
I like the approach. What do you think if we use a slighlty simpler version like balancerTemplate
? We could mention that in a separate folder for advanded examples.
I'm also considering to start building proper documentation about the usage and features of the project.
3. I'm not sure if I've gone about the API version bump correctly (ex: should I be bumping everything everywhere, or should I only bump the parts that have changes, and/or should the alpha3 version of objects inherit from alpha2, instead of redefining all the same properties?)
I've been revamping alpha2 with more features given that those are optional and shouldn't disrupt old versions of the CRD's installed. It is correct though create a new version when adding big changes; not sure if you've checked kubebuilder's API Multi-Version tutorial already. In short, when adding a new version we should pick which one is the "main" one and then work on conversions to work with multilpe versions in parallel. The idea is that internall you just work with one object representation.
Given that your modifications intrododuce optional fields to the schema, just add it to alpha2 for simplicity.
4. Any other suggestions/feedback are very welcome.
Thanks again for all the time you put into this! Really appreciate it : D
Hi @conneryn, PR #18 merged!
Is your feature request related to a problem? Please describe. I need to be able to control some spec properties on the onion service pods. My immediate pain is that I want to ensure the service continues to run even when the cluster comes under memory or CPU pressure, which means I need to be able to specify a higher
priorityClassName
for the pods.It would also be nice to be able to:
Additionally, I currently don't have any specific use-cases in mind, but I could envision other users wanting to set other pod properties (ex:
labels
,annotations
,hostNetwork
,topologySpreadConstraints
, etc). See https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#PodSpec for a full list ofPodSpec
properties.Describe the solution you'd like Add a "template" property to the
OnionService
spec:Rather than manually creating this template spec for this project, it may be best to leverage the existing "PodTemplateSpec" (although this may introduce complications/confusion if users try to define the
containers
in the spec?).Describe alternatives you've considered I considered changing the default
priorityClass
to something higher, and setting all less-crucial workloads to a lower class. This does not work for me, because there are several other 3rd party projects that don't support controlling their workloads' priority, and the OnionService is the only one I would want to be considered a high-priority class.Additional context I would love to make Onion Services the primary ingress channel into my cluster (potentially even for access to the control-plane), so I am very interested in trying to make it more robust and reliable.
I would be happy to start on a PR to support this, if you are happy with the strategy.