Skip to content

Feature/generic subresource#34

Merged
AnnaYue merged 34 commits into
mainfrom
feature/generic-subresource
Apr 23, 2026
Merged

Feature/generic subresource#34
AnnaYue merged 34 commits into
mainfrom
feature/generic-subresource

Conversation

@AnnaYue
Copy link
Copy Markdown
Contributor

@AnnaYue AnnaYue commented Apr 23, 2026

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

6. Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

AnnaYue and others added 30 commits April 16, 2026 17:46
Add a generic SubResourceAdapter interface and utilities for managing
subresources (PVCs, Services, etc.) in XSet controllers:

- SubResourceAdapter interface for generic subresource management
- NameTruncator for DNS-compliant name truncation with hash suffix
- LabelManager for label value handling with original value tracking
- TemplateHash utility using FNV-32a for consistent hashing
- SubResourceAdapterGetter interface for XSetController
- Documentation and implementation plan

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement SubResourceAdapter for PVC and Service resources:

- PvcSubResourceAdapter: manages PVC lifecycle with XSet
- ServiceSubResourceAdapter: example adapter for Service resources
- Both adapters support RecreateWhenXSetUpdated for recreation on updates

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add SubResourceControl to manage subresource lifecycle during XSet updates:

- RecreateWhenXSetUpdated: recreate subresources when XSet is updated
- SyncTargets: sync subresources with targets
- CleanupOnDelete: cleanup subresources when XSet is deleted
- Unit tests for SubResourceControl

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Integrate SubResourceControl into the XSet reconcile loop:

- Add SubResourceControl to SyncControl interface
- Wire subresource lifecycle into SyncTargets, Update, and Delete flows
- Add SyncContext fields for subresource management
- Update XSetController to initialize SubResourceControl

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Design for adding optional GetTargetPrefix and GetSubResourcePrefix
methods to allow controllers and adapters to customize naming prefixes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Plan for implementing optional GetTargetPrefix and GetSubResourcePrefix
methods with tests using aether leaderset controller.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add optional method for customizing target name prefixes.
Controllers can implement this to override the default naming pattern.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add optional method for customizing subresource name prefixes.
Adapters can implement this to override the default naming pattern.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Simplify GetTargetsPrefix to accept an optional override parameter.
Truncation logic moved to controller implementation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Check for optional TargetPrefixGetter implementation and use custom prefix
if provided. Falls back to default naming if not implemented.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add helper function for subresource prefix generation with override support.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Check for optional SubResourcePrefixGetter implementation and use custom prefix
if provided. Falls back to default naming if not implemented.

Also update PVC and Service adapters to stop setting Name directly, since
GenerateName is now set by SubResourceControl.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rceAdapter

Remove GetAttachedResourceNames method from the SubResourceAdapter interface
and refactor all usages to use label-based filtering instead.

The method was fundamentally flawed - it returned ALL subresources attached
to a target, not just those managed by xset. This could lead to incorrect
behavior when a target has external subresources.

Changes:
- Remove GetAttachedResourceNames from SubResourceAdapter interface
- Remove implementation from PvcSubResourceAdapter
- Delete unused ServiceSubResourceAdapter
- Refactor deleteUnusedResourcesForAdapter: remove "is still attached" check
  (K8s finalizers already prevent deletion of in-use PVCs)
- Refactor CheckAllowIncludeExclude: use GetFilteredResources + filterByTarget
- Refactor OrphanTargetResources: use GetFilteredResources + filterByTarget
- Refactor AdoptTargetResources: find orphaned resources by selector + orphaned
  label, then check if mounted to target using SubResourcePvcAdapter.GetXSpecVolumes
- Update all mock implementations in tests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…esource

- Remove BuildResource method from SubResourceAdapter interface
- Remove SubResourcePrefixGetter optional interface
- Add SubResourceDecorator optional interface with DecorateResource method
- Move resource creation logic from adapters to control code
- Control code now sets: namespace, owner reference, labels (controlled-by, instance-id, template name/hash)
- Decorators can optionally customize resources (set Name, add custom labels)
- Add comment that Hash in SubResourceTemplate is computed by controller
- Add documentation about fields decorators MUST NOT modify

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep docs/superpowers/ as local files only.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add SubResourceTemplateLabelKey and SubResourceTemplateHashLabelKey
- Keep SubResourcePvcTemplateLabelKey as deprecated aliases
- Both map to same string values for backward compatibility
Replace PVC-specific label constants with generic ones
Allows adapters to register custom types with the scheme
Replace hardcoded switch statement with generic reflection-based extraction
- Replace hardcoded switch statements in newListForGVK/newObjectForGVK
- Use meta.EachListItem for extractListItems
- Update NewRealSubResourceControl to register types via SubResourceSchemeAdapter
- Update all callers to handle error returns
…erface

