fix(k8s): run launcher run_k8s_config takes precedence over user depl…

[sohamwaghe]

…oyment config

Summary & Motivation

When includeConfigInLaunchedRuns.enabled: true is set in the Helm chart, run pods ignore the nodeSelector configured in runLauncher.config.k8sRunLauncher.runK8sConfig.podSpecConfig and instead inherit the nodeSelector from the user deployment pod. This means operators who want run pods on a different node pool (e.g. autoscaling GPU nodes) than their user deployment pods (e.g. cheap always-on nodes) have no way to enforce that.

Root cause

K8sContainerContext.create_for_run() in container_context.py builds two contexts:

context — from the run launcher config
user_defined_container_context — from the run's code origin, which is populated with user deployment config when includeConfigInLaunchedRuns is enabled

The final line was:

return context.merge(user_defined_container_context)

K8sContainerContext.merge() is defined to prefer the passed-in argument (other). So user_defined_container_context was always winning for any conflicting scalar fields like pod_spec_config.node_selector, regardless of what the run launcher explicitly configured.

Fix
Flip the merge order so the launcher config is applied last and wins on conflicts:

return user_defined_container_context.merge(context)

Non-conflicting fields from both sides are still preserved — the deep merge behavior is unchanged. Only fields that exist in both configs are now resolved in favour of the launcher, which is the correct precedence for an operator-level setting.

Files changed:

1. python_modules/libraries/dagster-k8s/dagster_k8s/container_context.py — one line changed in create_for_run()

2. python_modules/libraries/dagster-k8s/dagster_k8s_tests/unit_tests/test_container_context.py — two regression tests added

Test Plan

All 24 existing tests continue to pass
test_launcher_run_k8s_config_takes_precedence_over_container_context — verifies launcher node_selector wins over a conflicting user deployment node_selector
test_launcher_run_k8s_config_precedence_preserves_non_conflicting_fields — verifies that fields only present in the user deployment config (e.g. dns_policy) still come through after the merge

Fixes #33900

p.s open to reviews and any suggestiosn and help

Changelog

The changelog is generated by an agent that examines merged PRs and
summarizes/categorizes user-facing changes. You can optionally replace
this text with a terse description of any user-facing changes in your PR,
which the agent will prioritize. Otherwise, delete this section.

[greptile-apps]

Greptile Summary

This PR fixes a merge-order bug in K8sContainerContext.create_for_run() where user-deployment config (populated when includeConfigInLaunchedRuns.enabled: true) was overriding run launcher config for conflicting scalar fields such as pod_spec_config.node_selector. The single-line fix swaps the merge order so the run launcher's config is applied as the winning side.

• Core fix (container_context.py line 507): context.merge(user_defined_container_context) → user_defined_container_context.merge(context). Since merge() gives precedence to the other argument, the launcher config now wins over both user deployment context and run-tag-based overrides for any conflicting fields, while non-conflicting fields from the user deployment side are still preserved.
• Tests (test_container_context.py): Three new regression tests verify the new precedence for the basic conflict case, the non-conflicting field preservation case, and the include_run_tags=True path.

Confidence Score: 5/5

Safe to merge — the change is a single well-targeted line flip in create_for_run, backed by three new regression tests and no changes to the merge logic itself.

The fix is minimal and correct: swapping merge order so the run launcher's config wins over user-deployment config is exactly what the merge() semantics require, and the new tests confirm both the conflict resolution and the preservation of non-conflicting fields. All 24 pre-existing tests continue to pass.

No files require special attention.

Important Files Changed

Filename	Overview
python_modules/libraries/dagster-k8s/dagster_k8s/container_context.py	One-line fix swapping merge order so launcher config takes precedence; several inline comments removed as a side-effect.
python_modules/libraries/dagster-k8s/dagster_k8s_tests/unit_tests/test_container_context.py	Three new regression tests covering launcher precedence over container context and run tags; well-structured and correctly assert the new behavior.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant RL as RunLauncher
    participant CFR as create_for_run()
    participant UC as user_defined_context
    participant CTX as context (launcher)
    participant Result

    RL->>CFR: create_for_run(run, launcher, include_run_tags)
    CFR->>CTX: Build from launcher config (image_pull_policy, run_k8s_config, namespace, …)
    CFR->>UC: Build from run's code origin container_context
    alt "include_run_tags=True"
        CFR->>UC: Merge run tag K8s config into user_defined_context
    end
    CFR->>UC: validate_user_k8s_config_for_run (allowlist check)
    Note over CFR: OLD: context.merge(user_defined_context) → user deployment wins
    Note over CFR: NEW: user_defined_context.merge(context) → launcher wins
    CFR->>Result: user_defined_context.merge(context)
    Result-->>RL: K8sContainerContext with correct precedence

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant RL as RunLauncher
    participant CFR as create_for_run()
    participant UC as user_defined_context
    participant CTX as context (launcher)
    participant Result

    RL->>CFR: create_for_run(run, launcher, include_run_tags)
    CFR->>CTX: Build from launcher config (image_pull_policy, run_k8s_config, namespace, …)
    CFR->>UC: Build from run's code origin container_context
    alt "include_run_tags=True"
        CFR->>UC: Merge run tag K8s config into user_defined_context
    end
    CFR->>UC: validate_user_k8s_config_for_run (allowlist check)
    Note over CFR: OLD: context.merge(user_defined_context) → user deployment wins
    Note over CFR: NEW: user_defined_context.merge(context) → launcher wins
    CFR->>Result: user_defined_context.merge(context)
    Result-->>RL: K8sContainerContext with correct precedence

Loading

_{Reviews (2): Last reviewed commit: "test(k8s): add coverage for include_run_..." | Re-trigger Greptile}

[greptile-apps]

Missing coverage for include_run_tags=True path

Both new tests call create_for_run(..., include_run_tags=False). When include_run_tags=True, run tags are also merged into user_defined_container_context before the final merge with context (the launcher). A tag-defined pod_spec_config.node_selector is on the same side as the container-context-defined one, so the fix applies equally — but a test that exercises this path would make the invariant explicit and guard against a future regression where the tag-merge step is accidentally reordered after the launcher merge.

[sohamwaghe]

Good catch — adding a test for the include_run_tags=True path in the latest commit.