[REQUEST] Support specifying various PodSpec properties on the OnionService pods

[conneryn]

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:

1. add tolerations
2. set resource requests/limits
3. specify affinity rules

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 of PodSpec properties.

Describe the solution you'd like
Add a "template" property to the OnionService spec:

apiVersion: tor.k8s.torproject.org/v1alpha2
kind: OnionService
metadata:
  name: example-onion-service
spec:
  version: 3
  template:
    spec:
      priorityClassName: high-priority
      tolerations: []
      resources: {}
      affinity: {}
  rules: [...]

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.

[bugfest]

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.

[conneryn]

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!

[bugfest]

Awesome! Looking forward to hear from you

[conneryn]

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:

• Handle OnionBalancedService daemon pods.

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 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.
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:
       //...
   

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?)
4. Any other suggestions/feedback are very welcome.

[bugfest]

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 set priorityClassName, resources, topologySpreadConstraints, affinity, tolerations, nodeSelector, schedulerName, and runtimeClassName 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

[bugfest]

Hi @conneryn, PR #18 merged!