OSAC-769: Remove legacy SubnetRef field from ComputeInstance CRD#280
Conversation
|
@ori-amizur: This pull request references OSAC-769 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 task to target the "5.0.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. |
|
Warning Review limit reached
More reviews will be available in 50 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR removes the legacy single-NIC Risk: Moderate — the migration will modify existing CRs cluster-wide (or namespace-scoped) and must be validated to avoid unintended data changes. ChangesLegacy subnetRef field removal and networkAttachments migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/computeinstance_types.go`:
- Line 161: The current kubebuilder XValidation rule only checks size, allowing
replacement of the primary subnet entry; update the validation marker (the
+kubebuilder:validation:XValidation line) to enforce that if oldSelf is
non-empty the key of the primary attachment in self must equal the key of the
primary attachment in oldSelf (i.e., find element(s) where primary==true and
require their key values to be identical), so primary subnet key cannot be
changed after initial assignment; change the marker on the network attachments
field to implement this exact equality check between oldSelf and self for the
primary==true element.
In `@internal/migrations/migrations.go`:
- Around line 44-54: The runAll function currently uses the provided ctx without
a timeout, so wrap the migration loop in a derived context with a timeout (e.g.,
context.WithTimeout(ctx, 5*time.Minute)) and ensure defer cancel() is called;
then call each migration with the timed context (use timedCtx when invoking
m.Fn) and handle context.DeadlineExceeded by returning a clear error (or logging
via ctrllog.FromContext), so migrations (the all slice and each m.Fn) will be
canceled if they hang. Ensure imports include time and that you propagate the
wrapped context instead of the original.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4e02485d-abe1-4cf9-a805-8163a45f5588
📒 Files selected for processing (11)
api/v1alpha1/computeinstance_types.goapi/v1alpha1/computeinstance_types_test.gocmd/main.goconfig/crd/bases/osac.openshift.io_computeinstances.yamlinternal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/computeinstance_validation_test.gointernal/migrations/migrate_subnetrefs.gointernal/migrations/migrate_subnetrefs_test.gointernal/migrations/migrations.gointernal/migrations/migrations_suite_test.go
|
/retest |
Remove the deprecated spec.subnetRef field from ComputeInstanceSpec, completing the migration to the networkAttachments array for multi-NIC VM support. This aligns with the fulfillment-service (PR 565) which already removed its equivalent proto fields. Changes: - Remove SubnetRef field and mutual-exclusion CEL validation rule - Relax networkAttachments size rule to allow initial population (0→N) while still preventing subsequent add/remove - Add startup migration (internal/migrations) that patches legacy CRs using unstructured client to read stored subnetRef from etcd - Migration runs as LeaderElectionRunnable — only the leader executes - Update PrimarySubnetRef() to return empty string instead of legacy fallback - Update controller comments and tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/operator-crds/templates/osac.openshift.io_computeinstances.yaml`:
- Around line 189-191: The current CEL rule only checks sizes and allows
same-size replacements that can change subnetRef keys; update the validation
rule for the network attachment list to disallow any change to subnetRef for
existing entries by requiring either size(oldSelf) == 0 or (size(self) ==
size(oldSelf) AND each corresponding element preserves subnetRef). Replace the
existing expression (size(oldSelf) == 0 || size(self) == size(oldSelf)) with a
CEL expression that additionally asserts for all indices/keys in oldSelf that
oldSelf[i].subnetRef == self[i].subnetRef (or equivalent key-based comparison),
keep the message text, and regenerate/sync CRDs afterwards (run make helm-crds
or make check-helm-crds).
In `@internal/migrations/migrate_subnetrefs.go`:
- Around line 61-95: The current loop builds mergePatch from the list-time spec
(subnetRef) and unconditionally calls c.Patch (client.RawPatch with
types.MergePatchType), which can overwrite a concurrent update to
spec.networkAttachments; instead re-fetch the latest object before patching (use
c.Get on the same item.Name/Namespace), parse its spec and verify that
spec.networkAttachments is still empty (and spec.subnetRef still equals the
value you captured) and only then apply the mergePatch with c.Patch; if the
re-fetched object shows networkAttachments populated or subnetRef changed, skip
this item to avoid stale-write overwrite and continue the loop.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e04a13b8-e2eb-4eec-90e0-369e35f24b94
📒 Files selected for processing (12)
api/v1alpha1/computeinstance_types.goapi/v1alpha1/computeinstance_types_test.gocharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_computeinstances.yamlinternal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/computeinstance_validation_test.gointernal/migrations/migrate_subnetrefs.gointernal/migrations/migrate_subnetrefs_test.gointernal/migrations/migrations.gointernal/migrations/migrations_suite_test.go
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, ori-amizur 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 |
4 participants
Remove the deprecated spec.subnetRef field from ComputeInstanceSpec, completing the migration to the networkAttachments array for multi-NIC VM support. This aligns with the fulfillment-service (PR 565) which already removed its equivalent proto fields.
Changes:
Summary by CodeRabbit
New Features
Behavior / Schema Changes
Breaking Changes