OCPCLOUD-3359: Add component names, manifestSubstitutions, and observedGeneration to CAPI revisions - #2786
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@mdbooth: This pull request references OCPCLOUD-3359 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @mdbooth! Some important instructions when contributing to openshift/api: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds manifest substitution support, component naming, and observed revision generation tracking to ClusterAPI types and CRD schemas. Introduces ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/XL
|
@mdbooth: This pull request references OCPCLOUD-3359 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/test verify-hypershift-integration |
|
PR-Agent: could not fine a component named |
|
/test verify-hypershift-integration |
1 similar comment
|
/test verify-hypershift-integration |
Generating tests for 'verify-hypershift-integration' ✏️️Work in progress ... |
|
/test verify-hypershift-integration |
|
PR-Agent: could not fine a component named |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd manifest substitutions, component names, and revision generation tracking
WalkthroughsDescription• Add manifestSubstitutions field to enable envsubst-style variable substitution in manifests • Add name field to components for improved readability and debugging • Add observedRevisionGeneration field to track ClusterAPI object generation observed by controller • Comprehensive validation rules for new fields with detailed error messages Diagramflowchart LR
A["ClusterAPIInstallerRevision"] -->|adds| B["manifestSubstitutions"]
C["ClusterAPIInstallerComponent"] -->|adds| D["name field"]
E["ClusterAPIStatus"] -->|adds| F["observedRevisionGeneration"]
B -->|contains| G["ClusterAPIInstallerRevisionManifestSubstitution"]
G -->|has| H["key and value"]
File Changes1. operator/v1alpha1/types_clusterapi.go
|
Code Review by Qodo
|
mdbooth
changed the title
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // name is the human-readable name of the component. It is only used to make | ||
| // api revisions easier to understand. It must consist of alphanumeric | ||
| // characters, or '-'. | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=255 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[A-Za-z0-9-]+$')",message="name must consist of alphanumeric characters or '-'" | ||
| Name string `json:"name,omitempty"` |
There was a problem hiding this comment.
2. Component name length undocumented 📘 Rule violation ⚙ Maintainability
ClusterAPIInstallerComponent.Name adds MinLength/MaxLength validation but its comment does not document the length constraints or what happens when the optional field is omitted. This violates the requirements for documenting validation markers and omitted behavior.
Agent Prompt
## Issue description
The `ClusterAPIInstallerComponent.Name` field comment does not document all validation constraints (min/max length) and does not explicitly state behavior when the optional field is omitted.
## Issue Context
Compliance requires field comments to include all constraints expressed via kubebuilder markers and to describe omitted/default behavior for optional fields.
## Fix Focus Areas
- operator/v1alpha1/types_clusterapi.go[214-221]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // ClusterAPIStatus describes the current state of the capi-operator. | ||
| // +kubebuilder:validation:XValidation:rule="self.revisions.exists(r, r.name == self.desiredRevision && self.revisions.all(s, s.revision <= r.revision))",message="desiredRevision must be the name of the revision with the highest revision number" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.currentRevision) || self.revisions.exists(r, r.name == self.currentRevision)",message="currentRevision must correspond to an entry in the revisions list" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.observedRevisionGeneration) || has(self.observedRevisionGeneration)",message="observedRevisionGeneration may not be unset once set" | ||
| type ClusterAPIStatus struct { | ||
| // currentRevision is the name of the most recently fully applied revision. | ||
| // It is written by the installer controller. If it is absent, it indicates |
There was a problem hiding this comment.
3. Unset-forbidden status field 🐞 Bug ☼ Reliability
The new status-level XValidation forbidding unsetting observedRevisionGeneration can make status updates fail during controller version skew, because multiple controllers write ClusterAPIStatus and an older writer will drop the new field on marshal. This can block unrelated status updates (e.g., installer controller updating currentRevision) once the revision controller has set observedRevisionGeneration.
Agent Prompt
### Issue description
A new XValidation rule forbids unsetting `status.observedRevisionGeneration` once set. Because `ClusterAPIStatus` is written by multiple controllers, and because older controller versions (or any writer using stale types) will drop unknown fields when marshalling status, this rule can cause status update failures and block progress.
### Issue Context
- `currentRevision` is written by the installer controller.
- `desiredRevision`/`revisions` are written by the revision controller.
Once the revision controller starts setting `observedRevisionGeneration`, any older installer controller still running (or any status writer not preserving the field) will have its `status` updates rejected.
### Fix Focus Areas
- operator/v1alpha1/types_clusterapi.go[78-123]
- operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yaml[314-325]
- operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml[1074-1290]
### Suggested fix
Prefer backward-compatible validation:
- Drop the `may not be unset once set` XValidation, keeping only:
- `Minimum=1`
- `self >= oldSelf` (monotonic, when present)
This avoids hard failures during version skew; the revision controller can re-populate the field if an older writer clears it during rollout.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
The API is v1alpha1 and the controller is in TPNU. I don't think we need to worry about this. I prefer the strictness.
|
@mdbooth: This pull request references OCPCLOUD-3359 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/v1alpha1/types_clusterapi.go`:
- Around line 191-197: The manifestSubstitutions entry declares Value as a
pointer which prevents kubebuilder from marking it required; change the Value
field from a pointer (*string) to a non-pointer string type (string), keep the
existing kubebuilder comment markers (e.g., // +required and
MinLength/MaxLength), and remove the `omitempty` from the
`json:"value,omitempty"` tag (use `json:"value"`) so the generated CRD requires
the field while still allowing an empty string as a valid value; update the
struct field named `Value` in the same type that contains
`manifestSubstitutions` accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1ca73141-9640-4909-bd58-075370a70025
⛔ Files ignored due to path filters (3)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (5)
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gooperator/v1alpha1/zz_generated.deepcopy.gooperator/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/crds/0000_30_cluster-api_01_clusterapis.crd.yaml
- operator/v1alpha1/zz_generated.swagger_doc_generated.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest |
|
I have a complete implementation using this API in openshift/cluster-capi-operator#519. I am happy with the usability of it. /verified by @mdbooth |
openshift-ci-robot
added
the
verified
|
@mdbooth: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn |
|
/retest-required |
|
/test e2e-aws-serial-techpreview-2of2 |
|
/retest |
1 similar comment
|
/retest |
|
@mdbooth: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…tion Bump github.com/openshift/api to 3c6b218b (openshift/api#2786) which adds the ObservedRevisionGeneration field to ClusterAPIStatus. This field is needed for the hypershift install command to wait for the CAPI Operator to acknowledge unmanaged CRDs before applying them. The openshift/api bump requires k8s.io/* v0.35.1, which cascades into: - Bump openshift/client-go to a19e917 (compatible with new API) - Bump karpenter forks to versions built against k8s 0.35 - Fix MustBaseEnvSet call signature change (removed bool param) - Fix ClusterImagePolicy moved from config/v1alpha1 to config/v1 - Fix NodeSelectorRequirementWithMinValues struct change in karpenter - Patch vendored etcdctl for tablewriter v0.x -> v1.x API break Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bump github.com/openshift/api to 3c6b218b (openshift/api#2786) which adds the ObservedRevisionGeneration field to ClusterAPIStatus. The openshift/api bump requires k8s.io/* v0.35.1, which cascades into: - Bump openshift/client-go to a19e917 (compatible with new API) - Bump karpenter forks to versions built against k8s 0.35
|
/cherry-pick release-4.22 |
|
@mdbooth: new pull request created: #2819 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Bump github.com/openshift/api to 3c6b218b (openshift/api#2786) which adds the ObservedRevisionGeneration field to ClusterAPIStatus. The openshift/api bump requires k8s.io/* v0.35.1, which cascades into: - Bump openshift/client-go to a19e917 (compatible with new API) - Bump karpenter forks to versions built against k8s 0.35
Bump github.com/openshift/api to 3c6b218b (openshift/api#2786) which adds the ObservedRevisionGeneration field to ClusterAPIStatus. The openshift/api bump requires k8s.io/* v0.35.1, which cascades into: - Bump openshift/client-go to a19e917 (compatible with new API) - Bump karpenter forks to versions built against k8s 0.35
Bump github.com/openshift/api to 3c6b218b (openshift/api#2786) which adds the ObservedRevisionGeneration field to ClusterAPIStatus. The openshift/api bump requires k8s.io/* v0.35.1, which cascades into: - Bump openshift/client-go to a19e917 (compatible with new API) - Bump karpenter forks to versions built against k8s 0.35
* chore: bump k8s.io 0.34 → 0.35 and openshift/api Bump github.com/openshift/api to 3c6b218b (openshift/api#2786) which adds the ObservedRevisionGeneration field to ClusterAPIStatus. The openshift/api bump requires k8s.io/* v0.35.1, which cascades into: - Bump openshift/client-go to a19e917 (compatible with new API) - Bump karpenter forks to versions built against k8s 0.35 * chore: Update vendor and generated files Update vendor and generated files after k8s.io 0.34 → 0.35 and openshift/api bump * chore: fix code after k8s bump - Fix MustBaseEnvSet call signature change (removed bool param) - Fix ClusterImagePolicy moved from config/v1alpha1 to config/v1 - Fix NodeSelectorRequirementWithMinValues struct change in karpenter - Remove etcd/tests/v3 dependency to eliminate tablewriter v0.x/v1.x conflict (etcdctl uses v0.x API, karpenter requires v1.x) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: bump remaining k8s.io deps 0.34 → 0.35.1 Bumps cli-runtime, kube-aggregator, kube-scheduler, kubectl, and pod-security-admission from v0.34.2 to v0.35.1 to align with the core k8s.io modules already bumped in this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: disable WatchListClient for all HyperShift components client-go 0.35 enables WatchListClient by default, causing informers to use sendInitialEvents=true in watch requests. The hosted cluster's API server may not support this feature, causing the reflector to retry indefinitely without falling back to LIST. Disable the feature by default for all components: - HyperShift operator: guard in main(), covers all subcommands - CPO binary: guard in main(), covers all subcommands (ignition-server, etcd-defrag, konnectivity, token-minter, kas-bootstrap, etc.) - HCCO: env var set in deployment manifest (always false) - karpenter-operator: env var set in deployment manifest - control-plane-pki-operator: guard in main() The HO propagates its KUBE_FEATURE_WatchListClient env var to the CPO deployment dynamically, so the value can be overridden at the HO level. Components with the env var set in their deployment manifest (HCCO, karpenter-operator) are not affected by the code guard, as it only sets the value when the env var is not already present. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ci): include go.mod hash in build cache key and setup Go The ARC runners share a persistent Go build cache. When go.mod bumps the Go version (e.g. 1.25.3 → 1.25.7), stale cached objects compiled with the old version cause "does not match go tool version" errors. Include hashFiles('go.mod') in the cache key so the cache is invalidated when the Go version or dependencies change. Also add actions/setup-go to the test job to ensure the correct Go version from go.mod is used instead of the runner's pre-installed version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add unit tests for NormalizeV1Alpha1ClusterImagePolicy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
6 participants
component.name: This is a cosmetic change, but makes it easier for a human (e.g. dev/support) to understand what each component in a revision refers to.
component.manifestSubstitutions: These are values passed to 'envsubst' when rendering manifests. The initial use is to pass through cluster-wide TLS configuration in a controlled manner.
status.observedRevisionGeneration: Required by an API consumer to know that the revision controller has observed spec changes. Specifically, if a user adds an entry to spec.unmanagedCustomResourceDefinitions they need to know:
They are currently only able to observe the latter, making it impossible to use the API robustly.