refactor(operator): extract Grove workload rendering from graph controller

[julienmancuso]

Summary

This PR introduces a dedicated groveWorkloadRenderer and moves Grove-specific workload rendering out of the common DynamoGraphDeployment controller.

It advances the composition-first design by giving the Grove pathway a clearer internal structure:

1. groveProgram orchestrates the complete Grove pathway.
2. groveWorkloadRenderer resolves render inputs and constructs the desired PodCliqueSet.
3. groveProgram synchronizes that object and aggregates its readiness.
4. The common controller no longer owns Grove rendering or compatibility transformations.

Changes

• Add groveWorkloadRenderer with explicit dependencies:
  • Kubernetes client
  • operator configuration
  • runtime configuration
  • Docker secret retriever
• Introduce explicit groveRenderInputs containing:
  • the DGD used for rendering
  • the existing PodCliqueSet, when present
• Move Grove render preparation and compatibility handling into the renderer:
  • legacy worker-selector compatibility
  • restart annotation preservation
  • topology-constraint upgrades
  • clique and group ordering preservation
  • managed replica preservation
• Make groveProgram responsible for:
  • resolving Grove render inputs
  • requesting the desired PodCliqueSet
  • synchronizing the PodCliqueSet
  • wrapping the synchronized object for readiness aggregation
• Remove the corresponding Grove-specific rendering methods from the common graph controller.
• Update existing characterization and program tests to exercise the new boundary.

Design notes

This is deliberately an intermediate boundary.

The existing GenerateGrovePodCliqueSet implementation still performs some Kubernetes-backed input resolution. The new renderer therefore does not claim to be a pure rendering function yet. Instead, it makes those dependencies and responsibilities explicit so resolution and pure rendering can be separated cleanly in a follow-up.

The renderer does not reconcile resources, register watches, manage finalizers, or write status. Those orchestration responsibilities remain with groveProgram and the top-level reconciler.

Behavior

This PR is intended to preserve existing behavior. In particular, it retains:

• legacy worker identity compatibility
• native Grove worker identity behavior
• topology-constraint repair during upgrades
• existing PodCliqueSet ordering
• replica counts managed by rolling updates
• restart annotations
• existing readiness aggregation and error handling

---

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[sttts]

One conceptual follow-up on separating observation from deterministic rendering.

[sttts]

Disclosure: This comment was written by Codex as its interpretation of a discussion with Stefan; it is not Stefan’s text.

I like this intermediate seam. As a follow-up, I would turn it into a hard boundary between retrieval and rendering, not merely split the current work between two methods:

• ResolveGroveWorkloadModel is the effectful retrieval step. It may read Kubernetes resources, configuration, topology information, and secrets, and returns a complete, typed, read-only model.
• RenderPodCliqueSet is a side-effect-free function. It performs no I/O, has no Kubernetes client or secret retriever, writes no resources, and does not mutate the primary DGD or its input model. The same model must produce an equivalent desired PodCliqueSet.

Today, renderPodCliqueSet(ctx, req) looks like request -> desired object, but its real input also includes r.Client, configuration, the secret retriever, and any Kubernetes-backed observations performed by GenerateGrovePodCliqueSet. The explicit request is therefore not yet the complete semantic input, and rendering cannot be tested as an autonomous brick.

A concrete Grove-specific shape could be:

// groveWorkloadModel is a read-only snapshot of everything required to
// construct the desired Grove resources. It is derived from the primary DGD;
// it is not the mutable primary object itself.
type groveWorkloadModel struct {
    RenderDGD            *nvidiacomv1beta1.DynamoGraphDeployment
    ExistingPodCliqueSet *grovev1alpha1.PodCliqueSet

    RestartState    *dynamo.RestartState
    CheckpointInfos map[string]*checkpoint.CheckpointInfo

    RestartAnnotations map[string]string

    // Add the concrete topology, secret, and other facts currently resolved
    // through the Kubernetes client by GenerateGrovePodCliqueSet.
}

type groveWorkloadResolver struct {
    client                client.Reader
    config                *configv1alpha1.OperatorConfiguration
    runtimeConfig         *commoncontroller.RuntimeConfig
    dockerSecretRetriever dockerSecretRetriever
}

