⚠️ No Changeset found

Latest commit: a847ef6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

PR Review: Node Affinity Rules for Large Machine Worker Pool Scheduling

Summary

This PR adds Kubernetes node affinity configuration to separate large machine workloads (large-1x, large-2x) from standard workloads across different node pools. This is a well-targeted optimization for reducing scheduling delays during peak times.

Code Quality & Design

Strengths:

• Clean, focused implementation with minimal changes
• Feature is opt-in via KUBERNETES_LARGE_MACHINE_POOL_LABEL env var - no breaking changes
• Good asymmetric strategy: large machines get soft preference (can fall back to standard nodes) while standard machines have hard exclusion from the large pool
• Comments clearly explain the scheduling behavior

Observations:

1. Hardcoded machinepool label key (kubernetes.ts:378,399): The node label key is hardcoded to "machinepool". Consider whether this should be configurable alongside the label value, or at minimum, document this requirement in the env var comment. Operators will need to know their nodes must have the machinepool=<value> label.

2. Machine name prefix matching (kubernetes.ts:362): The #isLargeMachine function uses preset.name.startsWith("large-"). This works correctly with the current MachinePresetName enum (large-1x, large-2x), but be aware this couples the scheduling logic to the naming convention.

Potential Issues

1. No unit tests: The apps/supervisor/src/workloadManager/ directory has no test files. Given this is Kubernetes scheduling logic that affects production workload placement, consider adding tests for:

   • #isLargeMachine() correctly identifies large presets
   • #getNodeAffinity() returns undefined when env var is not set
   • Correct affinity structure for large vs non-large machines

   Per CLAUDE.md guidelines, tests should use testcontainers, but these particular functions are pure logic and could be unit tested directly.

2. Interaction with existing nodeSelector: The #defaultPodSpec already sets a nodeSelector based on KUBERNETES_WORKER_NODETYPE_LABEL. The new affinity rules are additive, which should work correctly with Kubernetes scheduling, but operators should understand both constraints apply.

Performance Considerations

The implementation looks efficient - no additional API calls or heavy computation. The affinity configuration is built once per pod creation.

Security

No security concerns. The feature is configuration-driven and affects pod scheduling only.

Minor Suggestions

1. The env var comment could be more descriptive:

   // Current:
   KUBERNETES_LARGE_MACHINE_POOL_LABEL: z.string().optional(), // if set, large-* presets affinity for machinepool=<value>
   // Suggested:
   KUBERNETES_LARGE_MACHINE_POOL_LABEL: z.string().optional(), // If set, applies node affinity: large-* presets prefer nodes with label machinepool=<value>, others avoid them

2. Consider adding a changeset if this is a notable feature for managed deployments (though since this is in apps/supervisor and not a public package, it may not be required).

Verdict

Approve with minor suggestions. The implementation is clean, well-scoped, and solves a real scheduling problem with a sensible asymmetric approach. The main suggestion is to add unit tests for the affinity logic to prevent regressions.

Review Complete

Your review story is ready!

View Story

Comment !reviewfast on this PR to re-generate the story.

PR Review: Node Affinity Rules for Large Machine Worker Pool Scheduling

Summary

This PR adds Kubernetes node affinity rules to separate large machine workloads (large-1x, large-2x) from standard workloads, addressing scheduling delays during peak times. The implementation is clean and follows the existing patterns in the codebase.

Code Quality

Strengths:

• Clean, well-structured code that follows existing patterns in the file
• Good use of private methods with descriptive names (#isLargeMachine, #getNodeAffinity)
• Appropriate use of soft preference (preferredDuringSchedulingIgnoredDuringExecution) for large machines, allowing fallback to standard nodes
• Hard constraint (requiredDuringSchedulingIgnoredDuringExecution) for standard machines prevents them from consuming large pool capacity
• Feature is opt-in via env var, maintaining backward compatibility

Minor Suggestions:

1. Consider using the existing PLACEMENT_TAGS_PREFIX (which defaults to node.cluster.x-k8s.io) for the node label key instead of hardcoding node.cluster.x-k8s.io/machinepool. This would allow for more flexibility.

2. The comment in the env file could be slightly more descriptive about what value to provide.

Potential Issues

1. Edge case - Nodes without the label: When KUBERNETES_LARGE_MACHINE_POOL_LABEL is set, non-large machines use a NotIn constraint. If a node does not have the node.cluster.x-k8s.io/machinepool label at all, the NotIn constraint will still allow scheduling on that node (since the label is simply absent, not matching the excluded value). This is probably the desired behavior but worth confirming.

2. No DoesNotExist handling: Standard workloads can still schedule on nodes that do not have the machinepool label at all. Consider whether you want to add an additional term to handle nodes without the label. Though this might be unnecessary complexity if your cluster setup guarantees all nodes have this label.

Performance Considerations

• The affinity rules add minimal overhead to pod scheduling
• Using soft preference (weight 100) for large machines is a good balance - it strongly prefers the large pool but allows spillover
• No performance concerns identified

Security Considerations

• No security concerns - this is purely a scheduling optimization
• The env var pattern follows existing conventions

Test Coverage

• No unit tests added for the new #isLargeMachine and #getNodeAffinity methods
• I see the supervisor has some test files (e.g., envUtil.test.ts, podCleaner.test.ts)
• Consider adding tests for:
  • #isLargeMachine returns true for large-1x, large-2x and false for other presets
  • #getNodeAffinity returns undefined when env var is not set
  • #getNodeAffinity returns correct affinity structure for large and non-large machines

Overall Assessment

This is a well-designed, practical solution to improve scheduling performance for large machine workloads. The implementation is clean and follows existing patterns. The main suggestion is to add unit tests for the new logic to ensure correctness and prevent regressions.

Recommendation: Approve with minor suggestions (consider tests and the optional improvements noted above).