Required for PvcControl wrapper delegation
…ceControl

- Rewrite pvc_control.go as pvcControlWrapper that delegates to SubResourceControl
- Remove duplicate TemplateHash implementation (use utils.go version)
- Convert between []*corev1.PersistentVolumeClaim and []SubResourceState
- Update tests to use new SubResourceState with GVK field
- Add AdoptSingleResource to SubResourceControl interface for wrapper

This completes the consolidation of PVC-specific logic into the generic
SubResourceControl framework. PvcControl now provides backward compatibility
by wrapping the generic implementation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The scheme is managed by the controller-runtime manager and passed via
mixin.Scheme. Standard Kubernetes types (PVC, Service, ConfigMap) are
already registered, and custom types are registered by the controller
using kube-xset when setting up the manager.

SubResourceSchemeAdapter was unnecessary complexity - adapters don't
need to register their own types.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…to single class

Merge pvc_adapter.go into pvc_control.go and simplify to a single
PvcSubResourceAdapter class that implements SubResourceAdapter.
Remove the unused PvcControl interface and RealPvcControl wrapper
that were not being used anywhere in the codebase.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PvcSubResourceAdapter is only used internally by BuildAdapters() for
auto-bridging legacy SubResourcePvcAdapter. Move it into getter.go
to keep related code together and simplify the package structure.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
types.go contained NameTruncator and LabelManager utility types.
Merge into utils.go to consolidate all utility code in one file.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown

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

This PR refactors kube-xset’s subresource handling from a PVC-specific implementation to a generic, adapter-driven SubResourceControl, and adds controller-configurable target name prefixes.

Changes:

  • Replace legacy PVC-only subresource management with a generic SubResourceControl driven by SubResourceAdapter implementations (with bridging support for legacy SubResourcePvcAdapter).
  • Update sync/scale/update/replace flows to operate on generic subresources (ExistingSubResources, template-change detection, orphan/adopt/reclaim).
  • Add TargetPrefixGetter support and update GetTargetsPrefix behavior + tests.

Reviewed changes

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