// Effectful, read-only retrieval: cluster/configuration -> complete model.
func (r *groveWorkloadResolver) ResolveGroveWorkloadModel(
    ctx context.Context,
    dgd *nvidiacomv1beta1.DynamoGraphDeployment,
    restartState *dynamo.RestartState,
    checkpointInfos map[string]*checkpoint.CheckpointInfo,
) (groveWorkloadModel, error)

// Pure rendering: complete model -> desired object.
func RenderPodCliqueSet(
    model groveWorkloadModel,
) (*grovev1alpha1.PodCliqueSet, error)

ResolveGroveWorkloadModel would perform all Kubernetes-backed reads, create the compatibility-adjusted RenderDGD, and resolve every environmental fact needed for rendering. RenderPodCliqueSet would only transform that model into the desired object. In particular, it would have no context.Context, client, reader, writer, or retriever available to it.

The program would retain ownership of the flow and all writes:

model, err := p.resolver.ResolveGroveWorkloadModel(
    ctx,
    req.DGD,
    req.RestartState,
    req.CheckpointInfos,
)
if err != nil {
    return ReconcileResult{}, err
}

desired, err := RenderPodCliqueSet(model)
if err != nil {
    return ReconcileResult{}, err
}

synced, err := p.syncPodCliqueSet(ctx, req.DGD, desired)
// scaling, services, readiness, and result construction remain here

This preserves the composition-first property: groveProgram still decides what is called when and in which sequence. Retrieval and rendering are concrete Grove-owned bricks; neither controls reconciliation flow. The side-effect-free renderer can also be tested exhaustively and with deliberately varied model inputs, independently of a fake Kubernetes client.

I would not introduce a generic provider-renderer interface here. ResolveGroveWorkloadModel and RenderPodCliqueSet should remain concrete, Grove-specific abstractions until independently developed code demonstrates a genuinely shared contract.

[sttts]

Look rather at #12263 (review). I don't think the model pattern is a good one. It creates an unstructured bag of stuff.

[sttts]

Disclosure: This comment was written by Codex as its interpretation of a discussion with Stefan; it is not Stefan’s text.

Overall assessment

I reviewed the cumulative stack from main through this PR.

The direction is good. The DGD controller is becoming a composition root:

DGD controller
    → select a workload program
    → invoke it once
    → persist the returned status
    → emit queued status-transition events

Each workload program owns its order and timing. The main remaining issue is that these new boundaries are still largely implemented as methods on the all-powerful DynamoGraphDeploymentReconciler. The architecture is visible in method names, but it is not yet sufficiently expressed through concrete ownership and dependency boundaries.

The general rule I would apply is:

Make composition explicit through concrete struct nesting. Let types and fields identify the responsibility; keep method names short and standardized.

1. Make the program contract explicit

I would simplify the request and result types to:

type Request struct {
    // DGD is the mutable primary object.
    //
    // Programs may mutate and directly persist non-status fields. Status is
    // returned through Result and persisted once by the outer reconciler.
    DGD *nvidiacomv1beta1.DynamoGraphDeployment
}

type Event struct {
    Type    string
    Reason  string
    Message string
}

type Result struct {
    ctrl.Result

    // Status is the complete desired DGD status after this attempt.
    Status nvidiacomv1beta1.DynamoGraphDeploymentStatus

    // Events contains events for status transitions that become real only
    // after Status has been persisted.
    Events []Event
}

func NewResult(dgd *nvidiacomv1beta1.DynamoGraphDeployment) Result {
    return Result{
        Status: dgd.DeepCopy().Status,
    }
}

func (r *Result) Eventf(
    eventType string,
    reason string,
    format string,
    args ...any,
) {
    r.Events = append(r.Events, Event{
        Type:    eventType,
        Reason:  reason,
        Message: fmt.Sprintf(format, args...),
    })
}

type workloadProgram interface {
    Reconcile(context.Context, Request) (Result, error)
}

The current resolvedFacts is empty and Request.Facts is unused. I would remove both:

type Request struct {
    DGD *nvidiacomv1beta1.DynamoGraphDeployment
}

