Skip to content

Feature: Allow workloadKind: Deployment - #873

Open
Maleware wants to merge 15 commits into
mainfrom
feat/opa-as-deployment
Open

Feature: Allow workloadKind: Deployment#873
Maleware wants to merge 15 commits into
mainfrom
feat/opa-as-deployment

Conversation

@Maleware

@Maleware Maleware commented Aug 11, 2026

Copy link
Copy Markdown
Member

Description

HINT: Takes spike/rif as base as changes overlap. Please merge #863 first.

Allow opa being deployed as workload deployment.

This PR contains:

  • opaCluster being deployable either as workloadKind Daemonset or Deployment
  • podDisruptionBudgets only for workloadKind Deployment
  • Affinities to fan out pods when Deployment is used across all nodes if possible
  • Integration tests for swap Daemonset <> Deployment. Checks defaults for localTrafficPolicy is set correctly according to workloadKind
  • Documentation on workloadKind mechanism, PDBs and default affinities.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

  • Changes are OpenShift compatible
  • CRD changes approved
  • CRD documentation for all fields, following the style guide.
  • Helm chart can be installed and deployed operator works
  • Integration tests passed (for non trivial changes)
  • Changes need to be "offline" compatible
  • Links to generated (nightly) docs added
  • Release note snippet added

Reviewer

  • Code contains useful comments
  • Code contains useful logging statements
  • (Integration-)Test cases added
  • Documentation added or updated. Follows the style guide.
  • Changelog updated
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

  • Feature Tracker has been updated
  • Proper release label has been added
  • Links to generated (nightly) docs added
  • Release note snippet added
  • Add type/deprecation label & add to the deprecation schedule
  • Add type/experimental label & add to the experimental features tracker

@Maleware
Maleware changed the base branch from main to spike/rif August 11, 2026 09:20
@Maleware Maleware self-assigned this Aug 11, 2026
@Maleware Maleware moved this to Development: In Progress in Stackable Engineering Aug 11, 2026
@Maleware
Maleware marked this pull request as ready for review August 12, 2026 10:47
@Maleware
Maleware marked this pull request as draft August 12, 2026 10:47
@Maleware
Maleware changed the base branch from spike/rif to main August 14, 2026 16:15
@Maleware
Maleware changed the base branch from main to spike/rif August 14, 2026 16:23
@Maleware
Maleware marked this pull request as ready for review August 19, 2026 13:00
@Maleware Maleware moved this from Development: In Progress to Development: Waiting for Review in Stackable Engineering Aug 19, 2026
@Maleware
Maleware marked this pull request as draft August 20, 2026 06:54
@Maleware

Copy link
Copy Markdown
Member Author

I turned it back into a draft so it cannot be merged by accident. Still ready for review.

Base automatically changed from spike/rif to main September 2, 2026 14:13
…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
@Maleware
Maleware force-pushed the feat/opa-as-deployment branch from 47d0fa2 to 61b8c20 Compare September 2, 2026 16:06
@Maleware

Maleware commented Sep 2, 2026

Copy link
Copy Markdown
Member Author
--- PASS: kuttl (1322.14s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/smoke_opa-1.16.2_openshift-false_use-tls-false (76.79s)
        --- PASS: kuttl/harness/openldap-user-info_opa-latest-1.16.2_openshift-false (88.01s)
        --- PASS: kuttl/harness/workload-kind_opa-latest-1.16.2_openshift-false (50.31s)
        --- PASS: kuttl/harness/smoke_opa-1.12.3_openshift-false_use-tls-true (79.86s)
        --- PASS: kuttl/harness/resources_opa-latest-1.16.2_openshift-false (19.21s)
        --- PASS: kuttl/harness/smoke_opa-1.12.3_openshift-false_use-tls-false (61.69s)
        --- PASS: kuttl/harness/logging_opa-1.16.2_openshift-false (79.47s)
        --- PASS: kuttl/harness/logging_opa-1.12.3_openshift-false (79.65s)
        --- PASS: kuttl/harness/aas-user-info_opa-latest-1.16.2_openshift-false (369.40s)
        --- PASS: kuttl/harness/config-overrides_opa-latest-1.16.2_openshift-false (58.94s)
        --- PASS: kuttl/harness/keycloak-user-info_opa-latest-1.16.2_keycloak-23.0.1_openshift-false (145.68s)
        --- PASS: kuttl/harness/smoke_opa-1.16.2_openshift-false_use-tls-true (66.54s)
        --- PASS: kuttl/harness/cluster-operation_opa-latest-1.16.2_openshift-false (54.29s)
        --- PASS: kuttl/harness/data-hub-resource-info_opa-latest-1.16.2_trino-latest-481_kafka-latest-4.2.1_superset-latest-6.1.0_data-hub-1.0.3_data-hub-prerequisites-0.3.0_openshift-false (1053.66s)
PASS

@Maleware
Maleware marked this pull request as ready for review September 2, 2026 16:35
@Maleware
Maleware requested a review from a team September 2, 2026 16:36
@labrenbe
labrenbe self-requested a review September 10, 2026 12:02
@labrenbe labrenbe moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Sep 10, 2026

@labrenbe labrenbe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When workload is DaemonSet should we log a warning that replicas is set but ignored?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly should we log a warning that a PDB is configured but ignored when workload is DaemonSet?

@Maleware Maleware Sep 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is a bit confusing to me

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah :D Done with: 55dd7af

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it makes sense. Can you elaborate on what is confusing you?

Comment thread rust/operator-binary/src/controller/build/resource/service.rs Outdated
Comment thread rust/operator-binary/src/controller/build/resource/service.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Development: In Review

Development

Successfully merging this pull request may close these issues.

2 participants