diff --git a/.agents/skills/add-dependency/SKILL.md b/.agents/skills/add-dependency/SKILL.md index f657ab8f7..9f4d64b84 100644 --- a/.agents/skills/add-dependency/SKILL.md +++ b/.agents/skills/add-dependency/SKILL.md @@ -19,7 +19,7 @@ Use a dependency when your controller needs to: ## Key Principles -See also "Dependency Timing" in @.agents/skills/new-controller/patterns.md +See also "Dependency Timing" in [patterns.md](../new-controller/patterns.md) ### 1. Resolve Dependencies Late @@ -156,6 +156,8 @@ func (c myReconcilerConstructor) SetupWithManager(ctx context.Context, mgr ctrl. In `actuator.go`, resolve the dependency before using it: +Use `orcv1alpha1.IsAvailable` as the readiness predicate. This is sufficient because `Status.ID` is always set before a resource becomes Available: + ```go func (actuator myActuator) CreateResource(ctx context.Context, obj *orcv1alpha1.MyResource) (*osResourceT, progress.ReconcileStatus) { resource := obj.Spec.Resource @@ -164,9 +166,7 @@ func (actuator myActuator) CreateResource(ctx context.Context, obj *orcv1alpha1. if resource.ProjectRef != nil { project, reconcileStatus := projectDependency.GetDependency( ctx, actuator.k8sClient, obj, - func(dep *orcv1alpha1.Project) bool { - return orcv1alpha1.IsAvailable(dep) && dep.Status.ID != nil - }, + orcv1alpha1.IsAvailable, ) if needsReschedule, _ := reconcileStatus.NeedsReschedule(); needsReschedule { return nil, reconcileStatus @@ -189,9 +189,7 @@ func (actuator myActuator) ListOSResourcesForImport(ctx context.Context, obj orc project, rs := dependency.FetchDependency( ctx, actuator.k8sClient, obj.Namespace, filter.ProjectRef, "Project", - func(dep *orcv1alpha1.Project) bool { - return orcv1alpha1.IsAvailable(dep) && dep.Status.ID != nil - }, + orcv1alpha1.IsAvailable, ) reconcileStatus = reconcileStatus.WithReconcileStatus(rs) @@ -235,7 +233,7 @@ Create dependency tests in `internal/controllers//tests/-dependency/ - Test that resource waits for dependency - Test that dependency deletion is blocked (if using DeletionGuard) -Follow @.agents/skills/testing/SKILL.md for running unit tests, linting, and E2E tests. +Follow [testing](../testing/SKILL.md) for running unit tests, linting, and E2E tests. ## Checklist diff --git a/.agents/skills/new-controller/SKILL.md b/.agents/skills/new-controller/SKILL.md index af8ef4be7..c327c3fe4 100644 --- a/.agents/skills/new-controller/SKILL.md +++ b/.agents/skills/new-controller/SKILL.md @@ -201,9 +201,28 @@ Implement: - `ListOSResourcesForAdoption()` - Match by spec fields - `GetResourceReconcilers()` - (if resource supports updates) +**ReconcileResourceActuator is optional**: The generic reconciler detects it via type assertion at runtime — there is no factory method to implement. To opt in, add the `reconcileResourceActuator` type alias and interface assertion in `actuator.go`, then implement `GetResourceReconcilers` on the actuator struct: + +```go +type ( + reconcileResourceActuator = interfaces.ReconcileResourceActuator[orcObjectPT, osResourceT] + resourceReconciler = interfaces.ResourceReconciler[orcObjectPT, osResourceT] +) + +var _ reconcileResourceActuator = myActuator{} + +func (actuator myActuator) GetResourceReconcilers(ctx context.Context, orcObject orcObjectPT, osResource *osResourceT, controller interfaces.ResourceController) ([]resourceReconciler, progress.ReconcileStatus) { + return []resourceReconciler{ + actuator.updateResource, + }, nil +} +``` + +If the resource is fully immutable (no mutable fields, no tags, no sub-resources), skip this entirely — the generic reconciler will not call it. + ### Implementation Patterns -Follow the patterns in @.agents/skills/new-controller/patterns.md when implementing the actuator and API types. +Follow the patterns in [patterns.md](patterns.md) when implementing the actuator and API types. ### Status Writer (internal/controllers//status.go) @@ -217,7 +236,7 @@ Implement: Complete the scaffolded API validation test in `test/apivalidations/_test.go` by adding tests for any resource-specific validations (enums, numeric ranges, tag uniqueness, format validation, cross-field rules). Look for `TODO(scaffolding)` markers in the generated file. -Complete the E2E test stubs in `internal/controllers//tests/` and run tests following @.agents/skills/testing/SKILL.md +Complete the E2E test stubs in `internal/controllers//tests/` and run tests following [testing](../testing/SKILL.md) ## Checklist diff --git a/.agents/skills/new-controller/patterns.md b/.agents/skills/new-controller/patterns.md index e26c60945..b96bfe983 100644 --- a/.agents/skills/new-controller/patterns.md +++ b/.agents/skills/new-controller/patterns.md @@ -85,49 +85,17 @@ Distinguish between errors that can be retried vs those requiring user action. | **Retryable** (default) | Transient issues (network, API unavailable) | Automatic retry with backoff | | **Terminal** | Invalid configuration, bad input, permission denied | No retry until spec changes | -```go -// Terminal on create: User must fix the spec -if err != nil { - if !orcerrors.IsRetryable(err) { - err = orcerrors.Terminal(orcv1alpha1.ConditionReasonInvalidConfiguration, - "invalid configuration creating resource: "+err.Error(), err) - } - return nil, progress.WrapError(err) -} - -// Terminal on update: Treat as terminal (spec likely conflicts with existing state) -if err != nil { - if !orcerrors.IsRetryable(err) { - err = orcerrors.Terminal(orcv1alpha1.ConditionReasonInvalidConfiguration, - "invalid configuration updating resource: "+err.Error(), err) - } - return progress.WrapError(err) -} -``` +Use `orcerrors.IsRetryable(err)` to check; wrap non-retryable errors with `orcerrors.Terminal()`. See AGENTS.md "Error Classification" for the code pattern. ## 5. Dependency Timing -Resolve dependencies as late as possible, as close to the point of use as possible. - -**Rationale**: Avoid injecting dependency requirements where not strictly required. This reduces coupling and gives users greater flexibility when fixing failed deployments. +Resolve dependencies as late as possible, as close to the point of use as possible. Only fetch a dependency when you actually need its ID for the current operation. -**Examples:** - A Subnet depends on Network for creation, but not for import by ID or deletion - Don't require recreating a deleted Network just to delete a Subnet whose `status.ID` is already set -- Add finalizers to dependencies only immediately before the OpenStack create/update call that references them - -```go -// Good: Only fetch dependency when needed for creation -if resource.VipSubnetRef != nil { - subnet, depRS := subnetDependency.GetDependency(ctx, ...) - reconcileStatus = reconcileStatus.WithReconcileStatus(depRS) -} - -// Bad: Fetching dependency unconditionally even when not needed -subnet, depRS := subnetDependency.GetDependency(ctx, ...) // Wrong if subnet is optional -``` +- Only fetch optional dependencies conditionally (`if resource.SubnetRef != nil`) -For detailed dependency implementation: @.agents/skills/add-dependency/SKILL.md +For detailed implementation: [add-dependency](../add-dependency/SKILL.md) ## 6. Code Clarity diff --git a/.agents/skills/proposal/SKILL.md b/.agents/skills/proposal/SKILL.md index 212fadd35..b735c31a6 100644 --- a/.agents/skills/proposal/SKILL.md +++ b/.agents/skills/proposal/SKILL.md @@ -91,45 +91,12 @@ Before writing a proposal, ask the user about: ## Research Phase -Before writing the proposal, research relevant areas based on the enhancement type: +Before writing the proposal, research the relevant area: -### For Controller Enhancements - -1. **OpenStack API** - - Read the OpenStack API documentation for the resource - - Identify required vs optional fields - - Understand resource lifecycle (creation, updates, deletion) - - Check for async operations (polling requirements) - -2. **Gophercloud Support** - - Check if gophercloud has client support for this resource - - Identify the module path and types - - Note any missing functionality that needs upstream work - -3. **Existing Patterns** - - Look at similar controllers in ORC for patterns to follow - - Identify if existing utilities can be reused - - Check if new generic functionality is needed - -4. **Dependencies** - - Map out all ORC resource dependencies - - Determine which are required vs optional - - Identify deletion guard requirements - -### For Infrastructure Enhancements (metrics, webhooks, etc.) - -1. **Current Implementation** - - Check existing code for related functionality (e.g., `cmd/manager/`, `internal/`) - - Identify current ports, endpoints, and configurations - - Verify technical details by reading the actual code - -2. **Framework Capabilities** - - Check controller-runtime documentation for built-in features - - Identify what's provided vs what needs custom implementation - -3. **Integration Points** - - How does this integrate with existing infrastructure? - - What configuration already exists that this should use? +- **OpenStack API & gophercloud**: Read the API docs, check gophercloud support, note async operations +- **Existing ORC patterns**: Look at similar controllers or infrastructure code for patterns to follow +- **Dependencies**: Map ORC resource dependencies (required vs optional, deletion guards) +- **Current implementation**: For infrastructure enhancements, read the actual code to verify technical details (ports, endpoints, framework capabilities) ## Filling Out the Template @@ -194,33 +161,9 @@ Address each of these in the **Risks and Edge Cases** section: | **OpenStack compatibility** | Does this work across different OpenStack versions? (N/A for K8s-only) | | **Interaction with existing features** | Could this conflict with existing behavior? | -### Verification Before Submission - -Before finalizing the proposal: - -1. **Internal consistency**: Verify anything referenced in one section is defined elsewhere - - If you mention a metric/API/config in mitigations, ensure it's defined in Proposal - - If you reference a flag, show its usage - -2. **Technical accuracy**: Verify details against actual code - - Check ports, endpoints, paths in the codebase - - Verify framework capabilities match what you describe - -3. **Completeness**: Ensure examples are complete and correct - - Code examples should compile conceptually - - Config examples should be valid YAML/JSON - -## Tips for Writing Good Enhancements - -1. **Be concise but complete** - Include enough detail for reviewers to understand the proposal without unnecessary verbosity. - -2. **Focus on the "why"** - Motivation is often more important than implementation details. - -3. **Think about edge cases** - The Risks and Edge Cases section is where you demonstrate you've thought through the implications. - -4. **Consider alternatives** - Showing that you've evaluated other approaches strengthens your proposal. +### Before Submitting -5. **Keep it updated** - As implementation progresses, update the Implementation History section. +Verify internal consistency (anything referenced in one section is defined elsewhere), technical accuracy (check against actual code), and that examples are complete. ## Submission Process diff --git a/.agents/skills/review/SKILL.md b/.agents/skills/review/SKILL.md new file mode 100644 index 000000000..fc235d3c8 --- /dev/null +++ b/.agents/skills/review/SKILL.md @@ -0,0 +1,320 @@ +--- +name: review +description: Review ORC controller code for Kubernetes best practices and ORC conventions. Use after implementing or modifying a controller. +disable-model-invocation: true +--- + +# ORC Code Review Guide + +Review ORC controller code for correctness, Kubernetes best practices, and ORC conventions. Produce a structured report at the end. + +## Step 1: Identify Review Scope + +Determine what changed and which checklists apply: + +1. Run `git diff` (or `git diff --cached`, or diff against the base branch) to identify modified files. +2. Categorize changes by file type: + - `api/v1alpha1/*_types.go` -> API Types checklist + - `internal/controllers/*/controller.go` -> Controller Setup checklist + - `internal/controllers/*/actuator.go` -> Actuator Logic checklist + - `internal/controllers/*/status.go` -> Status Writer checklist + - `internal/controllers/*/tests/` -> Test Coverage checklist + - Any `.go` file -> Code Style checklist +3. Always apply the Kubernetes Best Practices checklist. +4. Read every changed file in full before reviewing. Also read surrounding context (e.g., the full `*_types.go` for the resource, even if only part changed). + +## Step 2: API Types (`*_types.go`) + +Review any `api/v1alpha1/*_types.go` file against these rules: + +### Structure + +- [ ] Three hand-written types exist: `ResourceSpec`, `Filter`, `ResourceStatus`. +- [ ] Top-level types (``, `Spec`, `Status`, `List`) are code-generated in `zz_generated.*` files and NOT hand-edited. +- [ ] `ResourceSpec` contains fields mapping to OpenStack create API parameters. +- [ ] `Filter` contains a subset of identifying fields, all optional pointers. +- [ ] `ResourceStatus` contains observed state from OpenStack. + +### Validation Markers + +- [ ] String fields use `+kubebuilder:validation:MinLength` / `MaxLength` constraints. +- [ ] Numeric fields use `+kubebuilder:validation:Minimum` / `Maximum` where appropriate. +- [ ] Enum types use `+kubebuilder:validation:Enum` listing all valid values. +- [ ] Filter structs have `+kubebuilder:validation:MinProperties:=1` (at least one criterion required). +- [ ] Slice fields have `+listType` annotations (`set` for unique items like tags, `atomic` for ordered/opaque lists, `map` with `+listMapKey` for keyed lists like conditions). + +### Immutability + +- [ ] Fully immutable resources (rare, e.g., ServerGroup) apply `+kubebuilder:validation:XValidation:rule="self == oldSelf"` at the struct level. +- [ ] Partially mutable resources apply `rule="self == oldSelf"` on individual immutable fields, leaving mutable fields unmarked. +- [ ] Immutability validation messages are descriptive (e.g., `"imageRef is immutable"`). + +### Field Conventions + +- [ ] `+required` fields use value types (e.g., `RAM int32`) but still have `json:"...,omitempty"`. +- [ ] `+optional` fields use pointer types (e.g., `*OpenStackName`, `*bool`) to distinguish "not set" from zero. +- [ ] In `ResourceStatus`, fields are `+optional` with pointers or plain strings with `omitempty`. +- [ ] Status string fields use `string` with `+kubebuilder:validation:MaxLength=1024`, not the strongly-typed wrapper. + +### Shared Types + +- [ ] Resource names use `OpenStackName` (not raw `string`). Check the specific OpenStack project for the correct max length (Keystone: 64 chars, Neutron: 255 chars, etc.). +- [ ] References to other ORC objects use `KubernetesNameRef` with a `Ref` suffix (e.g., `projectRef`, `networkRef`). +- [ ] References NEVER point to raw OpenStack resource IDs. OpenStack IDs appear only in status. +- [ ] IP addresses use `IPvAny`, CIDRs use `CIDR`, MACs use `MAC`. +- [ ] Neutron resources use shared types: `NeutronDescription`, `NeutronTag`, `FilterByNeutronTags`, `NeutronStatusMetadata`. +- [ ] Non-Neutron resources define their own tag types with appropriate length constraints. + +### Sub-resources + +- [ ] Nested sub-resources have separate Spec and Status types (e.g., `SecurityGroupRule` vs `SecurityGroupRuleStatus`). +- [ ] Sub-resource Status types include an `ID` field when the sub-resource has its own OpenStack ID. +- [ ] Complex cross-field validation uses `XValidation` rules on the sub-resource struct. + +## Step 3: Controller Setup (`controller.go`) + +### Basic Setup + +- [ ] RBAC markers are present and minimal (only the verbs actually needed). +- [ ] Controller name is lowercase, may contain hyphens, and is unique across all controllers. +- [ ] `GetName()` returns the controller name constant. +- [ ] `SetupWithManager` follows the standard pattern: builder -> watches -> dependency registration -> reconciler creation -> complete. + +### Dependencies + +- [ ] Dependencies are declared as **package-level variables**, not inside functions. +- [ ] `DeletionGuardDependency` is used when deleting the dependency would either fail or cause the dependent to fail. +- [ ] Regular `Dependency` (no deletion guard) is used for import-only dependencies and cases where OpenStack allows the deletion. +- [ ] Each dependency has a descriptive name (e.g., `vipSubnetDependency` not `subnetDependency` when multiple subnet types exist). +- [ ] Field path strings in dependency declarations match the actual API field paths. +- [ ] Extraction functions correctly handle nil checks for optional references. + +### Watches + +- [ ] Each dependency has a corresponding `Watches` call in `SetupWithManager`. +- [ ] Watch handlers use `predicates.NewBecameAvailable` to avoid unnecessary reconciles. +- [ ] Credential dependency watch is always registered. +- [ ] All dependency registrations use `errors.Join` with `AddToManager`. + +## Step 4: Actuator Logic (`actuator.go`) + +### Structure + +- [ ] Type aliases defined at the top of the file for `osResourceT`, actuator interfaces, and `helperFactory`. +- [ ] Compile-time interface assertions present (`var _ createResourceActuator = myActuator{}`). +- [ ] OS client interface defined locally with only the methods the actuator needs. +- [ ] Actuator struct holds the OS client and optionally `k8sClient` (when dependencies are used). + +### Resource Name + +- [ ] `getResourceName` helper exists: returns `spec.resource.name` if set, otherwise falls back to the ORC object name. + +### GetOSResourceByID + +- [ ] Wraps errors with `progress.WrapError`. +- [ ] Handles "not found" correctly (returns `nil` resource, not an error). + +### ListOSResourcesForAdoption + +- [ ] Returns `false` (second return value) when `spec.resource` is nil (no spec to match against). +- [ ] Builds client-side filters matching the **full** resource spec for accurate adoption. + +### ListOSResourcesForImport + +- [ ] Builds filters from the import filter spec only. +- [ ] All filter fields are mapped. + +### CreateResource + +- [ ] Translates ORC spec into OpenStack `CreateOpts` completely. +- [ ] **MUST NOT** perform any action after the Create API call (idempotency requirement). +- [ ] Any actions before Create are idempotent (Create may be called many times). +- [ ] Non-retryable errors are wrapped with `orcerrors.Terminal`. +- [ ] Lists (tags, etc.) are sorted before passing to Create for deterministic state. +- [ ] Finalizers on dependencies are added immediately before the Create call, not earlier. + +### DeleteResource + +- [ ] **MUST NOT** perform any action after the Delete API call. +- [ ] Handles "not found" gracefully (resource already deleted). +- [ ] For resources with intermediate states: checks provisioning status before deleting, handles 409 Conflict by waiting. +- [ ] Minimal dependency requirements -- does not require dependencies that aren't strictly needed for deletion. + +### ReconcileResourceActuator (if implemented) + +- [ ] `GetResourceReconcilers` returns reconciler functions for post-creation tasks (e.g., setting Neutron tags, handling mutable field updates). +- [ ] Reconcilers that modify the OpenStack resource return a `progress.ProgressStatus` to force a status refresh. +- [ ] Reconcilers are independent and don't rely on side effects of other reconcilers. +- [ ] `updateResource` is used only for general mutable field updates via the resource's Update API (building `UpdateOpts`, single API call). Operations using a separate API have a descriptive name (e.g., `reconcileExtraSpecs`, `reconcileSubports`, `reconcilePassword`, `updateRules`). +- [ ] Single-concern reconcilers return `nil` (not a terminal error) when `spec.resource` is nil. Only `updateResource` returns a terminal error for nil `spec.resource`. +- [ ] `CreateResource` does not duplicate work that is handled by a reconciler. The `CreateResource` contract forbids actions that can fail after creating the primary resource. + +### Error Handling + +- [ ] All errors from OpenStack API calls are checked. +- [ ] Non-retryable errors (400, invalid config) wrapped with `orcerrors.Terminal` and an appropriate `ConditionReason`. +- [ ] Transient errors (5xx, network) left as default (automatic retry with backoff). +- [ ] `ReconcileStatus` return values are **never discarded** -- always assigned and propagated. +- [ ] When wrapping errors, use `progress.WrapError(err)` (not bare `fmt.Errorf`). + +### Dependency Resolution + +- [ ] Dependencies resolved **as late as possible**, close to the point of use. +- [ ] Dependencies not required for deletion unless strictly necessary (e.g., don't require Network to delete a Subnet with `status.ID` already set). +- [ ] Dependencies not required for import-by-ID. +- [ ] `GetDependency` results checked: if `needsReschedule` is true, return early. +- [ ] Readiness predicate uses `orcv1alpha1.IsAvailable` (the standard helper from `api/v1alpha1/conditions.go`). `Status.ID` is always set before a resource becomes Available, so checking `dep.Status.ID != nil` separately is unnecessary. + +## Step 5: Status Writer (`status.go`) + +### Structure + +- [ ] Type aliases for `objectApplyT` and `statusApplyT` (SSA apply configuration types). +- [ ] Compile-time interface assertion for `ResourceStatusWriter`. + +### ResourceAvailableStatus + +- [ ] Returns `ConditionTrue` only when the resource is completely ready for use. +- [ ] Returns `ConditionFalse` when `osResource` is nil and no `status.ID` exists (not yet created). +- [ ] Returns `ConditionUnknown` when `osResource` is nil but `status.ID` exists (can't verify current state). +- [ ] For resources with intermediate states (BUILD, PENDING_CREATE): returns `ConditionFalse` until the resource reaches a stable, usable state (e.g., ACTIVE). +- [ ] For resources in ERROR state: returns `ConditionFalse`. + +### ApplyResourceStatus + +- [ ] Maps **all** OpenStack resource fields to ORC status fields. +- [ ] Zero/empty values handled correctly: only include swap, ephemeral, description, etc., when non-zero/non-empty. +- [ ] Does NOT attempt to preserve previous status when the OpenStack resource can't be fetched (status.resource is cleared intentionally). +- [ ] Pointer fields in status use `ptr.To()` for conversion. + +## Step 6: Kubernetes Best Practices + +### Conditions + +- [ ] **Progressing=True** means status doesn't yet reflect spec AND controller expects more reconciles. +- [ ] **Progressing=False** means the object will NOT be reconciled again until the spec changes. This covers both success (Available=True) and terminal errors. +- [ ] **Available=True** means the resource is ready for use by consumers. +- [ ] Condition reasons use defined constants from `orcv1alpha1` (e.g., `ConditionReasonInvalidConfiguration`, `ConditionReasonTransientError`). +- [ ] Conditions are not set directly by the actuator -- the generic reconciler handles this based on `ReconcileStatus` and `ResourceStatusWriter` return values. + +### Finalizers + +- [ ] Finalizers on dependency objects are added only immediately before the OpenStack create/update call that references them (not during initialization). +- [ ] Deletion guard finalizers are managed by `DeletionGuardDependency` -- the controller doesn't manually add/remove them. +- [ ] The controller's own finalizer is managed by the generic reconciler framework. + +### Server-Side Apply + +- [ ] Status is written via SSA apply configurations (not direct status updates). +- [ ] `GetApplyConfig` returns a fresh apply configuration each time. +- [ ] Status is written in a single SSA transaction per reconcile. + +### Resource Safety + +- [ ] No cascade deletes unless the user explicitly requested them. +- [ ] No auto-correction of invalid states that might cause data loss. +- [ ] Prefer failing safely over making assumptions. + +## Step 7: Code Style + +See AGENTS.md for conventions (import ordering, logging levels, pointer handling). Only flag deviations that affect correctness: + +- [ ] Generated files (`zz_generated.*`) are not hand-edited. +- [ ] `make generate` has been run after any API type changes. +- [ ] Constants from gophercloud are preferred over locally defined string constants (e.g., `ports.StatusActive` instead of `"ACTIVE"`). + +## Step 8: Test Coverage + +### E2E Test Directories + +For each controller, verify the following test directories exist under `internal/controllers//tests/`: + +| Required Test | Purpose | +|---------------|---------| +| `-create-minimal/` | Create with only required fields, verify status matches | +| `-create-full/` | Create with all fields populated | +| `-import/` | Import an existing OpenStack resource | +| `-import-error/` | Import with no matches, verify error handling | +| `-dependency/` | Test dependency waiting and deletion guard protection | + +| Conditional Test | When Required | +|------------------|---------------| +| `-update/` | Resource has mutable fields | +| `-import-dependency/` | Import filter references other ORC objects | + +### E2E Test Quality + +- [ ] Each test directory has a `README.md` describing each step. +- [ ] Step files use zero-padded numeric prefixes (`00-`, `01-`, etc.). +- [ ] Cloud credentials secret created via `TestStep` command (not a manifest) using `E2E_KUTTL_OSCLOUDS`. +- [ ] Assertions verify `status.resource` fields match the spec. +- [ ] Conditions (`Available`, `Progressing`) are asserted with correct `status`, `reason`, and `message`. +- [ ] Dependency tests verify: (1) waiting state with `Progressing=True`, (2) availability after dep created, (3) finalizer blocks dep deletion, (4) dep deleted after resource deleted. +- [ ] Update tests use `kubectl replace` (not KUTTL patch) to test field removal. +- [ ] CEL expressions (`celExpr`) used for complex assertions (e.g., checking `deletionTimestamp`, finalizer membership, field absence with `!has(...)`). + +### Unit / API Validation Tests + +- [ ] API validation tests exist at `test/apivalidations/_test.go` for non-trivial validation rules. +- [ ] Unit tests cover any complex helper logic. + +## Step 9: Produce Review Report + +After running through all applicable checklists, produce a structured report: + +### Report Format + +``` +## Review Summary + +**Scope**: +**Overall**: + +## Blockers +Items that MUST be fixed before merge. These are correctness issues, violations +of idempotency/safety invariants, or missing required functionality. + +- [file:line] Description of the issue and why it's a blocker. + +## Warnings +Items that SHOULD be fixed. These are convention violations, missing edge case +handling, or patterns that may cause issues in production. + +- [file:line] Description and recommendation. + +## Suggestions +Items that COULD be improved. Style preferences, minor optimizations, or +additional test coverage that would be nice to have. + +- [file:line] Description and suggestion. + +## Positive Observations +Notable good practices observed in the code (keep brief, 2-3 items max). +``` + +### Severity Guidelines + +**Blocker** -- any of: +- Violates CreateResource/DeleteResource idempotency invariant (actions after the API call) +- Incorrect Progressing/Available condition semantics (could cause reconciliation to hang) +- Missing `ReconcileStatus` propagation (discarded return value) +- Terminal error not marked terminal (infinite retry of unfixable error) +- Missing finalizer or finalizer added too early +- Security issue (RBAC too broad, secrets leaked in logs) +- Data loss risk (cascade delete without explicit user intent) + +**Warning** -- any of: +- Missing validation markers on API types +- Dependency resolved too early (unnecessary coupling) +- Missing error wrapping (`progress.WrapError`) +- Incomplete status mapping (fields not reflected in status) +- Missing E2E test for a standard scenario +- Wrong logging level +- Missing interface assertion + +**Suggestion** -- any of: +- Import ordering +- Naming could be more descriptive +- Additional test coverage beyond the standard set +- Code could be simplified +- Comment could be clearer diff --git a/.agents/skills/testing/SKILL.md b/.agents/skills/testing/SKILL.md index 88919c1f7..317eb9998 100644 --- a/.agents/skills/testing/SKILL.md +++ b/.agents/skills/testing/SKILL.md @@ -20,9 +20,13 @@ make test ## E2E Test Prerequisites -E2E tests require `E2E_OSCLOUDS` environment variable pointing to a `clouds.yaml` file containing: -- A cloud named `openstack` - regular user credentials -- A cloud named `devstack-admin` - admin credentials +E2E tests require `E2E_OSCLOUDS` environment variable pointing to a `clouds.yaml` file containing cloud entries for regular and admin credentials. The cloud names are configurable via environment variables: + +| Variable | Description | Default | +|----------|-------------|--------| +| `E2E_OSCLOUDS` | Path to `clouds.yaml` | `/etc/openstack/clouds.yaml` | +| `E2E_OPENSTACK_CLOUD_NAME` | Cloud name for regular credentials | `devstack` | +| `E2E_OPENSTACK_ADMIN_CLOUD_NAME` | Cloud name for admin credentials | `devstack-admin-demo` | If the user did not provide `E2E_OSCLOUDS`, tell them local E2E testing will be skipped and they should run it manually later or in CI. diff --git a/.agents/skills/update-controller/SKILL.md b/.agents/skills/update-controller/SKILL.md index e7d59fe0a..f80d9601f 100644 --- a/.agents/skills/update-controller/SKILL.md +++ b/.agents/skills/update-controller/SKILL.md @@ -31,7 +31,7 @@ Research the resource before implementing changes: ## Key Principles -When updating controllers, follow the patterns in @.agents/skills/new-controller/patterns.md +When updating controllers, follow the patterns in [patterns.md](../new-controller/patterns.md) ## Common Update Scenarios @@ -81,27 +81,13 @@ When updating controllers, follow the patterns in @.agents/skills/new-controller 2. Implement `GetResourceReconcilers()` if not already present -3. Add update handling to the `updateResource()` reconciler (or create it if not present): - ```go - func (actuator myActuator) updateResource(...) progress.ReconcileStatus { - var updateOpts resources.UpdateOpts - // Add a handleXXXUpdate() call for each mutable field - handleMyFieldUpdate(&updateOpts, resource, osResource) - // Call API only if something changed - if updateOpts != (resources.UpdateOpts{}) { - _, err := actuator.osClient.UpdateResource(ctx, *obj.Status.ID, updateOpts) - // ... - } - } - - func handleMyFieldUpdate(updateOpts *resources.UpdateOpts, resource *resourceSpecT, osResource *osResourceT) { - if resource.MyField != nil && *resource.MyField != osResource.MyField { - updateOpts.MyField = resource.MyField - } - } - ``` +3. Add update handling to the `updateResource()` reconciler (or create it if not present). Follow the pattern in `internal/controllers/securitygroup/actuator.go` (or `trunk/actuator.go`): + - Build an `UpdateOpts` struct using `handleXXXUpdate()` helpers for each mutable field + - Use a `needsUpdate()` helper that serializes the opts to a map and checks `len() > 0` + - Call the Update API only if something changed, return `progress.NeedsRefresh()` + - Return terminal error if `spec.resource` is nil - **Note**: Only create a separate reconciler method if the field requires a different API call (e.g., tags on networking resources use a separate tags API). + **Note**: Only use `updateResource` when the field is updated via the resource's standard Update API. If the field requires a different API (e.g., extra specs, subports, tags on networking resources), create a separate single-concern reconciler instead. See [Adding a Single-Concern Reconciler](#adding-a-single-concern-reconciler) below. 4. Register in `GetResourceReconcilers()`: ```go @@ -110,9 +96,45 @@ When updating controllers, follow the patterns in @.agents/skills/new-controller }, nil ``` +### Adding a Single-Concern Reconciler + +When a mutable field uses a separate OpenStack API (not the resource's Update API), create a dedicated reconciler with a descriptive verb+noun name instead of adding logic to `updateResource`. + +**Examples**: `reconcileExtraSpecs` (flavor, volumetype), `reconcileSubports` (trunk), `reconcilePassword` (user), `updateRules` (securitygroup). + +Key differences from `updateResource`: + +- **Naming**: Use a descriptive name (e.g., `reconcileExtraSpecs`), not `updateResource`. +- **Nil guard**: Return `nil` when `spec.resource` is nil (not a terminal error). The terminal error pattern is reserved for `updateResource`. +- **Multiple API calls**: A single-concern reconciler may make multiple API calls (e.g., create some extra specs, delete others). This is an established pattern (see `reconcileSubports`, `updateRules`). +- **Idempotency**: Operations must be idempotent. If the reconciler fails partway through, the next reconciliation recomputes the diff from the current OpenStack state and retries only what's still needed. + +```go +func (actuator myActuator) reconcileExtraSpecs(ctx context.Context, obj orcObjectPT, osResource *osResourceT) progress.ReconcileStatus { + resource := obj.Spec.Resource + if resource == nil { + return nil // Not a terminal error (unlike updateResource) + } + + // Compute desired vs current diff + // Make API calls (creates, updates, deletes) + // Return progress.NeedsRefresh() if any changes were made +} +``` + +Register alongside other reconcilers in `GetResourceReconcilers()`: +```go +return []resourceReconciler{ + actuator.updateResource, // general field updates via Update API + actuator.reconcileExtraSpecs, // single-concern: separate API +}, nil +``` + +**Do NOT duplicate work in `CreateResource`**. If a reconciler handles a concern (e.g., extra specs), do not also set that data in `CreateResource`. The `CreateResource` contract forbids actions that can fail after creating the primary resource. The reconciler will handle it on the first reconciliation after creation. + ### Adding a Dependency -See @.agents/skills/add-dependency/SKILL.md for detailed steps. +See [add-dependency](../add-dependency/SKILL.md) for detailed steps. ### Improving DeleteResource @@ -156,43 +178,11 @@ func (actuator myActuator) DeleteResource(ctx context.Context, _ orcObjectPT, re Tags []string `json:"tags,omitempty"` ``` -2. Sort tags before creation (deterministic state): - ```go - tags := make([]string, len(resource.Tags)) - for i := range resource.Tags { - tags[i] = string(resource.Tags[i]) - } - slices.Sort(tags) - createOpts.Tags = tags - ``` - -3. Add tag update handler with sorting: - ```go - func handleTagsUpdate(updateOpts *resources.UpdateOpts, resource *resourceSpecT, osResource *osResourceT) { - desiredTags := make([]string, len(resource.Tags)) - for i := range resource.Tags { - desiredTags[i] = string(resource.Tags[i]) - } - slices.Sort(desiredTags) - - currentTags := make([]string, len(osResource.Tags)) - copy(currentTags, osResource.Tags) // Don't mutate original - slices.Sort(currentTags) - - if !slices.Equal(desiredTags, currentTags) { - updateOpts.Tags = &desiredTags - } - } - ``` +2. Sort tags before creation and comparison (use `slices.Sort` — see `patterns.md` §3 Deterministic State). -4. Register in `GetResourceReconcilers()`: - ```go - return []resourceReconciler{ - actuator.updateResource, // includes handleTagsUpdate - }, nil - ``` +3. Add a `handleTagsUpdate()` helper that sorts both desired and current tags, compares with `slices.Equal`, and sets `updateOpts.Tags` only if different. Copy before sorting to avoid mutating the original. -**Note**: Import `"slices"` for sorting/comparison functions. +4. Register `updateResource` (which calls `handleTagsUpdate`) in `GetResourceReconcilers()`. ### Adding Status Constants @@ -211,35 +201,15 @@ const ( ) ``` -See also `@.agents/skills/new-controller/patterns.md` for more details on this pattern. +See also [patterns.md](../new-controller/patterns.md) for more details on this pattern. ### Improving Error Handling -Ensure proper error classification: - -```go -// Terminal on create: Invalid configuration - user must fix spec -if err != nil { - if !orcerrors.IsRetryable(err) { - err = orcerrors.Terminal(orcv1alpha1.ConditionReasonInvalidConfiguration, - "invalid configuration creating resource: "+err.Error(), err) - } - return nil, progress.WrapError(err) -} - -// Terminal on update: -if err != nil { - if !orcerrors.IsRetryable(err) { - err = orcerrors.Terminal(orcv1alpha1.ConditionReasonInvalidConfiguration, - "invalid configuration updating resource: "+err.Error(), err) - } - return progress.WrapError(err) -} -``` +See [patterns.md](../new-controller/patterns.md) §4 Error Classification. Wrap non-retryable errors with `orcerrors.Terminal`; leave transient errors as-is for automatic retry. ## Testing Changes -Follow @.agents/skills/testing/SKILL.md for running unit tests, linting, and E2E tests. +Follow [testing](../testing/SKILL.md) for running unit tests, linting, and E2E tests. ## Checklist diff --git a/AGENTS.md b/AGENTS.md index 7795e08f0..e91759dda 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,11 +21,21 @@ openstack-resource-controller/ │ │ ├── status.go # Status writer implementation │ │ ├── zz_generated.*.go # Generated code (DO NOT EDIT) │ │ └── tests/ # KUTTL E2E tests +│ ├── logging/ # Log level constants │ ├── osclients/ # OpenStack API client wrappers │ ├── scope/ # Cloud credentials & client factory -│ └── util/ # Utilities (errors, dependency, tags) +│ └── util/ +│ ├── applyconfigs/ # SSA apply config helpers +│ ├── credentials/ # Credential watch & dependency setup +│ ├── dependency/ # Dependency framework +│ ├── errors/ # Error classification (Terminal, IsRetryable) +│ ├── finalizers/ # Finalizer helpers +│ ├── result/ # Result helpers +│ ├── strings/ # Finalizer/field-owner name generation +│ └── tags/ # Tag reconciliation utilities ├── cmd/ │ ├── manager/ # Main entry point +│ ├── models-schema/ # OpenAPI schema generation │ ├── resource-generator/ # Code generation │ └── scaffold-controller/ # New controller scaffolding └── website/docs/development/ # Detailed documentation @@ -44,7 +54,7 @@ All controllers use a generic reconciler that handles the reconciliation loop. C ### Key Interfaces -Controllers implement these methods (see `internal/controllers/flavor/` for a simple example): +Controllers implement these methods (see `internal/controllers/servergroup/` for a simple example): ```go // Required by all actuators @@ -60,7 +70,7 @@ CreateResource(ctx, orcObject) (*osResource, ReconcileStatus) DeleteResource(ctx, orcObject, osResource) ReconcileStatus // Optional - for updates after creation -GetResourceReconcilers(ctx, obj, osResource) ([]ResourceReconciler, error) +GetResourceReconcilers(ctx, obj, osResource, controller) ([]ResourceReconciler, ReconcileStatus) ``` ### Two Critical Conditions @@ -79,11 +89,17 @@ Every ORC object has these conditions: Methods return `ReconcileStatus` instead of `error`: +`ReconcileStatus` is a type alias for a pointer (`type ReconcileStatus = *reconcileStatus`). `nil` is a valid value meaning "success, no reschedule", and all methods are safe to call on a nil receiver. + ```go -nil // Success, no reschedule -progress.WrapError(err) // Wrap error for handling -reconcileStatus.WithRequeue(5*time.Second) // Schedule reconcile after delay -reconcileStatus.WithProgressMessage("waiting...") // Add progress message +nil // Success, no reschedule +progress.WrapError(err) // Wrap error for handling +reconcileStatus.WithRequeue(5*time.Second) // Schedule reconcile after delay +reconcileStatus.WithProgressMessage("...") // Add progress message +progress.NeedsRefresh() // Immediate re-reconcile to refresh status after mutation +progress.WaitingOnOpenStack(progress.WaitingOnReady, 15*time.Second) // Poll for OpenStack state change +progress.WaitingOnObject("Network", name, progress.WaitingOnCreation) // Wait for a k8s object +reconcileStatus.WithReconcileStatus(other) // Merge two ReconcileStatuses ``` ### Error Classification @@ -142,14 +158,49 @@ if needsReschedule, _ := reconcileStatus.NeedsReschedule(); needsReschedule { projectID := ptr.Deref(project.Status.ID, "") ``` +### Lightweight Dependency Lookup (FetchDependency) + +For one-off lookups that don't need finalizers (e.g., resolving refs in `ListOSResourcesForAdoption` or import filters), use `dependency.FetchDependency` instead of a declared dependency: + +```go +import "github.com/k-orc/openstack-resource-controller/v2/internal/util/dependency" + +project, rs := dependency.FetchDependency( + ctx, actuator.k8sClient, obj.Namespace, filter.ProjectRef, "Project", + func(dep *orcv1alpha1.Project) bool { + return orcv1alpha1.IsAvailable(dep) && dep.Status.ID != nil + }, +) +reconcileStatus = reconcileStatus.WithReconcileStatus(rs) +``` + +### Credentials Dependency (generated) + +Every controller has a `credentialsDependency` auto-generated in `zz_generated.controller.go`. It is a `DeletionGuardDependency` on `corev1.Secret` that ensures the cloud credentials secret exists and carries the controller's finalizer. It is checked in `newActuator()` before creating an OpenStack client: + +```go +_, reconcileStatus := credentialsDependency.GetDependencies( + ctx, controller.GetK8sClient(), orcObject, + func(*corev1.Secret) bool { return true }, +) +if needsReschedule, _ := reconcileStatus.NeedsReschedule(); needsReschedule { + return myActuator{}, reconcileStatus +} +``` + +The credential watch is registered in `SetupWithManager` via `credentials.AddCredentialsWatch()`. + ## Common Patterns -### Resource Name Helper +### Resource Name Helper (generated) + +`getResourceName` is auto-generated in `zz_generated.adapter.go` — do not write it manually. It returns `spec.resource.name` if set, otherwise falls back to the ORC object's Kubernetes name: ```go -func getResourceName(orcObject *orcv1alpha1.Flavor) string { +// In zz_generated.adapter.go (DO NOT EDIT) +func getResourceName(orcObject orcObjectPT) string { if orcObject.Spec.Resource.Name != nil { - return *orcObject.Spec.Resource.Name + return string(*orcObject.Spec.Resource.Name) } return orcObject.Name } @@ -173,6 +224,59 @@ var _ createResourceActuator = flavorActuator{} var _ deleteResourceActuator = flavorActuator{} ``` +### Actuator Factory (newActuator) + +Every controller defines a `newActuator()` function that resolves credentials, creates the OpenStack client scope, and returns the actuator. This is called by the `helperFactory` methods `NewCreateActuator` and `NewDeleteActuator`: + +```go +func newActuator(ctx context.Context, orcObject *orcv1alpha1.Flavor, controller generic.ResourceController) (flavorActuator, progress.ReconcileStatus) { + log := ctrl.LoggerFrom(ctx) + + // Ensure credential secrets exist and have our finalizer + _, reconcileStatus := credentialsDependency.GetDependencies( + ctx, controller.GetK8sClient(), orcObject, + func(*corev1.Secret) bool { return true }, + ) + if needsReschedule, _ := reconcileStatus.NeedsReschedule(); needsReschedule { + return flavorActuator{}, reconcileStatus + } + + clientScope, err := controller.GetScopeFactory().NewClientScopeFromObject( + ctx, controller.GetK8sClient(), log, orcObject, + ) + if err != nil { + return flavorActuator{}, progress.WrapError(err) + } + osClient, err := clientScope.NewComputeClient() // or NewNetworkClient, etc. + if err != nil { + return flavorActuator{}, progress.WrapError(err) + } + + return flavorActuator{osClient: osClient}, nil +} +``` + +### Tag Reconciliation (Neutron resources) + +Neutron resources use a separate tags API instead of the resource's Update API. The `internal/util/tags` package provides a reusable reconciler: + +```go +import "github.com/k-orc/openstack-resource-controller/v2/internal/util/tags" + +func (actuator myActuator) GetResourceReconcilers(...) ([]resourceReconciler, progress.ReconcileStatus) { + return []resourceReconciler{ + tags.ReconcileTags[orcObjectPT, osResourceT]( + orcObject.Spec.Resource.Tags, + osResource.Tags, + tags.NewNeutronTagReplacer(actuator.osClient, "security-groups", osResource.ID), + ), + actuator.updateRules, + }, nil +} +``` + +`ReconcileTags` computes the diff between desired and observed tags and replaces them atomically. For resources whose tags are set via the standard Update API (e.g., block storage), use a `handleTagsUpdate()` helper in `updateResource` instead. + ### Pointer Handling ```go @@ -202,9 +306,9 @@ type ServerResourceSpec struct { Tags []ServerTag `json:"tags,omitempty"` } -// Some resources are fully immutable (rare - e.g., Flavor, ServerGroup) -// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="FlavorResourceSpec is immutable" -type FlavorResourceSpec struct { +// Some resources are fully immutable (rare - e.g., ServerGroup) +// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ServerGroupResourceSpec is immutable" +type ServerGroupResourceSpec struct { // ... } ``` @@ -250,11 +354,53 @@ make test-e2e # Run KUTTL E2E tests (requires E2E_OSCLOUDS) make fmt # Format code ``` +## Reconciler Naming Conventions + +`GetResourceReconcilers` returns a list of reconciler functions. There are two types: + +1. **`updateResource`**: Handles general mutable field updates via the resource's Update API. Uses `handleXXXUpdate()` helpers to build an `UpdateOpts` struct, then makes a single API call. Returns a terminal error when `spec.resource` is nil. Examples: securitygroup, volumetype, trunk, router. + +2. **Single-concern reconcilers**: Handle a specific aspect of the resource using a separate API (not the resource's Update API). Named with a descriptive verb+noun (e.g., `reconcileExtraSpecs`, `reconcileSubports`, `reconcilePassword`, `updateRules`). Return `nil` (not a terminal error) when `spec.resource` is nil. May make multiple API calls within a single reconciler. + +```go +// updateResource pattern - general mutable fields via Update API +func (actuator myActuator) updateResource(ctx context.Context, obj orcObjectPT, osResource *osResourceT) progress.ReconcileStatus { + resource := obj.Spec.Resource + if resource == nil { + // Terminal error: updateResource is only registered for managed resources + return progress.WrapError( + orcerrors.Terminal(orcv1alpha1.ConditionReasonInvalidConfiguration, "Update requested, but spec.resource is not set")) + } + // ... build UpdateOpts, make single Update API call +} + +// Single-concern reconciler pattern - separate API +func (actuator myActuator) reconcileExtraSpecs(ctx context.Context, obj orcObjectPT, osResource *osResourceT) progress.ReconcileStatus { + resource := obj.Spec.Resource + if resource == nil { + return nil // Not a terminal error + } + // ... compute diff, make API calls (creates, deletes, etc.) +} +``` + +Both types are registered in `GetResourceReconcilers`: +```go +func (actuator myActuator) GetResourceReconcilers(ctx context.Context, orcObject orcObjectPT, osResource *osResourceT, controller generic.ResourceController) ([]resourceReconciler, progress.ReconcileStatus) { + return []resourceReconciler{ + actuator.updateResource, // general field updates + actuator.reconcileExtraSpecs, // single-concern reconciler + }, nil +} +``` + ## Reference Controllers -- **Simple**: `internal/controllers/flavor/` - No dependencies, immutable +- **Simple**: `internal/controllers/servergroup/` - No dependencies, fully immutable +- **Single-concern reconciler**: `internal/controllers/flavor/` - No dependencies, immutable except extra specs (`reconcileExtraSpecs`) - **With dependencies**: `internal/controllers/securitygroup/` - Project dependency, rules reconciliation -- **Complex**: `internal/controllers/server/` - Multiple dependencies, reconcilers +- **Multiple reconcilers**: `internal/controllers/trunk/` - `updateResource` + `reconcileSubports` + tags +- **Complex**: `internal/controllers/server/` - Multiple dependencies, many reconcilers ## Documentation