If a real common input appears later, add it explicitly. An empty Facts, Inputs, ComputedState, or similar container encourages an unstructured state bag.

Result.Status should be authoritative. The selected program owns its complete status, including state, conditions, rollout/restart status, and ObservedGeneration. The outer reconciler only persists it.

The result-on-error contract should also be documented:

Result.Status remains meaningful when Reconcile returns an error. The outer reconciler persists the completed part of the result before returning the error.

That matches the current behavior where rollout or checkpoint status may have changed before a later reconciliation step fails.

2. Replace method-name composition with struct composition

The programs should contain concrete reconcilers instead of the complete DGD reconciler:

type componentProgram struct {
    rollout         *dgdWorkerRolloutReconciler
    sharedResources *dgdSharedResourcesReconciler
    restart         *dgdRestartReconciler
    workloads       *dcdGraphReconciler
    scalingAdapters *dgdScalingAdaptersReconciler
}

type groveProgram struct {
    rollout         *dgdUnsupportedRolloutReconciler
    sharedResources *dgdSharedResourcesReconciler
    restart         *dgdRestartReconciler
    workloads       *groveWorkloadReconciler
    scalingAdapters *dgdScalingAdaptersReconciler
    topology        *dgdGroveTopologyConditionReconciler
}

The program flow then reads as composition:

func (p *componentProgram) Reconcile(
    ctx context.Context,
    req Request,
) (Result, error) {
    result := NewResult(req.DGD)

    if err := p.rollout.Reconcile(ctx, req.DGD, &result); err != nil {
        result.Fail(req.DGD.Generation, reasonRollingUpdateFailed, err)
        return result, err
    }

    checkpoints, err := p.sharedResources.Reconcile(ctx, req.DGD)
    if err != nil {
        result.Fail(req.DGD.Generation, reasonFailedToReconcileResources, err)
        return result, err
    }
    result.Status.Checkpoints = checkpoints.Statuses

    restart, err := p.restart.Reconcile(ctx, req.DGD, result.Status)
    if err != nil {
        result.Fail(req.DGD.Generation, reasonFailedToReconcileResources, err)
        return result, err
    }

    workloads, err := p.workloads.Reconcile(
        ctx,
        req.DGD,
        restart.State,
        checkpoints.Infos,
    )
    if err != nil {
        result.Fail(req.DGD.Generation, reasonFailedToReconcileResources, err)
        return result, err
    }

    result.Status.State = workloads.State
    result.Status.Components = workloads.ComponentStatus
    result.Status.Restart = restart.Status
    result.Status.ObservedGeneration = req.DGD.Generation

    ready := readyCondition(
        req.DGD.Generation,
        result.Status,
        workloads,
    )
    meta.SetStatusCondition(&result.Status.Conditions, ready)

    if err := p.scalingAdapters.Reconcile(ctx, req.DGD, workloads); err != nil {
        result.Fail(req.DGD.Generation, reasonFailedToReconcileResources, err)
        return result, err
    }
    return result, nil
}

Here readyCondition is an ordinary policy function. It produces the final condition once, including rollout state. No outer layer subsequently overrides it.

This is clearer than:

p.reconciler.reconcileWorkerRollout(...)
p.reconciler.reconcileProgramInputs(...)
p.reconciler.resolveProgramRestartState(...)
p.reconciler.reconcileWorkloads(...)
p.reconciler.reconcileProgramResult(...)

It also creates autonomous test boundaries. Each concrete reconciler can be instantiated independently, and its constructor and method signature make the required fixtures explicit:

reconciler := &dgdCheckpointsReconciler{
    client: fakeClient,
    config: checkpointConfig,
}
checkpoints, err := reconciler.Reconcile(ctx, dgd)

This test needs only checkpoint-related resources and configuration. It does not need a fully configured DynamoGraphDeploymentReconciler with unrelated rollout, discovery, ingress, scaling, and event dependencies.

Similarly:

reconciler := &dgdRestartReconciler{
    reader: fakeClient,
}
restart, err := reconciler.Reconcile(ctx, dgd, status)

Its fixtures describe precisely which immediate objects restart reconciliation may observe.

