OCPNODE-4168: Empty disk should be ignored for gcp additional disk#141
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
1 similar comment
|
/retest-required |
|
Adding a unit test here would probably be beneficial, however not worth blocking on. If you feel like raising a follow up that would be great. I've asked QE to verify, as we don't have any e2es for this :) /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: theobarberbany 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
|
/hold |
openshift-ci
Bot
added
do-not-merge/hold
WalkthroughDisk image handling in the GCP machine reconciler was changed: disk images are now optional, source image resolution to the project happens only for non-empty image strings, boot disks without an image now fail validation, and initialization parameters for attached disks are built via a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 the current code and only fix it if needed.
Inline comments:
In `@pkg/cloud/gcp/actuators/machine/reconciler_test.go`:
- Around line 407-416: In validateInstance (test helper) you currently call
t.Errorf when len(instance.Disks) != 2 but then immediately access
instance.Disks[1], which can panic; change the flow so the test stops or returns
when the disk count is wrong (e.g., replace t.Errorf with t.Fatalf or follow the
t.Errorf with an immediate return) before any access to instance.Disks[1] or
other indexes to avoid the panic.
In `@pkg/cloud/gcp/actuators/machine/reconciler.go`:
- Around line 317-341: The current logic skips setting initParams.SourceImage
whenever disk.Image is empty, which also affects boot disks; update the handling
so that an empty Image is only allowed for non-boot disks and boot disks with an
empty Image are rejected. Concretely, keep srcImage and the existing SourceImage
assignment, but before skipping SourceImage check disk.Boot (or the struct's
equivalent) and if it's a boot disk and disk.Image == "" return a clear error
(e.g., "boot disk must specify an image"); otherwise allow skipping SourceImage
for non-boot disks. Apply this change around the srcImage/disk.Image logic and
the initParams.SourceImage assignment in reconciler.go (symbols: srcImage,
disk.Image, disk.Boot, initParams.SourceImage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26e791e3-eae0-4dee-8ed9-969c665497e8
📒 Files selected for processing (2)
pkg/cloud/gcp/actuators/machine/reconciler.gopkg/cloud/gcp/actuators/machine/reconciler_test.go
| validateInstance: func(t *testing.T, instance *compute.Instance) { | ||
| if len(instance.Disks) != 2 { | ||
| t.Errorf("expected two disks, got %d", len(instance.Disks)) | ||
| } | ||
| if instance.Disks[1].InitializeParams.SourceImage != "" { | ||
| t.Errorf("Expected blank disk (empty SourceImage), got: %q", instance.Disks[1].InitializeParams.SourceImage) | ||
| } | ||
| if instance.Disks[1].InitializeParams.DiskSizeGb != 100 { | ||
| t.Errorf("Expected disk size 100, got: %d", instance.Disks[1].InitializeParams.DiskSizeGb) | ||
| } |
There was a problem hiding this comment.
Avoid potential panic in validation path after length mismatch.
Line 408 uses t.Errorf and then still accesses instance.Disks[1] (Line 411+). If disk count is wrong, this can panic and hide the actual assertion failure.
✅ Suggested hardening
validateInstance: func(t *testing.T, instance *compute.Instance) {
if len(instance.Disks) != 2 {
- t.Errorf("expected two disks, got %d", len(instance.Disks))
+ t.Fatalf("expected two disks, got %d", len(instance.Disks))
}
+ if instance.Disks[1].InitializeParams == nil {
+ t.Fatalf("expected InitializeParams on secondary disk")
+ }
if instance.Disks[1].InitializeParams.SourceImage != "" {
t.Errorf("Expected blank disk (empty SourceImage), got: %q", instance.Disks[1].InitializeParams.SourceImage)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| validateInstance: func(t *testing.T, instance *compute.Instance) { | |
| if len(instance.Disks) != 2 { | |
| t.Errorf("expected two disks, got %d", len(instance.Disks)) | |
| } | |
| if instance.Disks[1].InitializeParams.SourceImage != "" { | |
| t.Errorf("Expected blank disk (empty SourceImage), got: %q", instance.Disks[1].InitializeParams.SourceImage) | |
| } | |
| if instance.Disks[1].InitializeParams.DiskSizeGb != 100 { | |
| t.Errorf("Expected disk size 100, got: %d", instance.Disks[1].InitializeParams.DiskSizeGb) | |
| } | |
| validateInstance: func(t *testing.T, instance *compute.Instance) { | |
| if len(instance.Disks) != 2 { | |
| t.Fatalf("expected two disks, got %d", len(instance.Disks)) | |
| } | |
| if instance.Disks[1].InitializeParams == nil { | |
| t.Fatalf("expected InitializeParams on secondary disk") | |
| } | |
| if instance.Disks[1].InitializeParams.SourceImage != "" { | |
| t.Errorf("Expected blank disk (empty SourceImage), got: %q", instance.Disks[1].InitializeParams.SourceImage) | |
| } | |
| if instance.Disks[1].InitializeParams.DiskSizeGb != 100 { | |
| t.Errorf("Expected disk size 100, got: %d", instance.Disks[1].InitializeParams.DiskSizeGb) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cloud/gcp/actuators/machine/reconciler_test.go` around lines 407 - 416,
In validateInstance (test helper) you currently call t.Errorf when
len(instance.Disks) != 2 but then immediately access instance.Disks[1], which
can panic; change the flow so the test stops or returns when the disk count is
wrong (e.g., replace t.Errorf with t.Fatalf or follow the t.Errorf with an
immediate return) before any access to instance.Disks[1] or other indexes to
avoid the panic.
ngopalak-redhat
force-pushed
the
ngopalak/fix_gcp_disk
branch
from
c701585 to
60f5226
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/cloud/gcp/actuators/machine/reconciler_test.go (1)
421-429:⚠️ Potential issue | 🟡 MinorGuard against panic after disk-count mismatch in test validation.
At Line 421-Line 423, the test logs with
t.Errorfand then still dereferencesinstance.Disks[1]on Line 424-Line 428. If disk creation regresses, this can panic and hide the root failure.✅ Suggested hardening
validateInstance: func(t *testing.T, instance *compute.Instance) { if len(instance.Disks) != 2 { - t.Errorf("expected two disks, got %d", len(instance.Disks)) + t.Fatalf("expected two disks, got %d", len(instance.Disks)) + } + if instance.Disks[1].InitializeParams == nil { + t.Fatalf("expected InitializeParams on secondary disk") } if instance.Disks[1].InitializeParams.SourceImage != "" { t.Errorf("Expected blank disk (empty SourceImage), got: %q", instance.Disks[1].InitializeParams.SourceImage) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloud/gcp/actuators/machine/reconciler_test.go` around lines 421 - 429, The test currently calls t.Errorf when the disk count is wrong but then unconditionally dereferences instance.Disks[1], which can panic; change the validation to stop the test immediately on wrong count (e.g., replace the t.Errorf for len(instance.Disks) != 2 with t.Fatalf or add an immediate return/FailNow) so subsequent checks of instance.Disks[1].InitializeParams (SourceImage and DiskSizeGb) are only executed when the disk slice has the expected length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cloud/gcp/actuators/machine/reconciler_test.go`:
- Around line 421-429: The test currently calls t.Errorf when the disk count is
wrong but then unconditionally dereferences instance.Disks[1], which can panic;
change the validation to stop the test immediately on wrong count (e.g., replace
the t.Errorf for len(instance.Disks) != 2 with t.Fatalf or add an immediate
return/FailNow) so subsequent checks of instance.Disks[1].InitializeParams
(SourceImage and DiskSizeGb) are only executed when the disk slice has the
expected length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf421ab0-f4c3-4647-8053-ea3ad06ba16c
📒 Files selected for processing (2)
pkg/cloud/gcp/actuators/machine/reconciler.gopkg/cloud/gcp/actuators/machine/reconciler_test.go
|
@ngopalak-redhat: 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. |
|
/lgtm |
ngopalak-redhat
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ngopalak-redhat: This pull request references OCPNODE-4168 which is a valid jira issue. 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. |
|
/verified by @lunarwhite |
openshift-ci-robot
added
the
verified
|
@ngopalak-redhat: 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. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
5 participants
Context: We are tryin to add additional disk to GCP to test split disk: https://issues.redhat.com/browse/OCPSTRAT-188
Error:
How to recreate:
This will cause the above error.
Fix:
Approach 1: Change the API to make the image field a pointer. I didn't take this approach as API change can impact existing users
Approach 2: Empty image can be ignored - This approach is taken
Related PR: openshift/machine-api-operator#1471
Summary by CodeRabbit
Bug Fixes
Tests