diff --git a/keps/8-reduce-yaml-duplication/README.md b/keps/8-reduce-yaml-duplication/README.md index aea295a1..0ed4e99e 100644 --- a/keps/8-reduce-yaml-duplication/README.md +++ b/keps/8-reduce-yaml-duplication/README.md @@ -2,7 +2,7 @@ ## Summary -This KEP introduces `roleTemplates` field in RBG spec, enabling platform teams to define reusable Pod configurations that multiple roles can reference via `templateRef`. Uses Kubernetes Strategic Merge Patch for merging, reducing YAML duplication by approximately 20%. +This KEP introduces `roleTemplates` field in RBG spec, enabling platform teams to define reusable Pod configurations that multiple roles can reference via `templateRef`. Roles use a `templatePatch` field to overlay role-specific configurations. Uses Kubernetes Strategic Merge Patch for merging, reducing YAML duplication by approximately 20%. ## Motivation @@ -14,6 +14,7 @@ Managing multi-role RBG deployments requires duplicating base configurations acr ### Goals - Introduce `spec.roleTemplates` array for reusable Pod configurations - Enable `role.templateRef` to reference templates within same RBG +- Introduce `role.templatePatch` field for overlay configurations when using RoleTemplate - Use Kubernetes Strategic Merge Patch for configuration merging - No template variables (no `{{.var}}` syntax) @@ -30,20 +31,81 @@ Managing multi-role RBG deployments requires duplicating base configurations acr **Story 2**: SRE updates `roleTemplates[0].template.spec.containers[0].image`, controller automatically rolls update to all referencing roles. +**Story 3**: User runs `kubectl rbg preview` to visualize merged configurations before applying. + ### Risks and Mitigations + - **Field naming ambiguity**: Using single field name for both complete config and overlay could cause confusion + - **Mitigation**: Design separate fields (`template` for traditional mode, `templatePatch` for overlay mode) with mutual exclusivity, following Kubernetes ecosystem naming conventions (e.g., Argo Workflows `podSpecPatch`) + - **Strategic Merge complexity**: Users may not understand how volumes, env, and other fields merge, leading to configuration errors - **Mitigation 1**: Provide clear merge behavior table in documentation, explaining which fields merge by name vs complete replacement - - **Mitigation 2**: Add dry-run capability to rbgctl (similar to `kubectl apply --dry-run`) to preview merged configuration before applying + - **Mitigation 2**: Add preview/diff tools to visualize and compare merged configurations - **Template reference errors**: Roles referencing non-existent templates cause deployment failures - **Mitigation**: Controller validates templateRef exists during reconciliation and returns explicit error messages -- **Unexpected field overrides**: Users unclear which roleTemplate fields are overridden by role.template +- **Unexpected field overrides**: Users unclear which roleTemplate fields are overridden by role.templatePatch - **Mitigation**: Document Strategic Merge behavior explicitly - resources/command use complete replacement, volumes/env merge by name ## Design Details +### Naming Convention + +KEP-8 introduces the `templatePatch` field, following the `{target}Patch` naming pattern to align with Kubernetes ecosystem conventions: + +- **Kubernetes ecosystem precedent**: Argo Workflows uses `podSpecPatch` (not `patchPodSpec`) +- **Semantic clarity**: `templatePatch` clearly indicates "a patch for the template" (noun + modifier) +- **API design principle**: Field names should describe content/type (noun), not operations (verb) + +**Current naming landscape**: +```yaml +# KEP-8 (new, aligned with ecosystem) +role.templatePatch # {target}Patch pattern + +# Existing LWS fields (retained for backward compatibility) +leaderWorkerSet.patchLeaderTemplate # patch{Target} pattern +leaderWorkerSet.patchWorkerTemplate # patch{Target} pattern +``` + +**Future consideration**: A separate proposal may standardize existing LWS fields to `leaderTemplatePatch` / `workerTemplatePatch` for consistency, but this is deferred to avoid breaking changes in v1alpha1. + +### Field Semantics: Template vs TemplatePatch + +**Traditional Mode**: +```yaml +roles: +- name: prefill + template: # Complete configuration + spec: {...} +``` + +**With RoleTemplate**: +```yaml +roleTemplates: +- name: base + template: # Base template + spec: {...} + +roles: +- name: prefill + templateRef: {name: base} + templatePatch: # Overlay configuration + spec: {...} +``` + +**Key Points**: +- `template` and `templatePatch` are mutually exclusive in `role` +- Naming follows `{target}Patch` convention (aligned with Argo Workflows `podSpecPatch`) +- Note: Existing LWS fields `patchLeaderTemplate`/`patchWorkerTemplate` retain current naming for backward compatibility + +**Validation**: + +| templateRef | template | templatePatch | +|-------------|----------|---------------| +| Not set | Required | Rejected | +| Set | Rejected | Required | + ### API Overview **Basic Example (StatefulSet)**: @@ -93,7 +155,7 @@ spec: replicas: 2 templateRef: name: sglang-base - template: + templatePatch: spec: containers: - name: sglang @@ -119,7 +181,7 @@ spec: replicas: 1 templateRef: name: sglang-base - template: + templatePatch: spec: containers: - name: sglang @@ -182,6 +244,13 @@ spec: replicas: 1 templateRef: name: sglang-base + templatePatch: + spec: + containers: + - name: sglang + env: + - name: MODEL + value: Qwen3-32B workload: apiVersion: leaderworkerset.x-k8s.io/v1 kind: LeaderWorkerSet @@ -210,16 +279,19 @@ spec: - "python3 -m sglang.launch_server --model-path /models/Qwen3-32B --tp 2 --dist-init-addr $(LWS_LEADER_ADDRESS):20000 --nnodes $(LWS_GROUP_SIZE) --node-rank $(LWS_WORKER_INDEX)" ``` -In LWS scenarios, roleTemplate provides shared configuration (image, volumes, resources), while patchLeaderTemplate/patchWorkerTemplate customize distributed commands using LWS environment variables. +In LWS scenarios, roleTemplate provides shared configuration (image, volumes, resources), `role.templatePatch` adds role-specific config, and patchLeaderTemplate/patchWorkerTemplate customize distributed commands. **Merge Behavior**: | Field | Merge Strategy | Example | |-------|---------------|---------| -| Volumes | Merge by `name` | Template has volume "model", role adds "cache" → Both present | -| Env | Merge by `name` | Template has env "POD_IP", role adds "ROLE" → Both present | -| Resources | Replace entirely | Template has 2 GPU, role specifies 4 GPU → 4 GPU used | -| Command | Replace entirely | Template has base cmd, role specifies full cmd → Role cmd used | +| Volumes | Merge by `name` | Template has volume "model", templatePatch adds "cache" → Both present | +| Env | Merge by `name` | Template has env "POD_IP", templatePatch adds "ROLE" → Both present | +| Resources | Replace entirely | Template has 2 GPU, templatePatch specifies 4 GPU → 4 GPU used | +| Command | Replace entirely | Template has base cmd, templatePatch specifies full cmd → templatePatch cmd used | + +> Uses Kubernetes [Strategic Merge Patch](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#use-a-strategic-merge-patch-to-update-a-deployment) for native merge semantics. + ### Controller Behavior @@ -227,31 +299,36 @@ In LWS scenarios, roleTemplate provides shared configuration (image, volumes, re ```mermaid flowchart TD A[Reconcile Role] --> B{Has templateRef?} - B -->|Yes| C[Fetch roleTemplate by name] - C --> D[Strategic Merge: roleTemplate.template + role.template] - D --> E[Create StatefulSet] - B -->|No| F[Use role.template directly] - F --> E + B -->|Yes| C[Validate: templatePatch is set] + C --> D[Fetch roleTemplate by name] + D --> E[Strategic Merge: roleTemplate.template + role.templatePatch] + E --> F[Create StatefulSet] + B -->|No| G[Validate: template is set] + G --> H[Use role.template directly] + H --> F ``` **LWS Scenario**: ```mermaid flowchart TD A[Reconcile Role] --> B{Has templateRef?} - B -->|Yes| C[Fetch roleTemplate by name] - C --> D[Strategic Merge: roleTemplate.template + role.template] - D --> E[Merged Base Template] - B -->|No| E - - E --> F[Apply patchLeaderTemplate to Base] - F --> G[Apply patchWorkerTemplate to Base] - G --> H[Create LeaderWorkerSet] + B -->|Yes| C[Validate: templatePatch is set] + C --> D[Fetch roleTemplate by name] + D --> E[Strategic Merge: roleTemplate.template + role.templatePatch] + E --> F[Merged Base Template] + B -->|No| G[Validate: template is set] + G --> H[Use role.template] + H --> F + + F --> I[Apply patchLeaderTemplate to Base] + I --> J[Apply patchWorkerTemplate to Base] + J --> K[Create LeaderWorkerSet] ``` **Processing Order**: -1. Validate templateRef exists in roleTemplates array -2. Fetch roleTemplate by name -3. Strategic Merge: `roleTemplate.template` + `role.template` → merged base template +1. Validate: `template` XOR `templatePatch` based on `templateRef` +2. Fetch roleTemplate by name (if using RoleTemplate) +3. Strategic Merge: `roleTemplate.template` + `role.templatePatch` → merged base template 4. For LWS workloads: - Apply `patchLeaderTemplate` to merged base → Leader Pod spec - Apply `patchWorkerTemplate` to merged base → Worker Pod spec @@ -259,11 +336,7 @@ flowchart TD - Use merged base template directly 6. Create workload (StatefulSet or LeaderWorkerSet) -**Important**: The `role.template` field serves as both: -- Source for Strategic Merge with roleTemplate -- Base template for LWS patchLeaderTemplate/patchWorkerTemplate - -This means patchLeaderTemplate/patchWorkerTemplate apply **after** roleTemplate merging. +**Important**: The `role.templatePatch` provides overlay for roleTemplate, then the merged result serves as base for LWS patches. This means patchLeaderTemplate/patchWorkerTemplate apply **after** roleTemplate merging. #### Update Workflow @@ -293,7 +366,7 @@ flowchart TD **Update Trigger**: - Each workload carries a `role-revision-hash-{roleName}` label - Reconciler compares current label value with expected hash from new Revision -- If different → Resolves template (merging updated roleTemplate + role.template) → Updates workload via Server-Side Apply +- If different → Resolves template (merging updated `roleTemplate.template` + `role.templatePatch`) → Updates workload via Server-Side Apply - Workload controller (StatefulSet/Deployment/LWS) detects PodTemplate change → Triggers rolling update per rolloutStrategy #### ControllerRevision Evolution During roleTemplate Update @@ -326,11 +399,73 @@ kubectl apply -f rbg-with-image-v1.0.yaml Rollback creates a new Revision (incrementing number) with previous content, maintaining linear history while allowing bidirectional updates. +### Preview and Diff Tools + +To help users understand Strategic Merge behavior, KEP-8 provides preview/diff capabilities via kubectl plugins: + +#### Preview: Visualize Merged Configurations + +```bash +kubectl rbg preview my-rbg.yaml +``` + +Resolves and displays the final PodTemplateSpec for each role after merging `roleTemplate.template` + `role.templatePatch`. + +**Output Example**: +```yaml +# Role: prefill (resolved) +--- +spec: + containers: + - name: sglang + image: sglang:v0.5.1 # from roleTemplate + command: [...] # from templatePatch + resources: + limits: + nvidia.com/gpu: "2" # from roleTemplate + +# Role: decode (resolved) +--- +spec: + containers: + - name: sglang + image: sglang:v0.5.1 # from roleTemplate + command: [...] # from templatePatch (different from prefill) +``` + +#### Diff: Compare Configurations + +```bash +kubectl rbg diff my-rbg.yaml +``` + +Compares resolved configurations between local YAML file and cluster state. + +**Output Example**: +```diff +Role: prefill +- image: sglang:v0.5.0 # cluster ++ image: sglang:v0.5.1 # local + +Role: decode +- image: sglang:v0.5.0 ++ image: sglang:v0.5.1 +``` + +**Use Cases**: +- Validate merge behavior before applying changes +- Understand impact of roleTemplate modifications on all referencing roles +- Review configuration changes during PR reviews + +**Implementation**: Extends existing `kubectl-rbg-*` plugin pattern with new commands. + ### Test Plan **Unit Tests**: - Template resolution by name - Strategic Merge logic for different field types (volumes, env, resources, command) +- Validation: `templatePatch` requires `templateRef` +- Validation: `template` and `templatePatch` are mutually exclusive - Validation: templateRef references non-existent template - Validation: templateRef references template in different RBG (should fail) @@ -338,6 +473,8 @@ Rollback creates a new Revision (incrementing number) with previous content, mai - Create RBG with roleTemplates, verify merge behavior - Update roleTemplate image, verify all referencing roles updated - LWS scenario: verify patchLeaderTemplate/patchWorkerTemplate applied after merge +- Preview: verify resolved output correctness +- Diff: verify comparison logic **E2E Tests**: - Deploy multi-role RBG using roleTemplates @@ -346,8 +483,9 @@ Rollback creates a new Revision (incrementing number) with previous content, mai - LWS deployment: verify leader and worker Pods have correct merged configuration **Manual Verification**: -- Use `rbgctl get rbg --dry-run` to preview merged configuration before applying -- Verify error messages when templateRef is invalid +- Use `kubectl rbg preview` to visualize merged configuration before applying +- Use `kubectl rbg diff` to compare local changes with cluster state +- Verify error messages when templateRef is invalid or fields are mutually exclusive ## Alternatives @@ -360,6 +498,8 @@ Rollback creates a new Revision (incrementing number) with previous content, mai - RBAC complexity (who can modify templates?) - Template versioning and compatibility issues +**Single `template` field with dual semantics**: Simpler API but confusing semantics (same field, different meaning based on `templateRef`). Community feedback favored explicit `templatePatch` field. + **RoleTemplate approach chosen because**: - Templates scoped to single RBG (simpler RBAC) - Native Strategic Merge Patch (Kubernetes-native behavior) @@ -393,6 +533,11 @@ roles: - name: prefill templateRef: name: sglang-base + templatePatch: + spec: + containers: + - name: sglang + # extraArgs appended to base command extraArgs: - --disaggregation-mode - prefill @@ -407,3 +552,4 @@ roles: - **2025-10-21**: Initial KEP-8 proposal submitted - **2025-10-24**: Revised to focus on RoleTemplates (Phase 1), defer ExtraArgs - **2025-10-27**: Supplemented Controller Behavior section with update workflow and ControllerRevision evolution details +- **2025-10-30**: Updated API design with `role.templatePatch` field and preview/diff tooling, aligned naming with Kubernetes ecosystem conventions \ No newline at end of file