This improves more than readability:

• constructors declare external dependencies;
• method parameters and return values declare the data contract;
• test fixtures declare observable external state;
• failures can be injected independently;
• program tests can focus on ordering and composition.

No shared interface is required merely because these structs expose methods named Reconcile.

The standardized method names should have stable meanings:

• Reconcile: observe and converge external state.
• Resolve: retrieve or derive without writes or event emission.
• Render: construct desired resources.
• Get: perform a direct lookup.
• Ensure: perform one explicit creation or convergence operation.
• Eventf: append a status-transition event to a result.

I would not introduce a provider lifecycle interface such as:

type WorkloadProvider interface {
    Prepare(...)
    Render(...)
    Scale(...)
    CheckReady(...)
    Cleanup(...)
}

That would move ownership of order and timing into the outer controller. The workload program should continue owning its complete flow.

3. Turn the implicit reconcilers in reconcileProgramInputs into actual reconcilers

reconcileProgramInputs currently performs effectful reconciliation of:

• planner and EPP RBAC;
• top-level PVCs;
• discovery resources;
• GMS ResourceClaimTemplates;
• checkpoints;
• EPP resources;
• the wait-for-leader ConfigMap;
• MPI SSH keys.

These are already potential independent reconcilers. They should become concrete reconcilers with their own dependencies, tests, and files:

type dgdSharedResourcesReconciler struct {
    rbac              *dgdRBACReconciler
    pvcs              *dgdPVCReconciler
    discovery         *dgdDiscoveryReconciler
    gmsResourceClaims *dgdGMSResourceClaimsReconciler
    checkpoints       *dgdCheckpointsReconciler
    epp               *dgdEPPReconciler
    waitForLeader     *dgdWaitForLeaderReconciler
    sshKeys           *dgdSSHKeysReconciler
}

The shared reconciler explicitly composes them:

func (r *dgdSharedResourcesReconciler) Reconcile(
    ctx context.Context,
    dgd *nvidiacomv1beta1.DynamoGraphDeployment,
) (DGDCheckpointsResult, error) {
    if err := r.rbac.Reconcile(ctx, dgd); err != nil {
        return DGDCheckpointsResult{}, err
    }
    if err := r.pvcs.Reconcile(ctx, dgd); err != nil {
        return DGDCheckpointsResult{}, err
    }
    if err := r.discovery.Reconcile(ctx, dgd); err != nil {
        return DGDCheckpointsResult{}, err
    }
    if err := r.gmsResourceClaims.Reconcile(ctx, dgd); err != nil {
        return DGDCheckpointsResult{}, err
    }

    checkpoints, err := r.checkpoints.Reconcile(ctx, dgd)
    if err != nil {
        return DGDCheckpointsResult{}, err
    }

    if err := r.epp.Reconcile(ctx, dgd); err != nil {
        return DGDCheckpointsResult{}, err
    }
    if err := r.waitForLeader.Reconcile(ctx, dgd); err != nil {
        return DGDCheckpointsResult{}, err
    }
    if dgd.HasAnyMultinodeComponent() {
        if err := r.sshKeys.Reconcile(ctx, dgd); err != nil {
            return DGDCheckpointsResult{}, err
        }
    }
    return checkpoints, nil
}

The checkpoint result is domain-specific because checkpoints actually produce data required by later operations:

type DGDCheckpointsResult struct {
    Infos    map[string]*checkpoint.CheckpointInfo
    Statuses map[string]nvidiacomv1beta1.ComponentCheckpointStatus
}

Other reconcilers may only return an error:

type dgdPVCReconciler struct {
    client client.Client
    config *configv1alpha1.OperatorConfiguration
}

func (r *dgdPVCReconciler) Reconcile(
    ctx context.Context,
    dgd *nvidiacomv1beta1.DynamoGraphDeployment,
) error

The criterion should be cohesive ownership, not mechanical fragmentation:

• If a block reconciles a distinct resource family, give it a reconciler.
• If it requires a distinct dependency set, give it a reconciler.
• If it has an independently testable fixture contract, give it a reconciler.
• Keep ordinary pure calculations as functions or local expressions.

The files should consistently use the DGD prefix:

dgd_workload_program.go
dgd_component_program.go
dgd_grove_program.go
dgd_shared_resources_reconciler.go
dgd_restart_reconciler.go
dgd_rbac_reconciler.go
dgd_pvc_reconciler.go
dgd_discovery_reconciler.go
dgd_gms_resource_claims_reconciler.go
dgd_checkpoints_reconciler.go
dgd_epp_reconciler.go
dgd_wait_for_leader_reconciler.go
dgd_ssh_keys_reconciler.go
dgd_scaling_adapters_reconciler.go
dgd_grove_topology_condition_reconciler.go

4. Inject client.Reader where code only reads

The Grove renderer currently embeds client.Client:

type groveWorkloadRenderer struct {
    client.Client
    config                *configv1alpha1.OperatorConfiguration
    runtimeConfig         *commoncontroller.RuntimeConfig
    dockerSecretRetriever dockerSecretRetriever
}

Its Kubernetes access is read-only: it reads the existing PodCliqueSet, topology bindings, and checkpoint resources.

The important dependency reduction is therefore not inventing object-specific reader interfaces. It is injecting the existing controller-runtime client.Reader instead of client.Client:

type groveWorkloadRenderer struct {
    reader                client.Reader
    config                *configv1alpha1.OperatorConfiguration
    runtimeConfig         *commoncontroller.RuntimeConfig
    dockerSecretRetriever dockerSecretRetriever
}

func newGroveWorkloadRenderer(
    reader client.Reader,
    config *configv1alpha1.OperatorConfiguration,
    runtimeConfig *commoncontroller.RuntimeConfig,
    dockerSecretRetriever dockerSecretRetriever,
) *groveWorkloadRenderer {
    return &groveWorkloadRenderer{
        reader:                reader,
        config:                config,
        runtimeConfig:         runtimeConfig,
        dockerSecretRetriever: dockerSecretRetriever,
    }
}

There is no need for GroveRenderRequest, GroveWorkloadModel, an object-specific reader interface, or a getter wrapper. Pass the actual inputs and keep retrieved values local:

func (r *groveWorkloadRenderer) Render(
    ctx context.Context,
    dgd *nvidiacomv1beta1.DynamoGraphDeployment,
    restartState *dynamo.RestartState,
    checkpointInfos map[string]*checkpoint.CheckpointInfo,
) (*grovev1alpha1.PodCliqueSet, error) {
    existing := &grovev1alpha1.PodCliqueSet{}
    key := types.NamespacedName{
        Name:      dynamo.PCSNameForDGD(dgd.Name, dgd.Spec.Components),
        Namespace: dgd.Namespace,
    }
    if err := r.reader.Get(ctx, key, existing); err != nil {
        if !apierrors.IsNotFound(err) {
            return nil, fmt.Errorf("get PodCliqueSet %s: %w", key, err)
        }
        existing = nil
    }

    effectiveDGD := dgd.DeepCopy()
    applyGroveCompatibility(effectiveDGD, existing)

    desired, err := dynamo.GenerateGrovePodCliqueSet(
        ctx,
        effectiveDGD,
        r.config,
        r.runtimeConfig,
        r.reader,
        r.dockerSecretRetriever,
        restartState,
        restartAnnotationsFromPodCliqueSet(existing),
        checkpointInfos,
    )
    if err != nil {
        return nil, err
    }

    prepareGroveTopologyConstraintUpgrade(desired, existing)
    preserveGrovePodCliqueSetOrder(desired, existing)
    preserveGrovePodCliqueSetReplicas(desired, existing, checkpointInfos)
    return desired, nil
}

GenerateGrovePodCliqueSet and the functions below it should likewise accept client.Reader rather than client.Client where they only perform Get or List:

func GenerateGrovePodCliqueSet(
    ctx context.Context,
    dgd *nvidiacomv1beta1.DynamoGraphDeployment,
    config *configv1alpha1.OperatorConfiguration,
    runtimeConfig *commoncontroller.RuntimeConfig,
    reader client.Reader,
    secretsRetriever SecretsRetriever,
    restartState *RestartState,
    existingRestartAnnotations map[string]string,
    checkpointInfos map[string]*checkpoint.CheckpointInfo,
) (*grovev1alpha1.PodCliqueSet, error)

