Storage Management UI integration with support backend changes (# 1095) #1277
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… related ops, Auto detect arrays on target env so that discovery is easy for the user, Create UI to trigger the flow and understand the basic flow.
- Add missing AssignedIP field to MigrationSpec - Add missing ServerGroup and AssignedIPsPerVM fields to MigrationPlanSpec - Add missing PeriodicSyncInterval and PeriodicSyncEnabled to AdvancedOptions - Remove duplicate Disk type declaration - Fix protobuf duplicate declarations - Restore vmwareutils.go from base branch - Add missing replace directives for local modules in go.mod - Regenerate deepcopy and CRDs
- Remove unused GlobalSettingsPage import - Fix incorrect SecurityGroupAndSSHKeyStep import (should be SecurityGroupAndServerGroupStep) - Add Boolean() cast for periodicSyncEnabled checkbox to fix type error
Contributor
|
Bito Automatic Review Skipped - Large PR |
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Other comments (24)
- v2v-helper/go.mod (7-7) This change downgrades the gophercloud dependency from v2.9.0 to v1.14.1, which is a major version change. This will likely require significant code changes as APIs between major versions are often incompatible. Please ensure all gophercloud API calls have been updated accordingly.
- v2v-helper/go.mod (10-10) The replace directive and dependency for `github.com/platform9/vjailbreak/common/utils` have been removed. If this module is still being used in the code, it will cause compilation errors. Please ensure all imports have been updated accordingly.
- k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_migrationplans.yaml (72-72) The `assignedIPsPerVM` field has been removed, which is used to assign specific IPs to VMs during cold migration. Is there a replacement for this functionality in the new schema? Without this field, how will IP assignments work for cold migrations?
- k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_migrationplans.yaml (72-72) The `periodicSyncEnabled` and `periodicSyncInterval` fields have been removed. The PR description mentions adding 'PeriodicSync options', but these fields are being removed. Where are the new periodic sync options defined?
- ui/src/features/migration/MigrationForm.tsx (288-288) There's a typo in the field error updater key. It should be 'openstackCreds' but is written as 'opeanstackCreds', which would cause OpenStack credential validation errors not to be properly displayed.
- ui/src/features/migration/MigrationForm.tsx (448-450) The error handler for ArrayCredsMapping is using the wrong field error key ('storageMapping'). This should be a unique key like 'arrayCredsMapping' to avoid confusion with regular storage mapping errors.
-
k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_migrationtemplates.yaml (84-92)
The PR adds a new storage copy method 'vendor-based' and makes storageMapping optional, but there's no validation to ensure the correct mapping field is provided based on the selected copy method. Consider adding validation to ensure that:
- When
storageCopyMethodis 'normal',storageMappingmust be provided - When
storageCopyMethodis 'vendor-based',arrayCredsMappingmust be provided
Without this validation, users could create invalid configurations that would fail during migration.
- When
- deploy/installer.yaml (4128-4128) The dnsPolicy has been changed from 'ClusterFirstWithHostNet' to 'ClusterFirst'. This might affect DNS resolution if the pod needs to access host network resources. Was this change intentional?
- ui/src/api/array-creds/arrayCreds.ts (217-222) The getArrayCredsSecret function should include error handling when decoding base64 values with atob() to prevent runtime errors if the values aren't valid base64 strings.
- k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_migrations.yaml (58-61) The PR removes the `assignedIP` field which was used for cold migration IP assignments, but I don't see a replacement field for this functionality. If this feature is still needed, consider adding a replacement field or documenting how this functionality is now handled differently.
-
ui/src/components/drawers/ArrayCredsDrawer.tsx (199-199)
The condition for credential validation should check if all required credential fields are provided, not just any one of them, to match the validation requirements.
// Wait for validation (credentials are always required now) if (formData.managementEndpoint?.trim() && formData.username?.trim() && formData.password?.trim()) { - ui/src/api/array-creds/arrayCreds.ts (206-208) The updateArrayCredsSecret function should handle errors more specifically. Currently, it assumes any error means the secret doesn't exist, but there could be other errors (like permission issues or network problems).
- ui/src/features/migration/NetworkAndStorageMappingStep.tsx (58-58) The code is using a string literal 'Succeeded' to check array validation status. Consider using a constant or enum value instead to avoid potential mismatches with the API's expected values.
- ui/src/features/migration/NetworkAndStorageMappingStep.tsx (65-69) When switching from 'vendor-based' to 'normal' storage copy method, the arrayCredsMappings aren't cleared. This could lead to confusion if the user switches back to 'vendor-based' later, as old mappings would still be present. Consider clearing arrayCredsMappings when switching to 'normal' mode.
-
ui/src/components/drawers/ArrayCredsDrawer.tsx (37-41)
The normalizeVendorType function only handles 'Pure Storage' but should include mappings for all vendor types in VENDOR_TYPES to ensure consistent normalization.
const normalizeVendorType = (vendorType: string): string => { const mapping: Record<string, string> = { 'Pure Storage': 'pure', 'NetApp ONTAP': 'ontap', 'HPE Alletra': 'hpalletra' } return mapping[vendorType] || vendorType.toLowerCase() } - deploy/00crds.yaml (1993-1996) The vendorType field in the MigrationPlan CRD is limited to only 'pure' as an enum value, but the PR description mentions supporting multiple vendors including NetApp ONTAP and HPE Alletra. Consider expanding the enum to include all supported vendor types for consistency.
- ui/src/features/migration/MigrationForm.tsx (459-459) The arrayCredsMapping parameter is typed as 'any', which reduces type safety. Consider using a more specific type to match the expected structure.
- ui/src/features/migration/MigrationForm.tsx (660-661) Using 'any' type for storageMappings and arrayCredsMapping variables reduces type safety. Consider using more specific types that match the expected structure of these objects.
-
k8s/migration/api/v1alpha1/migration_types.go (88-90)
The VendorType field has a comment indicating it should be an enum with 'pure' as a value, but there's no actual enum validation defined. Consider adding a proper kubebuilder validation marker like:
// VendorType specifies the vendor type of the storage system // +optional // +kubebuilder:validation:Enum=pure VendorType string `json:"vendorType,omitempty"` - k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_rollingmigrationplans.yaml (194-197) The `vendorType` enum currently only supports 'pure'. If there are plans to support additional storage vendors in the future, consider making this field more extensible or adding documentation about the limited support.
- k8s/migration/config/crd/bases/vjailbreak.k8s.pf9.io_rollingmigrationplans.yaml (172-173) The newly added `arrayCredsRef` field is missing a description. Consider adding one to maintain consistency with other fields and improve documentation.
- deploy/00crds.yaml (214-214) Similar to the ArrayCreds CRD, the singular form of ArrayCredsMapping is set to 'arraycredsmapping' while the plural is 'arraycredsmappings'. Consider changing the singular form to follow Kubernetes naming conventions.
- deploy/installer.yaml (22-22) The singular form for the ArrayCreds CRD is set to 'arraycreds' which is the same as the plural form. The singular form should typically be 'arraycred' to follow Kubernetes naming conventions.
- deploy/00crds.yaml (22-22) The singular form of the ArrayCreds CRD is set to 'arraycreds' (same as plural). This can cause confusion when referring to the resource. Consider changing the singular form to 'arraycred' to follow Kubernetes naming conventions.
💡 To request another review, post a new comment with "/windsurf-review".
| description: PeriodicSyncInterval is the interval at which the | ||
| migration plan should be synced | ||
| type: string | ||
| type: object |
Contributor
There was a problem hiding this comment.
The periodicSyncEnabled and periodicSyncInterval fields have been removed from the CRD. Make sure that any code referencing these fields has been updated accordingly to prevent runtime errors.
Comment on lines
58
to
59
| onClose={() => setOpenMigrationForm(false)} | ||
| onSuccess={handleSuccess} | ||
| /> |
Contributor
There was a problem hiding this comment.
The onSuccess prop has been removed from the MigrationFormDrawer component. If this component still expects this prop internally, it might cause runtime errors or unexpected behavior.
| } | ||
|
|
||
| // Timeout - validation took too long | ||
| return { success: false, message: 'Validation timed out after 60 seconds' } |
Contributor
There was a problem hiding this comment.
The error message mentions 60 seconds, but the actual timeout calculation is 30 attempts * 2000ms. For clarity, these should match.
Suggested change
| return { success: false, message: 'Validation timed out after 60 seconds' } | |
| // Timeout - validation took too long | |
| return { success: false, message: `Validation timed out after ${maxAttempts * intervalMs / 1000} seconds` } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Key changes:
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 #1095
Testing done