Add support for PodDisruptionBudgets

[JorTurFer]

Although sidecar and daemonset don't allow pdb, deployment and statefulset modes are workload types where pdbs make sense totally, as they are centralized workloads.
Currently, we are using the collector helm chart because it supports pdbs but we'd like to use the operator for using Target Allocator, but the pdbs are a requirement for us.

I think that adding support to pdbs is a good addition to the operator and I'm willing to contribute with the feature if you also agree with having it

[pavolloffay]

thanks for the request @JorTurFer

Do you want to use PodDisruptionBudgets for target allocator as well?

[JorTurFer]

I haven't tested properly the target allocator yet, but if it's something that requires HA, yes. I mean, I know that we can use affinities to prevent some scenarios, but we have to ensure that we have at least X instances of the collector working. That's why we needed the pdbs

[JorTurFer]

Do you see the request useful? I'm willing to implement (but obviously, I prefer to ask before rather than drafting a PR that never is merged xD)

[pavolloffay]

I am not well familiar with the concept perhaps @jaronoff97 ?

I found this

From version 1.15 PDBs support custom controllers where the scale subresource is enabled.

https://kubernetes.io/docs/tasks/run-application/configure-pdb/#identify-an-application-to-protect

otelcol CRD implements the scale subresource

[JorTurFer]

No no, I want to deploy a pdb for the generated deployment / statefulset. I mean, with this manifest:

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: sidecar-for-my-app
spec:
  mode: deployment

The operator generates a deployment for the collector. I want to generate also a PDB for that generated deployment, not for the CRD itself

[jaronoff97]

Hey @JorTurFer thanks for getting this going here :) Is there a reason we would do this in the operator and not just let users apply this themselves (that's what i do currently)? What pavol mentions is accurate in that if we were to do this in the operator we would want to do it the same way as how the hpa created by the operator works i.e. by targeting the scale subresource.

[JorTurFer]

oooh! got it. Actually that's better 😄

The reason for adding the pdb is that you can define all the resources within OpenTelemetryCollector manifests. I mean, why don't let users to apply the ingress themselves? or the HPA? As a user, I can define all the aspects of the collector (configs, volumes,sa, services, ingress, hpa) except the pdb, which is important for the reliability of the system. As I said, I could use anti-affinity for ensuring that there isn't 2 collector instances on the same node for being safe on node disruptions (such as upgrades, scaling in, etc ) but the pod disruption budget is already for that (it's an api explicitly created for it indeed), so defining it in the same place where I define all the other stuff sounds worth (at least in my mind).

[jaronoff97]

yeah makes sense! I think as long as we let it work with the collector in the same way as the HPA that would be great. I definitely would accept it!

[JorTurFer]

Nice!
Let me take a look and give a try. You can assign the issue to me if you want :)
I'll draft a PR and we can discuss the implementation over the code 😄

[jaronoff97]

sounds great, thanks for taking this on 🙇

[JorTurFer]

I think that I have already something to review :)

[cmergenthaler]

Hey @JorTurFer, thanks for adding the feature! Is there a reason why the PDB hasn't been added for Target Allocator as well?

[JorTurFer]

As I understood the TargetAllocator isn't critical and doesn't require HA (but maybe I'm wrong and we have to add it there too)
I mean, it does not process real time traffic but configure the collector, so 5-10 seconds down are not critical

[cmergenthaler]

Thanks for the explanation. From what I've understood, it is recommended to run at least 2 replicas of TA, so that it is also HA, see #2159 (comment)
So I think having pdb in place would be valid as well

[jaronoff97]

@cmergenthaler i think that's a valid concern – would you mind opening up another issue? I think there's reason here to do some more work to pipe these things better for user concerns. I'll add a discussion topic for our SIG meeting this week.

[cmergenthaler]

@jaronoff97 done: #2261

[JorTurFer]

Oh, I misunderstood the TA I guess.
Let's continue the discussion on the issue :)