Show a summary per file
File Description
xset_controller.go Wires SubResourceControl into controller setup and deletion-time reclaim flow.
synccontrols/x_utils.go Adds controller-provided target prefix override; adjusts generateName behavior.
synccontrols/x_utils_test.go Unit tests for updated GetTargetsPrefix signature/behavior.
synccontrols/x_update.go Switches update checks from PVC template change to generic subresource template change.
synccontrols/x_scale.go Replaces PVC orphan/adopt logic with SubResourceControl include/exclude handling.
synccontrols/x_replace.go Replaces PVC provisioning with generic subresource creation for replace-created targets.
synccontrols/types.go Changes SyncContext to store ExistingSubResources and renames update-change flag.
synccontrols/sync_control.go Integrates subresource listing/adoption/cleanup, creation on scale-out, deletion on scale-in, and template-change update hooks; refactors BatchDeleteTargetsByLabel.
subresources/utils.go Introduces shared helpers (template hashing, key formatting, truncation, label helpers).
subresources/utils_test.go Adds unit tests for new utility helpers.
subresources/subresource_control.go New generic subresource controller implementation (list/adopt/create/delete/orphan/check).
subresources/subresource_control_test.go Adds initial tests/skeletons for subresource control.
subresources/getter.go Adds adapter discovery + legacy PVC adapter bridge (BuildAdapters, PvcSubResourceAdapter).
subresources/getter_test.go Tests adapter discovery/bridging logic.
subresources/pvc_control.go Removes the legacy PVC-only control implementation.
api/xset_controller_types.go Adds SubResourceAdapterGetter and TargetPrefixGetter optional interfaces.
api/well_knowns.go Adds generic subresource template label keys (with PVC label backward compatibility).
api/well_knowns_test.go Tests label alias/backward compatibility behavior.
api/subresource_types.go Introduces SubResourceAdapter, SubResourceDecorator, and SubResourceTemplate API types.
CLAUDE.md Adds project guide documentation describing the new architecture and usage patterns.
.gitignore Adds ignores for worktrees and local planning artifacts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread synccontrols/x_utils.go Outdated
Comment on lines +109 to +113
func GetTargetsPrefix(override, controllerName string) string {
if override != "" {
return override
}
return prefix
return fmt.Sprintf("%s-", controllerName)
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

GetTargetsPrefix no longer validates whether the default "{controllerName}-" prefix is a valid DNS subdomain (or within length limits). If controllerName is already at the max length, adding "-" can make GenerateName invalid and cause target creation to fail. Consider restoring the previous validation/fallback behavior (or truncating safely) for the default case; keep override behavior as-is if you intentionally delegate validation to the controller.

Copilot uses AI. Check for mistakes.
Comment thread synccontrols/sync_control.go Outdated
Comment on lines +228 to +229
// delete unused subresources
if r.subResourceControl != nil {
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

SyncTargets now deletes "unused" subresources for every active target. With the new generic deletion logic, this can delete subresources that are still referenced by the running target (e.g., PVC template removed from XSet spec but the Pod still mounts it until the Pod is recreated), which can break workloads. Defer deletion until the target is being deleted/recreated, or make DeleteTargetUnusedResources verify the subresource is not currently in use by the target before deleting.

Suggested change
// delete unused subresources
if r.subResourceControl != nil {
// delete unused subresources only when the target is already being removed/recreated.
// Active targets may still reference subresources that are no longer present in the
// latest spec until the target is actually recreated.
if r.subResourceControl != nil && (target.GetDeletionTimestamp() != nil || targetDuringReplace(r.xsetLabelAnnoMgr, target)) {

Copilot uses AI. Check for mistakes.
Comment thread synccontrols/sync_control.go Outdated
// BatchDeleteTargetsByLabel try to trigger target deletion by to-delete label
// BatchDeleteTargetsByLabel triggers target deletion following the same lifecycle pattern as scale-in.
// It triggers TargetOpsLifecycle, waits for permission, then directly deletes the targets.
// Note: PVC cleanup is handled separately by ensureReclaimPvcs in xset_controller.go.
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The doc comment for BatchDeleteTargetsByLabel still says PVC cleanup is handled by ensureReclaimPvcs in xset_controller.go, but that function was removed and cleanup is now routed through SubResourceControl/ReclaimSubResourcesOnDeletion. Please update the comment to reflect the current deletion/cleanup flow to avoid misleading future changes.

Suggested change
// Note: PVC cleanup is handled separately by ensureReclaimPvcs in xset_controller.go.
// Any deletion-related subresource cleanup (including PVC reclamation) is handled separately through
// SubResourceControl/ReclaimSubResourcesOnDeletion rather than in this method.

Copilot uses AI. Check for mistakes.
Comment thread subresources/subresource_control.go Outdated
gvk := adapter.Meta()
obj, err := scheme.New(gvk)
if err != nil {
continue // Skip unknown GVKs
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

setUpCacheForAdapters silently skips adapters whose GVK isn't registered in the scheme (scheme.New error). That can lead to partially-enabled subresource management with hard-to-debug behavior (e.g., resources created but never listed/adopted/deleted). Consider returning an explicit error (or at least logging) when an adapter's GVK isn't recognized so controller authors know they must add the type to the scheme.

Suggested change
continue // Skip unknown GVKs
return fmt.Errorf("failed to set up cache for adapter GVK %s: type is not registered in the scheme; ensure this type is added to the controller scheme: %w", gvk, err)

Copilot uses AI. Check for mistakes.
Comment thread subresources/subresource_control.go Outdated
Comment on lines +440 to +444
// Remove finalizers before deleting to ensure immediate removal from etcd.
// Without this, resources with finalizers (e.g., PVCs with kubernetes.io/pvc-protection)
// would only get DeletionTimestamp set but remain in etcd until the finalizer controller runs.
if len(resource.GetFinalizers()) > 0 {
patch := client.MergeFrom(resource.DeepCopyObject().(client.Object))
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

deleteResource forcefully removes all finalizers before deleting the subresource. This is risky for resources that rely on finalizers for safety (e.g., PVC protection) and can cause premature deletion/data loss or violate Kubernetes controller contracts. Prefer issuing a normal delete and requeueing while waiting for finalizers/controllers to complete, or make finalizer removal narrowly scoped/opt-in per adapter with clear safety guarantees.

Copilot uses AI. Check for mistakes.
Comment on lines +689 to +697
// Delete unclaimed old resources (not in templates)
for templateName, state := range oldResources {
// If resource template is still in use, keep it
if templateNames[templateName] {
continue
}

if err := sc.deleteResource(ctx, xset, state.Object, state.GVK); err != nil {
return fmt.Errorf("failed to delete unclaimed %s %s: %w", gvk.Kind, state.Object.GetName(), err)
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

DeleteTargetUnusedResources deletes "unclaimed" old resources solely based on template presence. For PVCs (and likely other subresources), a template can be removed from the XSet spec while existing targets still reference/mount that resource until they are recreated/updated. Deleting here can remove in-use resources. Consider checking whether the target still references the subresource (adapter-provided usage check) before deletion, or only performing this cleanup when the target is terminating.

Suggested change
// Delete unclaimed old resources (not in templates)
for templateName, state := range oldResources {
// If resource template is still in use, keep it
if templateNames[templateName] {
continue
}
if err := sc.deleteResource(ctx, xset, state.Object, state.GVK); err != nil {
return fmt.Errorf("failed to delete unclaimed %s %s: %w", gvk.Kind, state.Object.GetName(), err)
// Delete old resources whose templates were removed only when the target is terminating.
// A template may be removed from the XSet while an existing target still references the
// previously created subresource until that target is recreated or updated.
if target.GetDeletionTimestamp() != nil {
for templateName, state := range oldResources {
// If resource template is still in use, keep it
if templateNames[templateName] {
continue
}
if err := sc.deleteResource(ctx, xset, state.Object, state.GVK); err != nil {
return fmt.Errorf("failed to delete unclaimed %s %s: %w", gvk.Kind, state.Object.GetName(), err)
}

Copilot uses AI. Check for mistakes.
Comment on lines +106 to +112
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Note: Full integration test requires a fake client with index support.
// This test documents the expected interface behavior.
// The actual functionality is tested in the suite test.
})
}
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Several tests here define table cases but the t.Run bodies are empty and make no assertions, so they add no coverage and can be misleading. Either implement meaningful unit tests (e.g., with a fake client + index/selector behavior) or remove these placeholder tests until they’re ready.

Copilot uses AI. Check for mistakes.
AnnaYue and others added 4 commits April 23, 2026 13:13
- Use HaveLen instead of len() == in test assertions
- Use indexing instead of range value copy for large structs
- Combine consecutive append calls
- Add nolint comment for unavoidable rangeValCopy when building slice from map

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1. synccontrols/x_utils.go: Restore DNS validation in GetTargetsPrefix
   - Re-add validation for controllerName prefix to ensure valid DNS subdomain

2. synccontrols/sync_control.go: Fix unused subresource deletion
   - Only delete unused subresources when target is being deleted or replaced
   - Active targets may still reference subresources no longer in spec
   - Update BatchDeleteTargetsByLabel doc comment to reflect current cleanup flow

3. subresource_control.go: Multiple safety improvements
   - setUpCacheForAdapters: Return explicit error when GVK not in scheme
   - deleteResource: Add TODO comment about finalizer removal trade-offs
   - DeleteTargetUnusedResources: Add IMPORTANT doc comment about when to call

4. subresource_control_test.go: Remove empty placeholder tests
   - Remove table-driven tests with no assertions

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Removing all finalizers bypasses safety mechanisms like
kubernetes.io/pvc-protection which prevents PVC deletion while mounted.

Changes:
- deleteResource: Issue normal delete, let Kubernetes handle finalizers
- CreateTargetResources: Wait for dying resources naturally instead of
  forcefully removing their finalizers

Resources with finalizers will get a deletion timestamp and be deleted
when their finalizers are cleared by their respective controllers.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove unused 'target' parameter from deleteUnusedResourcesForAdapter
- Remove unused 'target' parameter from isAdapterTemplateChanged
- Fix appendAssign warning in sync_control.go

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@AnnaYue AnnaYue merged commit b75e174 into main Apr 23, 2026
4 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 23, 2026
@AnnaYue AnnaYue deleted the feature/generic-subresource branch April 23, 2026 06:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants