Skip to content

feat: implement hpa primitive#20

Open
sourcehawk wants to merge 25 commits intomainfrom
feature/hpa-primitive
Open

feat: implement hpa primitive#20
sourcehawk wants to merge 25 commits intomainfrom
feature/hpa-primitive

Conversation

@sourcehawk
Copy link
Owner

Implements the hpa Kubernetes resource primitive following the pattern established by the existing ConfigMap and Deployment primitives.

Summary

  • Adds hpa primitive package under pkg/primitives/hpa/
  • Implements required lifecycle interfaces
  • Includes editors, mutator, flavors, and builder

Checklist

  • Compiles cleanly
  • Tests pass
  • Follows naming conventions in CONTEXT.md
  • Does not modify shared files

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new hpa primitive to the operator-component-framework, extending the existing primitives system with first-class support for Kubernetes autoscaling/v2 HorizontalPodAutoscaler resources (builder, mutator/editors, flavors, lifecycle handlers), plus accompanying documentation, tests, and an end-to-end example.

Changes:

  • Introduces pkg/primitives/hpa (builder/resource, mutator, handlers, and field-application flavors).
  • Adds a new shared mutation editor editors.HPASpecEditor for structured HPA spec mutation (metrics/behavior/target ref).
  • Adds documentation and a runnable example demonstrating feature-gated mutations, flavors, and data extraction.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/primitives/hpa/resource.go Defines the HPA primitive resource wrapper over generic.IntegrationResource.
pkg/primitives/hpa/builder.go Fluent builder for configuring HPA primitive (mutations, flavors, handlers, extractors).
pkg/primitives/hpa/builder_test.go Unit tests for builder validation and configuration wiring.
pkg/primitives/hpa/mutator.go Plan-and-apply mutator supporting metadata + HPA spec edit categories and feature boundaries.
pkg/primitives/hpa/mutator_test.go Unit tests for mutator edits, ordering, and multi-feature behavior.
pkg/primitives/hpa/handlers.go Default operational + suspension handler implementations for HPA.
pkg/primitives/hpa/handlers_test.go Unit tests for default handler behavior and precedence rules.
pkg/primitives/hpa/flavors.go Field-application flavors for preserving current labels/annotations.
pkg/primitives/hpa/flavors_test.go Tests for flavor behavior and registration ordering.
pkg/mutation/editors/hpaspec.go Adds HPASpecEditor for typed HPA spec mutations (metrics, behavior, etc.).
pkg/mutation/editors/hpaspec_test.go Tests for HPASpecEditor methods (metric upsert/removal, behavior, raw access).
examples/hpa-primitive/main.go Runnable fake-client example exercising reconciliation steps and suspension behavior.
examples/hpa-primitive/resources/hpa.go Example resource factory assembling base HPA + mutations/flavors/extractor.
examples/hpa-primitive/features/mutations.go Example feature-gated mutations and flavor helpers for the HPA primitive.
examples/hpa-primitive/app/controller.go Example controller wiring the HPA resource into a Component reconciliation.
examples/hpa-primitive/app/owner.go Re-exports shared example CRD types for the example package.
examples/hpa-primitive/README.md Documents how to run and what the example demonstrates.
docs/primitives/hpa.md New primitive documentation describing API, ordering, handlers, flavors, and examples.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • handlers.go:49 — Added explicit corev1.ConditionUnknown case in DefaultOperationalStatusHandler so the condition's Message is surfaced via conditionReason instead of falling through to the generic "Waiting for HPA conditions to be populated" reason.
  • handlers.go:93 — Added doc comment on DefaultSuspensionStatusHandler noting it assumes delete-on-suspend is in effect, and that overriding the deletion decision requires a custom suspend status handler.
  • docs/primitives/hpa.md:272 — Added guidance note after the WithCustomSuspendDeletionDecision example that keeping the HPA during suspension requires updating suspend mutation and status handling.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 22, 2026 19:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • handlers.go:57 — Split the fallback pending reason into two cases: "Waiting for HPA conditions to be populated" (empty conditions) vs "Waiting for ScalingActive condition on HPA" (conditions present but ScalingActive missing). Added test case for the missing-ScalingActive scenario.
  • hpaspec.go:118 — Enhanced metricsMatch for Object metrics to also compare DescribedObject, and for Pods/Object/External metrics to compare the full MetricIdentifier (Name + Selector) via a new metricIdentifiersMatch helper. Added tests for Object metrics with different DescribedObjects and External metrics with different Selectors.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 00:38