This creates a hard write boundary without adding custom one-method interfaces.

The same rule should be applied throughout:

• Inject client.Reader when a unit only performs Get or List.
• Inject client.Writer when it only mutates ordinary resources.
• Give status-writing capability only to the outer reconciler.
• Use client.Client when a concrete reconciler genuinely needs both reads and writes.

Do not extract a function merely to rename a single Get. Extract only when it represents reusable policy, a non-trivial invariant, or a cohesive operation.

For DCD rendering, checkpoint.EnsureStoragePVC is a write currently performed in rendering code. That operation should either move to a concrete reconciler before rendering:

type dcdWorkloadReconciler struct {
    checkpointStorage *dcdCheckpointStorageReconciler
    renderer           *dcdWorkloadRenderer
}

func (r *dcdWorkloadReconciler) Reconcile(
    ctx context.Context,
    dcd *nvidiacomv1beta1.DynamoComponentDeployment,
) error {
    if err := r.checkpointStorage.Ensure(ctx, dcd.Namespace); err != nil {
        return err
    }

    desired, err := r.renderer.Render(ctx, dcd)
    // Reconcile desired objects.
}

Or the renderer should receive that specific creator operation. It should not receive a complete client.Client merely because one nested path performs one write.

This should remain pragmatic. A client.Reader on a narrowly scoped renderer is fine. The goal is not purity at any cost; it is a clear capability boundary without argument bags or trivial wrapper abstractions.

5. Make each program own its complete returned status

Replace:

ReadyReason  Reason
ReadyMessage Message

by having the program install the final Ready condition directly into Result.Status.Conditions.

There should not be a separate Result.Ready value that the outer reconciler later merges or overrides. That would split condition ownership.

The program resolves and installs the condition once:

ready := readyCondition(
    req.DGD.Generation,
    result.Status,
    workloads,
)
meta.SetStatusCondition(&result.Status.Conditions, ready)

The readiness policy includes rollout state:

func readyCondition(
    generation int64,
    status nvidiacomv1beta1.DynamoGraphDeploymentStatus,
    workloads ReconcileResult,
) metav1.Condition {
    if rollingUpdateInProgress(status.RollingUpdate) {
        return metav1.Condition{
            Type:               "Ready",
            Status:             metav1.ConditionFalse,
            ObservedGeneration: generation,
            Reason:             "rolling_update_in_progress",
            Message:            "Rolling update in progress",
        }
    }

    readyStatus := metav1.ConditionFalse
    if workloads.State == nvidiacomv1beta1.DGDStateSuccessful {
        readyStatus = metav1.ConditionTrue
    }
    return metav1.Condition{
        Type:               "Ready",
        Status:             readyStatus,
        ObservedGeneration: generation,
        Reason:             string(workloads.Reason),
        Message:            string(workloads.Message),
    }
}

Program failures likewise update their own returned status:

func (r *Result) Fail(
    generation int64,
    reason Reason,
    err error,
) {
    r.Status.State = nvidiacomv1beta1.DGDStateFailed
    meta.SetStatusCondition(&r.Status.Conditions, metav1.Condition{
        Type:               "Ready",
        Status:             metav1.ConditionFalse,
        ObservedGeneration: generation,
        Reason:             string(reason),
        Message:            err.Error(),
    })
}

The existing outer propagateTopologyCondition reads a Grove PodCliqueSet and interprets Grove-specific status. Move that observation into the Grove program through a nested reconciler:

func (r *dgdGroveTopologyConditionReconciler) Reconcile(
    ctx context.Context,
    dgd *nvidiacomv1beta1.DynamoGraphDeployment,
    result *Result,
) error {
    // Read the Grove condition through client.Reader.
    // Update result.Status.Conditions.
    // Queue a transition event through result.Eventf when appropriate.
}

The outer controller then persists the returned status without modifying it:

programResult, programErr := program.Reconcile(
    ctx,
    Request{DGD: dgd},
)

dgd.Status = programResult.Status
statusErr := r.Status().Update(ctx, dgd)

On the program path, condition ownership is therefore unambiguous:

Program:
    computes conditions
    installs conditions into Result.Status

Outer reconciler:
    persists Result.Status unchanged

6. Emit events from the layer that persists their edge

There are two different kinds of edges.

Locally persisted mutation edges

If a reconciler locally detects a meaningful object change and its client mutation succeeds, the edge has already happened. It may emit the event directly afterward:

changed := applyDesiredAnnotations(service, desiredAnnotations)
if changed {
    if err := r.client.Update(ctx, service); err != nil {
        return fmt.Errorf("update Service %s: %w", service.Name, err)
    }
    r.recorder.Eventf(
        dgd,
        corev1.EventTypeNormal,
        "ServiceUpdated",
        "Updated Service %s",
        service.Name,
    )
}

The rule is:

Emit a direct mutation event only when the reconciler locally detects a meaningful change and the corresponding client mutation succeeds.

Therefore:

• Create: emit only when this call actually created the object, not after an ignored AlreadyExists.
• Update: emit only when a semantic difference caused the update.
• Patch: emit only when the calculated patch represents a semantic difference.
• Delete: emit only when this call actually requested deletion, not after an ignored NotFound.
• No-op synchronization: no event.

Status transition edges

If the edge exists only in the returned, in-memory DGD status, direct emission is premature. The event must be returned with that status:

previous := meta.FindStatusCondition(
    result.Status.Conditions,
    conditionType,
)
next := resolvedCondition(...)
meta.SetStatusCondition(&result.Status.Conditions, next)
if conditionChanged(previous, next) {
    result.Eventf(
        corev1.EventTypeWarning,
        next.Reason,
        "Topology condition changed: %s",
        next.Message,
    )
}

The restart reconciler similarly returns the next status without emitting directly:

previousRestart := result.Status.Restart
restart, err := p.restart.Reconcile(ctx, req.DGD, result.Status)
if err != nil {
    result.Fail(req.DGD.Generation, reasonFailedToReconcileResources, err)
    return result, err
}
if becameSuperseded(previousRestart, restart.Status) {
    result.Eventf(
        corev1.EventTypeWarning,
        "RestartSuperseded",
        "Restart %s superseded by rolling update",
        restart.Status.ObservedID,
    )
}
result.Status.Restart = restart.Status

The outer reconciler persists status and only then emits queued status-transition events:

programResult, programErr := program.Reconcile(
    ctx,
    Request{DGD: dgd},
)

dgd.Status = programResult.Status
statusErr := r.Status().Update(ctx, dgd)
if statusErr != nil {
    return programResult.Result, statusErr
}
for _, event := range programResult.Events {
    r.Recorder.Event(dgd, event.Type, event.Reason, event.Message)
}
return programResult.Result, programErr

The resulting rule is:

Events for locally detected semantic mutations may be emitted directly after the mutation succeeds. Events representing DGD status transitions must be returned with the status and emitted only after that status is persisted.

The current computeRestartStatus should therefore not emit RestartSuperseded itself. Likewise, the unsupported-rollout warning should be emitted only after the worker-hash mutation representing that edge succeeds.

Recommendation

I support the direction of the stack. The workload programs now own their control flow, and the outer DGD controller is becoming a real composition root.

I would establish these changes now:

• remove the empty Facts;
• make Result.Status authoritative;
• have each program install its own conditions;
• document partial Result.Status on error;
• add Result.Events and Result.Eventf for status-transition events;
• stop emitting status-transition events from compute and resolve paths;
• inject client.Reader instead of client.Client for read-only paths.

The next structural step should move the implicit reconcilers off DynamoGraphDeploymentReconciler into concrete nested dgd*Reconciler structs with separate dgd_*_reconciler.go files.

The guiding principle is:

Composition should be visible in the struct graph. Methods should use short, standardized verbs. Each reconciler should own one coherent responsibility and only the capabilities required for that responsibility.

[datadog-official]

🔄 Datadog auto-retried 1 job - 1 passed on retry

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: d7f86f1 | Docs | Datadog PR Page | Give us feedback!}