Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add update backup target action #830

Merged
merged 5 commits into from
Dec 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/models/backup.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,14 +363,13 @@ export default {
}
},
updateBackgroundBackups(state, action) {
let volumeName = getBackupVolumeName(state.search)
if (volumeName && action.payload && action.payload.data) {
const backupData = action.payload.data.filter((item) => {
return item.volumeName === volumeName
})
const volumeId = state.search?.keyword
const targetBackupVolume = state.backupVolumes.find((item) => item.name === volumeId) || {}
if (targetBackupVolume?.volumeName && action.payload?.data) {
const backupData = action.payload.data.filter((item) => item.volumeName === targetBackupVolume?.volumeName)
return {
...state,
backupData: backupData || [],
backupData,
}
}
return {
houhoucoop marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
30 changes: 30 additions & 0 deletions src/models/volume.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ export default {
bulkExpandVolumeModalVisible: false,
updateBulkReplicaCountModalVisible: false,
customColumnVisible: false,
updateBackupTargetModalVisible: false,
updateBulkBackupTargetModalVisible: false,
volumeCloneModalVisible: false,
updateDataLocalityModalVisible: false,
updateSnapshotMaxCountModalVisible: false,
Expand All @@ -77,6 +79,7 @@ export default {
recurringJobList: [],
softAntiAffinityKey: '',
updateReplicaSoftAntiAffinityVisible: false,
updateBackupTargetModalKey: Math.random(),
changeVolumeModalKey: Math.random(),
bulkChangeVolumeModalKey: Math.random(),
bulkExpandVolumeModalKey: Math.random(),
Expand All @@ -93,6 +96,7 @@ export default {
bulkAttachHostModalKey: Math.random(),
engineUpgradeModaKey: Math.random(),
volumeCloneModalKey: Math.random(),
updateBulkBackupTargetModalKey: Math.random(),
bulkEngineUpgradeModalKey: Math.random(),
expansionVolumeSizeModalKey: Math.random(),
updateReplicaCountModalKey: Math.random(),
Expand Down Expand Up @@ -407,6 +411,20 @@ export default {
}
yield put({ type: 'query' })
},
*backupTargetUpdate({
payload,
}, { call, put }) {
yield put({ type: 'hideUpdateBackupTargetModal' })
yield call(execAction, payload.url, payload.params)
yield put({ type: 'query' })
},
*bulkBackupTargetUpdate({
payload,
}, { call, put }) {
yield put({ type: 'hideUpdateBulkBackupTargetModal' })
yield payload.urls.map(url => call(execAction, url, payload.params))
yield put({ type: 'query' })
},
Comment on lines +414 to +427
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for backup target updates

The backup target update effects should include error handling to provide feedback to users.

 *backupTargetUpdate({
   payload,
 }, { call, put }) {
   yield put({ type: 'hideUpdateBackupTargetModal' })
-  yield call(execAction, payload.url, payload.params)
+  try {
+    const resp = yield call(execAction, payload.url, payload.params)
+    if (resp && resp.status === 200) {
+      message.success('Backup target updated successfully')
+    } else {
+      throw new Error('Update failed')
+    }
+  } catch (err) {
+    message.error(`Failed to update backup target: ${err.message}`)
+  }
   yield put({ type: 'query' })
 },

Similar error handling should be added to bulkBackupTargetUpdate.

Committable suggestion skipped: line range outside the PR's diff.

*accessModeUpdate({
payload,
}, { call, put }) {
Expand Down Expand Up @@ -919,6 +937,12 @@ export default {
showUpdateAccessMode(state, action) {
return { ...state, ...action.payload, updateAccessModeModalVisible: true, updateAccessModeModalKey: Math.random() }
},
showUpdateBackupTarget(state, action) {
return { ...state, ...action.payload, updateBackupTargetModalVisible: true, updateBackupTargetModalKey: Math.random() }
},
showUpdateBulkBackupTarget(state, action) {
return { ...state, ...action.payload, updateBulkBackupTargetModalVisible: true, updateBulkBackupTargetModalKey: Math.random() }
},
showUpdateBulkReplicaCountModal(state, action) {
return { ...state, ...action.payload, updateBulkReplicaCountModalVisible: true, updateBulkReplicaCountModalKey: Math.random() }
},
Expand Down Expand Up @@ -952,6 +976,12 @@ export default {
hideUpdateAccessModeModal(state) {
return { ...state, updateAccessModeModalVisible: false }
},
hideUpdateBackupTargetModal(state) {
return { ...state, updateBackupTargetModalVisible: false }
},
hideUpdateBulkBackupTargetModal(state) {
return { ...state, updateBulkBackupTargetModalVisible: false }
},
showBulkExpandVolumeModal(state, action) {
return { ...state, bulkExpandVolumeModalVisible: true, selectedRows: action.payload, bulkExpandVolumeModalKey: Math.random() }
},
Expand Down
12 changes: 6 additions & 6 deletions src/routes/backup/BackupDetail.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ function Backup({ backup, volume, setting, backingImage, loading, location, disp
const defaultReplicaCountSetting = settings.find(s => s.id === 'default-replica-count')
const defaultNumberOfReplicas = defaultReplicaCountSetting !== undefined ? parseInt(defaultReplicaCountSetting.value, 10) : 3

const { volumeName } = queryString.parse(location.search)
const currentBackUp = backupVolumes.find((item) => item.volumeName === volumeName) || {}
const { keyword } = queryString.parse(location.search)
const currentBackUp = backupVolumes.find(bkVol => bkVol.name === keyword) || {}
const v1DataEngineEnabledSetting = settings.find(s => s.id === 'v1-data-engine')
const v2DataEngineEnabledSetting = settings.find(s => s.id === 'v2-data-engine')
const v1DataEngineEnabled = v1DataEngineEnabledSetting?.value === 'true'
const v2DataEngineEnabled = v2DataEngineEnabledSetting?.value === 'true'

const backups = volumeName ? backupData.filter(bk => bk.volumeName === volumeName) : backupData
sortBackups(backupData)
const backups = currentBackUp ? backupData.filter(bk => bk.volumeName === currentBackUp.volumeName) : backupData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to add the backuptarget filter to the URL as well?
The scenarios are as follows:

  • Create a volume with the backup target default and create a backup. (e.g. 1217-test)
  • Change the backup target to cifs and create another backup.
  • On the backup page, both backups appear with their respective targets (expected).
    Screenshot 2024-12-20 at 10 40 38 AM (2)
  • Clicking the volume with the default target shows cifs in the details page.
    Screenshot 2024-12-20 at 10 43 11 AM (2)

Copy link
Contributor

@houhoucoop houhoucoop Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue found: When clicking Restore Latest Backup and Create Disaster Recovery Volume for 1217-test (backup target: default), the modal doesn’t appear.

Copy link
Contributor Author

@a110605 a110605 Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the backend bug that the /v1/backupvolumes/bug-test-a98258ed?action=backupList (default) returns cifs backup data. @mantissahz will fix from backend first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@houhoucoop , @mantissahz , let's review and merge this PR to add update backup target feature first. We can fix this bug in next PR working with backend change.

sortBackups(backups)

const backupProps = {
backup: backups,
Expand Down Expand Up @@ -64,7 +64,7 @@ function Backup({ backup, volume, setting, backingImage, loading, location, disp
dispatch({
type: 'backup/delete',
payload: {
volumeName,
volumeName: currentBackUp.volumeName,
name: record.id,
listUrl,
...queryString.parse(location.search),
Expand Down Expand Up @@ -139,7 +139,7 @@ function Backup({ backup, volume, setting, backingImage, loading, location, disp
onOk() {
dispatch({
type: 'backup/deleteAllBackups',
payload: volumeName,
payload: currentBackUp.id,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate currentBackUp before deletion

The deletion payload should validate the existence of currentBackUp to prevent runtime errors.

 dispatch({
   type: 'backup/deleteAllBackups',
-  payload: currentBackUp.id,
+  payload: currentBackUp?.id,
 })
📝 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.

Suggested change
payload: currentBackUp.id,
payload: currentBackUp?.id,

})
},
})
Expand Down
6 changes: 1 addition & 5 deletions src/routes/backup/BackupVolume.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,7 @@ function BackupVolume({ backup, loading, setting, backingImage, dispatch, locati
payload: record,
})
},
DeleteAllBackups(record) {
// dispatch({
// type: 'backup/CreateStandVolume',
// payload: record,
// })
deleteAllBackups(record) {
showDeleteConfirm(record)
},
showBackingImageInfo(record) {
Expand Down
5 changes: 2 additions & 3 deletions src/routes/backup/BackupVolumeList.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class List extends React.Component {
if (e.key === 'recovery') {
this.props.Create(record)
} else if (e.key === 'deleteAll') {
this.props.DeleteAllBackups(record)
this.props.deleteAllBackups(record)
} else if (e.key === 'restoreLatestBackup') {
this.props.restoreLatestBackup(record)
} else if (e.key === 'syncBackupVolume') {
Expand Down Expand Up @@ -117,7 +117,6 @@ class List extends React.Component {
search: queryString.stringify({
field: 'name',
keyword: record.name,
volumeName: record.volumeName
}),
}}>
{id}
Expand Down Expand Up @@ -326,7 +325,7 @@ List.propTypes = {
onSorterChange: PropTypes.func,
Create: PropTypes.func,
onRowClick: PropTypes.func,
DeleteAllBackups: PropTypes.func,
deleteAllBackups: PropTypes.func,
dispatch: PropTypes.func,
restoreLatestBackup: PropTypes.func,
syncBackupVolume: PropTypes.func,
Expand Down
73 changes: 73 additions & 0 deletions src/routes/volume/UpdateBackupTargetModal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import React from 'react'
import PropTypes from 'prop-types'
import { Form, Select } from 'antd'
import { ModalBlur } from '../../components'
const FormItem = Form.Item
const { Option } = Select

const formItemLayout = {
labelCol: {
span: 6,
},
wrapperCol: {
span: 16,
},
}

const modal = ({
item,
backupTargets,
visible,
onCancel,
onOk,
form: {
getFieldDecorator,
validateFields,
getFieldsValue,
},
}) => {
function handleOk() {
validateFields((errors) => {
if (errors) {
return
}
const data = { ...getFieldsValue() }
const url = item?.actions?.updateBackupTargetName
onOk(data, url)
Comment on lines +34 to +36
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for failed API calls

The handleOk function should handle potential API failures. Consider adding a try-catch block and error state management.

-      const data = { ...getFieldsValue() }
-      const url = item?.actions?.updateBackupTargetName
-      onOk(data, url)
+      try {
+        const data = { ...getFieldsValue() }
+        const url = item?.actions?.updateBackupTargetName
+        if (!url) {
+          throw new Error('Update action URL not found')
+        }
+        onOk(data, url)
+      } catch (error) {
+        console.error('Failed to update backup target:', error)
+        // Handle error appropriately
+      }

Committable suggestion skipped: line range outside the PR's diff.

})
}

const modalOpts = {
title: 'Update Backup Target',
visible,
onCancel,
onOk: handleOk,
}
if (!item) {
return null
}
return (
<ModalBlur {...modalOpts}>
<Form layout="horizontal">
<FormItem label="BackupTarget" {...formItemLayout}>
{getFieldDecorator('backupTargetName', {
initialValue: backupTargets.find(bt => bt.name === item.backupTargetName)?.name || 'default',
})(<Select>
{ backupTargets.map(bt => <Option key={bt.name} disabled={bt.available === false} value={bt.name}>{bt.name}</Option>)}
</Select>)}
</FormItem>
Comment on lines +54 to +58
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding validation rules for backup target selection

The form field lacks validation rules to ensure a backup target is selected.

 {getFieldDecorator('backupTargetName', {
   initialValue: backupTargets.find(bt => bt.name === item.backupTargetName)?.name || 'default',
+  rules: [{
+    required: true,
+    message: 'Please select a backup target',
+  }],
 })(<Select>
📝 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.

Suggested change
initialValue: backupTargets.find(bt => bt.name === item.backupTargetName)?.name || 'default',
})(<Select>
{ backupTargets.map(bt => <Option key={bt.name} disabled={bt.available === false} value={bt.name}>{bt.name}</Option>)}
</Select>)}
</FormItem>
initialValue: backupTargets.find(bt => bt.name === item.backupTargetName)?.name || 'default',
rules: [{
required: true,
message: 'Please select a backup target',
}],
})(<Select>
{ backupTargets.map(bt => <Option key={bt.name} disabled={bt.available === false} value={bt.name}>{bt.name}</Option>)}
</Select>)}
</FormItem>

</Form>
</ModalBlur>
)
}

modal.propTypes = {
form: PropTypes.object.isRequired,
visible: PropTypes.bool,
backupTargets: PropTypes.array,
onCancel: PropTypes.func,
item: PropTypes.object,
onOk: PropTypes.func,
}

export default Form.create()(modal)
76 changes: 76 additions & 0 deletions src/routes/volume/UpdateBulkBackupTargetModal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import React from 'react'
import PropTypes from 'prop-types'
import { Form, Select } from 'antd'
import { ModalBlur } from '../../components'
const FormItem = Form.Item
const { Option } = Select

const formItemLayout = {
labelCol: {
span: 6,
},
wrapperCol: {
span: 16,
},
}

const modal = ({
items,
backupTargets,
visible,
onCancel,
onOk,
form: {
getFieldDecorator,
validateFields,
getFieldsValue,
},
}) => {
function handleOk() {
validateFields((errors) => {
if (errors) {
return
}
const data = { ...getFieldsValue() }
const urls = items.reduce((final, item) => {
final.push(item.actions.updateBackupTargetName)
return final
}, [])
onOk(data, urls)
Comment on lines +34 to +39
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation and optimize bulk update processing

The bulk update processing needs validation and optimization improvements.

-      const data = { ...getFieldsValue() }
-      const urls = items.reduce((final, item) => {
-        final.push(item.actions.updateBackupTargetName)
-        return final
-      }, [])
+      const data = { ...getFieldsValue() }
+      if (!Array.isArray(items) || items.length === 0) {
+        throw new Error('No items selected for bulk update')
+      }
+      const urls = items
+        .filter(item => item?.actions?.updateBackupTargetName)
+        .map(item => item.actions.updateBackupTargetName)
+      if (urls.length === 0) {
+        throw new Error('No valid update URLs found')
+      }
📝 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.

Suggested change
const data = { ...getFieldsValue() }
const urls = items.reduce((final, item) => {
final.push(item.actions.updateBackupTargetName)
return final
}, [])
onOk(data, urls)
const data = { ...getFieldsValue() }
if (!Array.isArray(items) || items.length === 0) {
throw new Error('No items selected for bulk update')
}
const urls = items
.filter(item => item?.actions?.updateBackupTargetName)
.map(item => item.actions.updateBackupTargetName)
if (urls.length === 0) {
throw new Error('No valid update URLs found')
}
onOk(data, urls)

})
}

const modalOpts = {
title: 'Update Backup Target',
visible,
onCancel,
onOk: handleOk,
}
if (!items) {
return null
}
return (
<ModalBlur {...modalOpts}>
<Form layout="horizontal">
<FormItem label="BackupTarget" {...formItemLayout}>
{getFieldDecorator('backupTargetName', {
initialValue: 'default',
})(<Select>
{ backupTargets.map(bt => <Option key={bt.name} disabled={bt.available === false} value={bt.name}>{bt.name}</Option>)}
</Select>)}
</FormItem>
</Form>
</ModalBlur>
)
}

modal.propTypes = {
form: PropTypes.object.isRequired,
visible: PropTypes.bool,
backupTargets: PropTypes.array,
onCancel: PropTypes.func,
items: PropTypes.array,
onOk: PropTypes.func,
}

export default Form.create()(modal)
5 changes: 5 additions & 0 deletions src/routes/volume/VolumeActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ function actions({
changeVolume,
showUpdateDataLocality,
showUpdateAccessMode,
showUpdateBackupTarget,
showUpdateReplicaAutoBalanceModal,
showUnmapMarkSnapChainRemovedModal,
engineUpgradePerNodeLimit,
Expand Down Expand Up @@ -128,6 +129,9 @@ function actions({
case 'updateAccessMode':
showUpdateAccessMode(record)
break
case 'updateBackupTargetName':
showUpdateBackupTarget(record)
break
case 'pvAndpvcCreate':
createPVAndPVC(record)
break
Expand Down Expand Up @@ -223,6 +227,7 @@ function actions({
{ key: 'updateDataLocality', name: 'Update Data Locality', disabled: !canUpdateDataLocality() || upgradingEngine() },
{ key: 'updateSnapshotDataIntegrity', name: 'Snapshot Data Integrity', disabled: false },
{ key: 'updateAccessMode', name: 'Update Access Mode', disabled: (selected.kubernetesStatus && selected.kubernetesStatus.pvStatus) || !canUpdateAccessMode() },
{ key: 'updateBackupTargetName', name: 'Update Backup Target' },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding disabled condition for backup target update action.

The action item for updating backup target is missing a disabled condition, unlike other actions in the array. Consider adding appropriate conditions to prevent updates in invalid states.

Apply this diff:

-    { key: 'updateBackupTargetName', name: 'Update Backup Target' },
+    { key: 'updateBackupTargetName', name: 'Update Backup Target', disabled() { return !backupTargetAvailable || isHasStandy() || hasVolumeRestoring() } },

Committable suggestion skipped: line range outside the PR's diff.

{ key: 'updateReplicaAutoBalance', name: 'Update Replicas Auto Balance', disabled: !canUpdateReplicaAutoBalance() },
{ key: 'updateUnmapMarkSnapChainRemoved', name: 'Allow snapshots removal during trim', disabled: false },
{ key: 'updateReplicaSoftAntiAffinity', name: 'Update Replica Soft Anti Affinity', disabled: false },
Expand Down
6 changes: 6 additions & 0 deletions src/routes/volume/VolumeBulkActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ function bulkActions({
backupTargetMessage,
showBulkUnmapMarkSnapChainRemovedModal,
trimBulkFilesystem,
showUpdateBulkBackupTarget,
showUpdateBulkSnapshotDataIntegrityModal,
showUpdateReplicaSoftAntiAffinityModal,
showUpdateReplicaZoneSoftAntiAffinityModal,
Expand Down Expand Up @@ -116,6 +117,9 @@ function bulkActions({
case 'updateBulkAccessMode':
showUpdateBulkAccessMode(selectedRows)
break
case 'updateBulkBackupTarget':
showUpdateBulkBackupTarget(selectedRows)
break
case 'createPVAndPVC':
createPVAndPVC(selectedRows)
break
Expand Down Expand Up @@ -200,6 +204,7 @@ function bulkActions({
{ key: 'updateBulkDataLocality', name: 'Update Data Locality', disabled() { return selectedRows.length === 0 || isHasStandy() || disableUpdateBulkDataLocality() || upgradingEngine() } },
{ key: 'updateSnapshotDataIntegrity', name: 'Snapshot Data Integrity', disabled() { return selectedRows.length === 0 } },
{ key: 'updateBulkAccessMode', name: 'Update Access Mode', disabled() { return selectedRows.length === 0 || isHasStandy() || disableUpdateAccessMode() } },
{ key: 'updateBulkBackupTarget', name: 'Update Backup Target', disabled() { return selectedRows.length === 0 } },
{ key: 'updateReplicaAutoBalance', name: 'Update Replicas Auto Balance', disabled() { return selectedRows.length === 0 || disableUpdateReplicaAutoBalance() } },
{ key: 'createPVAndPVC', name: 'Create PV/PVC', disabled() { return selectedRows.length === 0 || isHasStandy() || hasVolumeRestoring() || isHasPVC() || isFaulted() || !hasAction('pvCreate') || !hasAction('pvcCreate') } },
{ key: 'bulkChangeVolume', name: 'Activate Disaster Recovery Volume', disabled() { return selectedRows.length === 0 || selectedRows.some((item) => !item.standby) } },
Expand Down Expand Up @@ -256,6 +261,7 @@ bulkActions.propTypes = {
showBulkCloneVolume: PropTypes.func,
showUpdateBulkReplicaCount: PropTypes.func,
showUpdateBulkDataLocality: PropTypes.func,
showUpdateBulkBackupTarget: PropTypes.func,
showUpdateBulkAccessMode: PropTypes.func,
showUpdateBulkSnapshotDataIntegrityModal: PropTypes.func,
showUpdateReplicaAutoBalanceModal: PropTypes.func,
Expand Down
Loading
Loading