@sourcehawk sourcehawk force-pushed the feature/hpa-primitive branch from 9ba72fe to a8555be Compare March 23, 2026 00:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • resource.go:74 — Updated DeleteOnSuspend GoDoc to accurately describe that a retained HPA may attempt to scale its target, rather than claiming it has no effect.
  • examples/hpa-primitive/main.go:87 — Fixed comment from "HPA will be deleted" to "HPA is not deleted by default" to match actual default behavior.
  • docs/primitives/hpa.md:184 — Updated EnsureMetric documentation to reflect full metric identity matching rules (including Selector for Pods/External and DescribedObject for Object metrics).
  • docs/primitives/hpa.md:355 — Replaced incorrect "HPA deletion on suspend is intentional" guidance with accurate "HPA retention on suspend is the default" guidance that matches the no-op suspension behavior.

Intentionally ignored:

  • handlers.go:82 — DefaultDeleteOnSuspendHandler returning false is an intentional design decision made in commit 9ba72fe ("default suspension behavior is noop"). The no-op default was deliberately chosen to avoid unnecessary churn and simplify resumption. Updated surrounding docs to be transparent about the trade-off instead of changing the default.

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 16:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings March 23, 2026 16:58
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • resource.go:17 — Added generic.PreserveStatus(current, original) call in DefaultFieldApplicator to prevent deep copy from wiping live HPA Status.Conditions, which would cause operational status to incorrectly report Pending. Updated docstring and docs accordingly.
  • resource_test.go:55 — Added TestDefaultFieldApplicator_PreservesStatus test with Status.Conditions on current, asserting CurrentReplicas, DesiredReplicas, and Conditions are preserved after field application.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

sourcehawk and others added 7 commits March 23, 2026 20:16
Provides typed methods for setting scale target ref, min/max replicas,
metrics (with upsert-by-type/name semantics), and scaling behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nd handlers

Integration lifecycle primitive for autoscaling/v2 HorizontalPodAutoscaler.
DefaultDeleteOnSuspend = true (HPA has no native suspend field).
Operational status derived from ScalingActive and AbleToScale conditions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers capabilities, building, mutations (always/boolean/version-gated),
internal ordering, editors, operational status, suspension, flavors,
and a full autoscaling example.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates base HPA construction, CPU/memory metric mutations,
boolean-gated features, conservative scale-down behavior, flavors,
operational status reporting, delete-on-suspend, and data extraction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix custom field applicator example: preserve Status before DeepCopyInto
  overwrites it, matching the comment's intent of preserving live fields
- Fix data extraction capability description: clarify it is opt-in via
  WithDataExtractor, not automatic

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…suspend docs

- Add explicit corev1.ConditionUnknown case in DefaultOperationalStatusHandler
  so the condition's Message is surfaced instead of a generic fallback
- Add doc note on DefaultSuspensionStatusHandler that it assumes delete-on-suspend
- Add guidance in hpa.md that keeping HPA on suspend requires custom handlers

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 18:27
@sourcehawk sourcehawk review requested due to automatic review settings March 24, 2026 18:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

The DefaultSuspensionStatusHandler reason string previously asserted that
the HPA was deleted, which is misleading when consumers override deletion
behaviour via WithCustomSuspendDeletionDecision. Update the reason to be
deletion-agnostic and document that WithCustomSuspendStatus should be
used when a custom reason is needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • pkg/primitives/hpa/handlers.go:114 — Made the default suspension reason deletion-agnostic ("HorizontalPodAutoscaler suspended to prevent scaling interference") so it remains accurate when deletion behaviour is overridden via WithCustomSuspendDeletionDecision. Added doc comments noting that WithCustomSuspendStatus should be used for custom reasons. Updated test and docs to match.

Intentionally ignored:

  • Makefile:120 — This comment is about the PR checklist/description accuracy, not a code issue. The Makefile change (adding the HPA example to run-examples) is intentional and necessary for the primitive to be exercised. The PR description should be updated by the author to reflect that minimal shared files were touched.

<!-- claude-review-cycle -->

@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 18:32
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Comment on lines +126 to +130
// metricIdentifiersMatch reports whether two MetricIdentifier values match.
// It compares both Name and Selector.
func metricIdentifiersMatch(a, b autoscalingv2.MetricIdentifier) bool {
return a.Name == b.Name && reflect.DeepEqual(a.Selector, b.Selector)
}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

metricIdentifiersMatch uses reflect.DeepEqual on *metav1.LabelSelector. This treats semantically equivalent selectors as different when MatchExpressions are the same set but in a different order, which can cause EnsureMetric to append duplicates instead of upserting. Consider comparing selectors semantically (e.g., normalize/sort expressions+values before compare, or convert both selectors to a canonical labels.Selector representation and compare that).

Copilot uses AI. Check for mistakes.
Comment on lines +71 to +75
func (e *HPASpecEditor) RemoveMetric(metricType autoscalingv2.MetricSourceType, name string) {
filtered := e.spec.Metrics[:0]
for _, m := range e.spec.Metrics {
if m.Type == metricType && metricName(m) == name {
continue
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

RemoveMetric(metricType, name) matches only by type+name and ignores selector (and for Object metrics, ignores DescribedObject). Because EnsureMetric supports multiple metrics with the same name but different selector/described object, the typed API can’t remove a single specific metric identity. Consider extending RemoveMetric to remove by full identity (e.g., accept a MetricSpec/MetricIdentifier + described object), or clearly documenting that it removes all metrics for that type/name and directing users to Raw() for fine-grained removal.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Comment on lines +296 to +307
The default suspension status handler reports `Suspended` immediately with the reason
`"HorizontalPodAutoscaler suspended to prevent scaling interference"`.
Override this handler with `WithCustomSuspendStatus` if you need a reason that reflects custom deletion behaviour.

Override with `WithCustomSuspendDeletionDecision` if you want to retain the HPA during suspension (e.g. when the scale
target is managed externally and will not be present during suspension):

```go
hpa.NewBuilder(base).
WithCustomSuspendDeletionDecision(func(_ *autoscalingv2.HorizontalPodAutoscaler) bool {
return false // keep HPA during suspension
})
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

The Suspension section shows overriding WithCustomSuspendDeletionDecision to retain the HPA, but the default suspend status reason ("HorizontalPodAutoscaler deleted…") will still be reported unless WithCustomSuspendStatus is also overridden. Please document this coupling so users don’t end up with misleading suspension status/reason when they choose to retain the HPA.

Copilot uses AI. Check for mistakes.
Replace reflect.DeepEqual with order-insensitive comparison for
LabelSelectors in metricIdentifiersMatch. This prevents EnsureMetric
from appending duplicates when MatchExpressions or Values appear in a
different order but are semantically equivalent.

Also clarify RemoveMetric docs: it removes all metrics matching
type+name regardless of selector, and directs users to Raw() for
fine-grained removal.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 2 Complete

Addressed:

  • metricIdentifiersMatch (line 130): Replaced reflect.DeepEqual on *metav1.LabelSelector with a semantic comparison (labelSelectorsEqual) that normalizes MatchExpressions and Values order before comparing, preventing false negatives that could cause duplicate metric entries.
  • RemoveMetric (line 75): Enhanced documentation to explicitly state it removes all metrics matching the given type and name (regardless of selector or described object), and directs users to Raw() for fine-grained removal.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 18:36
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 18:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants