fix: improved fetching details of VM from vmware in less API calls #828
Conversation
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (3)
-
k8s/migration/internal/controller/vmwarecreds_controller.go (127-130)
The error message when creating/updating VMwareMachine doesn't include which VM failed, making it difficult to troubleshoot issues with specific VMs.
err = utils.CreateOrUpdateVMwareMachine(ctx, scope.Client, scope.VMwareCreds, &vm) if err != nil { return ctrl.Result{}, errors.Wrap(err, fmt.Sprintf("Error creating or updating VMwareMachine '%s' for VMwareCreds", vm.Name)) } -
k8s/migration/internal/controller/vmwarecreds_controller.go (123-126)
The current logging when skipping VMs with empty fields could be more informative. Consider logging which specific fields are empty for each VM to make debugging easier.
if vm.Name == "" || vm.ESXiName == "" || vm.ClusterName == "" { ctxlog.Info("Skipping VM with empty required fields", "VM", vm.Name, "ESXi", vm.ESXiName, "Cluster", vm.ClusterName, "nameEmpty", vm.Name == "", "esxiEmpty", vm.ESXiName == "", "clusterEmpty", vm.ClusterName == "") continue } - k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_rdmdisks.yaml (53-53) The description for `openstackVolumeRef` has been changed from 'OpenstackVolumeRefInfo' to 'OpenstackVolumeRef ...'. For clarity in documentation, the description should use the exact type name without ellipsis. If the field type is indeed `OpenstackVolumeRef`, then the description should be updated accordingly without the '...'.
💡 To request another review, post a new comment with "/windsurf-review".
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #23be1e
Actionable Suggestions - 1
-
k8s/migration/pkg/utils/credutils.go - 1
- Premature storage of host reference in map · Line 1661-1662
Additional Suggestions - 2
-
k8s/migration/pkg/utils/credutils.go - 2
-
Redundant VM configuration and name prefix check · Line 1597-1599Duplicate check for VM config and vCLS prefix. The same condition is checked twice, making the second check redundant. Remove the second check to improve code clarity and performance.
Code suggestion
@@ -1647,9 +1647,6 @@ // NIC & Guest Network extraction nic, _ := ExtractVirtualNICs(&moVM) guestNetwork, _ := ExtractGuestNetworkInfo(&moVM) - - if moVM.Config == nil || strings.HasPrefix(moVM.Config.Name, "vCLS-") { - continue - } // Determine cluster name var clusterName string
-
Redundant map store operation causing inefficiency · Line 1678-1678Duplicate `hostSystemMap.Store()` call with the same key but different value. The second call overwrites the first one, making the first call redundant.
Code suggestion
@@ -1677,8 +1677,7 @@ ESXIName: esxiName, ClusterName: clusterName, } - hostSystemMap.Store(moVM.Runtime.Host.Value, clusterDetails) hostSystemMap.Store(moVM.Runtime.Host.Value, clusterDetails)
-
Review Details
-
Files reviewed - 4 · Commit Range:
ad60b93..ad60b93- deploy/00crds.yaml
- k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_rdmdisks.yaml
- k8s/migration/internal/controller/vmwarecreds_controller.go
- k8s/migration/pkg/utils/credutils.go
-
Files skipped - 0
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
|
Bito Review Skipped - No Changes Detected |
There was a problem hiding this comment.
Code Review Agent Run #401feb
Actionable Suggestions - 1
-
k8s/migration/pkg/utils/credutils.go - 1
- RDM disk info updates overwritten in loop · Line 1398-1408
Review Details
-
Files reviewed - 1 · Commit Range:
ad60b93..d6c4704- k8s/migration/pkg/utils/credutils.go
-
Files skipped - 0
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
|
@sarika-p9 @OmkarDeshpande7 : Please take commits from this PR : RDMDisk attributes - not populating. Considering my time constraints please take it over for this change from here. |
|
Bito Review Skipped - No Changes Detected |
… details retrieval
…al parameters for improved functionality
There was a problem hiding this comment.
Code Review Agent Run #27d427
Actionable Suggestions - 1
-
k8s/migration/internal/controller/vmwarecreds_controller.go - 1
- Cleanup operations in wrong location · Line 131-139
Review Details
-
Files reviewed - 5 · Commit Range:
d6c4704..f8decca- k8s/migration/internal/controller/vmwarecreds_controller.go
- k8s/migration/pkg/utils/credutils.go
- v2v-helper/migrate/migrate_test.go
- v2v-helper/openstack/openstackops_mock.go
- v2v-helper/vm/vmops_mock.go
-
Files skipped - 0
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
…ent and batch processing for VMs
There was a problem hiding this comment.
Code Review Agent Run #e02605
Actionable Suggestions - 7
-
k8s/migration/internal/controller/vmwarecreds_controller.go - 3
- Division by zero risk · Line 131-135
- Division by zero risk · Line 131-135
- Resource leak in error handling · Line 92-94
-
k8s/migration/pkg/utils/credutils.go - 1
- Nil pointer dereference · Line 1593-1595
-
k8s/migration/pkg/utils/rollingmigrationutils.go - 1
- Defer placement causes nil panic · Line 221-225
-
k8s/migration/pkg/utils/maintenance_mode.go - 2
- Nil pointer dereference · Line 179-179
- Nil pointer dereference · Line 178-180
Additional Suggestions - 2
-
k8s/migration/pkg/utils/credutils.go - 1
-
Redundant nil check · Line 1568-1568Redundant nil check for moVM.Config. The same condition is already checked at line 1555, making this check unreachable and unnecessary. This creates confusion about the control flow and suggests the code structure needs cleanup.
Code suggestion
@@ -1567,4 +1567,1 @@ - // NIC & Guest Network extraction - nicList, _ := ExtractVirtualNICs(&moVM) - if moVM.Config == nil || strings.HasPrefix(moVM.Config.Name, "vCLS-") { - continue + // NIC & Guest Network extraction + nicList, _ := ExtractVirtualNICs(&moVM)
-
-
k8s/migration/internal/controller/vmwarecreds_controller.go - 1
-
Redundant cleanup defer · Line 106-106Remove the redundant defer statement at line 106 since we've already added a more comprehensive cleanup mechanism earlier in the function. Having two defer statements for the same cleanup operation would attempt to logout twice, which could cause issues.
Code suggestion
@@ -105,2 +105,1 @@ - // log out from vCenter or ESXI - defer utils.LogoutVMwareClient(ctx, r.Client, scope.VMwareCreds, connection) // Logout the client + // log out from vCenter or ESXI
-
Review Details
-
Files reviewed - 4 · Commit Range:
f8decca..4e1e6cf- k8s/migration/internal/controller/vmwarecreds_controller.go
- k8s/migration/pkg/utils/credutils.go
- k8s/migration/pkg/utils/maintenance_mode.go
- k8s/migration/pkg/utils/rollingmigrationutils.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
…-in-one-call to seperate changes
…ed properly in maintenance mode
…ed properly in maintenance mode
There was a problem hiding this comment.
Code Review Agent Run #17f560
Actionable Suggestions - 1
-
k8s/migration/pkg/utils/rollingmigrationutils.go - 1
- Resource cleanup order issue · Line 228-228
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
pkg/vpwned/upgrade/version_checker.go - 1
- Registry migration risk · Line 141-141
Review Details
-
Files reviewed - 6 · Commit Range:
4e1e6cf..e454688- pkg/vpwned/server/version.go
- pkg/vpwned/upgrade/version_checker.go
- k8s/migration/internal/controller/vmwarecreds_controller.go
- k8s/migration/pkg/utils/credutils.go
- k8s/migration/pkg/utils/maintenance_mode.go
- k8s/migration/pkg/utils/rollingmigrationutils.go
-
Files skipped - 0
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
Co-authored-by: bito-code-review[bot] <188872107+bito-code-review[bot]@users.noreply.github.com> Signed-off-by: rishabh625 <43094970+rishabh625@users.noreply.github.com>
Code Review Agent Run #238473Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
… and streamline client creation
… and streamline client creation
There was a problem hiding this comment.
Code Review Agent Run #a7a045
Actionable Suggestions - 1
-
k8s/migration/pkg/utils/credutils.go - 1
- Missing client caching · Line 514-515
Review Details
-
Files reviewed - 1 · Commit Range:
08a9102..1c6742a- k8s/migration/pkg/utils/credutils.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
…ary deferred functions
…ary deferred functions
…ary deferred functions
There was a problem hiding this comment.
Code Review Agent Run #e94e1c
Actionable Suggestions - 1
-
k8s/migration/pkg/utils/maintenance_mode.go - 1
- Double logout race condition · Line 36-39
Review Details
-
Files reviewed - 4 · Commit Range:
1c6742a..036d121- k8s/migration/internal/controller/vmwarecreds_controller.go
- k8s/migration/pkg/utils/credutils.go
- k8s/migration/pkg/utils/maintenance_mode.go
- k8s/migration/pkg/utils/rollingmigrationutils.go
-
Files skipped - 0
-
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
…hing and session management
…ary deferred functions
…hing and session management
There was a problem hiding this comment.
Code Review Agent Run #e89271
Actionable Suggestions - 2
-
k8s/migration/pkg/utils/credutils.go - 2
- Premature cache storage · Line 1547-1548
- Uninitialized variable · Line 1728-1730
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
k8s/migration/pkg/utils/credutils.go - 1
- Nil pointer dereference · Line 1634-1634
Review Details
-
Files reviewed - 5 · Commit Range:
036d121..60fdffd- k8s/migration/pkg/utils/credutils.go
- k8s/migration/internal/controller/vmwarecreds_controller.go
- k8s/migration/pkg/utils/maintenance_mode.go
- k8s/migration/pkg/utils/rollingmigrationutils.go
- k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_migrations.yaml
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at mithil@platform9.com.
Documentation & Help
AI Code Review powered by
|
Bito Review Skipped - No Changes Detected |
|
@rishabh625 Please rebase the PR to the latest master, I will run the build again, also please share some scale testing numbers of before and after your change. |
Signed-off-by: sarika <sarika@platform9.com>
3 participants
What this PR does / why we need it
This PR aims to improve fetching of vmware virtual machines details efficiently
Which issue(s) this PR fixes
(optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged)fixes #790
Special notes for your reviewer
Testing done
please add testing details (logs, screenshots, etc.)
Time taken to create 1700 VMs - 50 seconds
Age of Pod - 8m
Age of CR created - 7m
Time taken to create CR - 50 secs ( age of pod - age of cr created)
Summary by Bito
This pull request refactors the VMware VM fetching mechanism to improve efficiency through batched processing, improved concurrency management, and reduced API calls. The changes include adding a Reauth flag to session setup, optimizing annotation and network name retrieval, enhancing RDM disk processing, and implementing improved host cluster information extraction and connection cleanup logic.