Feature: Allow workloadKind: Deployment - #873
Conversation
|
I turned it back into a draft so it cannot be merged by accident. Still ready for review. |
…rs.roleConfig Replaces the EmptyRoleConfig parameter of OpaRoleType with a product-specific OpaRoleConfig, as the extension point for deploying the servers role as either a DaemonSet or a Deployment (#525). The CRD half only - no builder, PDB or orphan-cleanup changes. Both internalTrafficPolicy and podDisruptionBudget.enabled are Options with a null default, because their effective default depends on workloadKind and an OpenAPI default cannot express that. The operator derives them instead: workloadKind | internalTrafficPolicy | podDisruptionBudget.enabled DaemonSet | Local | false Deployment | Cluster | true A hard-coded schema default would let the apiserver stamp a value in before the operator sees the object, so "user chose Local" and "user said nothing" would be indistinguishable. It would also create a PodDisruptionBudget on every existing DaemonSet installation at upgrade time, protecting nothing. Decision: stackabletech/decisions#91
47d0fa2 to
61b8c20
Compare
|
labrenbe
left a comment
There was a problem hiding this comment.
Looking good in general. My main concern is if switching workload kind really has to be disruptive.
| pub struct OpaRoleConfig { | ||
| /// The Kubernetes workload the OPA servers run as. | ||
| /// | ||
| /// * `DaemonSet`: one Pod per node. `replicas` is ignored. |
There was a problem hiding this comment.
When workload is DaemonSet should we log a warning that replicas is set but ignored?
There was a problem hiding this comment.
I wouldn't log it since in terms of a DaemonSet, replicas do not exist the way they would for a STS or Deployment. And since it cannot make sense I wouldn't be verbose about this.
| /// Whether a PodDisruptionBudget should be written out for this role. | ||
| /// | ||
| /// Defaults to `true` when `workloadKind` is `Deployment` and to `false` when it is | ||
| /// `DaemonSet`, since a PodDisruptionBudget doesn't make sense for a DaemonSet. |
There was a problem hiding this comment.
Similarly should we log a warning that a PDB is configured but ignored when workload is DaemonSet?
There was a problem hiding this comment.
PDBs do not make sense in the eyes of a DaemonSet with internalTrafficPolicy: Local as any disruption of any pod would lead to impact on the platform components on the same node regardless how many pods are left on other nodes.
However, more generic, DaemonSets do not implement the scale subresource (according to docs) means we cannot use maxUnavailable. Best we might be able to do is minAvailable which is an integer. Since a DS scales with node counts, minAvailable wouldn't give us the effect we'd want from a PDB.
Means, I wouldn't warn about ignoring something which was never intended to be used in the first place.
|
|
||
| == Changing the workload kind | ||
|
|
||
| Changing `workloadKind` replaces the workload object, so policy queries can fail while the new Pods start up. |
There was a problem hiding this comment.
Maybe we could deploy both workloads in parallel and delete the old workload + change the service only once the new workload is available to avoid disruption.
There was a problem hiding this comment.
I like the idea a lot and gave it some thought during implementation. I didn't do it for following reasons.
First, realising this would lead to a steep increase of resources used on a cluster. This of course depends on the replica and node count respectively, but can lead to a state where a Deployment or DeamonSet cannot be scheduled fully which leaves us somehow stuck half way in the switch.
Which leads directly to the second point: What is the exact definition of the "new" cluster being ready enough? Ports open? Pods being served? Status: ready? I wasn't able to come up with something decent to guarantee correct availability.
Last, building this would need at least some sort of a state machine. We would somehow need to persist a "mid-migration" marker (could be an annotation for example) which needs to be placed and removed according to the newly deployed clusters state, which is kinda complex and needs some code to orchestrate that.
All in all it was for me to much effort for a result which has a difficult to foresee and recover failure mode.
This said, if someone has a better idea how this can be achieved easily, I'd really like to have it.
| @@ -0,0 +1,33 @@ | |||
| = Pod placement | |||
| :description: The default affinity spreads OPA Pods across Kubernetes nodes, which matters for a Deployment, and how to override it with your own affinities. | |||
There was a problem hiding this comment.
This sentence is a bit confusing to me
| /// note on [`get_affinity`]. | ||
| const ANTI_AFFINITY_BETWEEN_ROLE_PODS_WEIGHT: i32 = 70; | ||
|
|
||
| /// The default affinity of `role`: prefer to spread its Pods across nodes. |
There was a problem hiding this comment.
I have to say I find this doc comment a bit confusing. I'm not completely sure why, maybe because it's mixing a bunch of things...
There was a problem hiding this comment.
To me it makes sense. Can you elaborate on what is confusing you?
Co-authored-by: Benedikt Labrenz <benedikt@labrenz.org>
Co-authored-by: Benedikt Labrenz <benedikt@labrenz.org>
…-operator into feat/opa-as-deployment
Description
HINT: Takesspike/rifas base as changes overlap. Please merge #863 first.Allow opa being deployed as workload deployment.
This PR contains:
opaClusterbeing deployable either as workloadKindDaemonsetorDeploymentpodDisruptionBudgetsonly for workloadKindDeploymentDeploymentis used across all nodes if possibleDaemonset<>Deployment. Checks defaults forlocalTrafficPolicyis set correctly according toworkloadKindworkloadKindmechanism,PDBsand default affinities.Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker