Provide osd migration status alert for dr brownfield #1097
Conversation
|
|
|
@GowthamShanmugam Please review |
| severity: 'passed', | ||
| }, | ||
| }; | ||
| documentationLink = 'www.google.com'; |
There was a problem hiding this comment.
Please help me point the document link i should use ?
There was a problem hiding this comment.
simply use prod document of ODF DR section
|
Please add screenshots |
| icon = GreenCheckCircleIcon; | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
We definitely don't want to make it a part of Alert.
| <div className="mco-alert-name-container"> | ||
| {name && ( | ||
| <span className="mco-status-card__alert-item-header">{name}</span> | ||
| )} | ||
| {onClose && ( | ||
| <button | ||
| type="button" | ||
| onClick={onClose} | ||
| className="mco-close-button" | ||
| > | ||
| <CloseIcon /> | ||
| </button> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
This part is messing up with the alert card, can we keep this entire part are out of alert card?
There was a problem hiding this comment.
lets not touch alert cars, keep you status under alert card, Please discuss this with UX
| .mco-alert-name-container { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } | ||
|
|
||
| .mco-close-button { | ||
| background: none; | ||
| border: none; | ||
| cursor: pointer; | ||
| position: absolute; | ||
| left: 90%; | ||
| } |
There was a problem hiding this comment.
use patternfly components Flex and Button, you dont need these CSS
| .co-dashboard-migration-status { | ||
| border-top: var(--pf-global--BorderWidth--sm) solid var(--pf-global--BorderColor--100); | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| } |
There was a problem hiding this comment.
use <Divider />
| const getMigrationStatus = (data) => { | ||
| if (data && data.length > 0) { | ||
| const bluestoreCount = | ||
| data[0]?.status?.storage?.osd?.storeType?.['bluestore']; | ||
| const bluestoreRdrCount = | ||
| data[0]?.status?.storage?.osd?.storeType?.['bluestore-rdr']; | ||
|
|
||
| const isDisasterRecoveryTarget = | ||
| data[0]?.metadata?.annotations?.[DISASTER_RECOVERY_TARGET_ANNOTATION] === | ||
| 'true'; | ||
|
|
||
| if (bluestoreCount > 0) { | ||
| if (bluestoreRdrCount > 0) { | ||
| return MigrationStatus.InProgress; | ||
| } else if (isDisasterRecoveryTarget) { | ||
| return MigrationStatus.InProgress; | ||
| } else { | ||
| return MigrationStatus.Pending; | ||
| } | ||
| } else if (bluestoreRdrCount > 0) { | ||
| return MigrationStatus.Completed; | ||
| } | ||
|
|
||
| return ''; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This function is also used in detail cards to trigger migration, keep one function and keep it in common place.
| activeAt: currentDateTime, | ||
| state: AlertStates.Silenced, | ||
| annotations: { | ||
| description: `Cluster is undergoing OSDs migration. ${percentageComplete}% completed (${migratedDevices}/${totalOsd} remaining)`, |
There was a problem hiding this comment.
translate the text
| description: `Cluster is undergoing OSDs migration. ${percentageComplete}% completed (${migratedDevices}/${totalOsd} remaining)`, | ||
| }, | ||
| labels: { | ||
| alertname: 'Cluster OSDs are being migrated', |
There was a problem hiding this comment.
translate the text
| }, | ||
| labels: { | ||
| alertname: 'Cluster OSDs are being migrated', | ||
| severity: 'current', |
There was a problem hiding this comment.
no need to translate the text?
| state: AlertStates.Silenced, | ||
| annotations: { | ||
| description: | ||
| 'OSDs migration is successful. Your cluster is ready for a regional DR setup.', |
There was a problem hiding this comment.
translate the text
| 'OSDs migration is successful. Your cluster is ready for a regional DR setup.', | ||
| }, | ||
| labels: { | ||
| alertname: 'Cluster ready for Regional-DR setup', |
There was a problem hiding this comment.
translate the text
| }, | ||
| labels: { | ||
| alertname: 'Cluster ready for Regional-DR setup', | ||
| severity: 'passed', |
There was a problem hiding this comment.
translate the text
There was a problem hiding this comment.
keep severity under enum
| <AlertsBody> | ||
| <CustomAlertItem | ||
| alert={migrationAlert as any} | ||
| documentationLink={documentationLink} | ||
| docHelperText={docHelperText} | ||
| onClose={handleAlertClose} | ||
| icon={icon} | ||
| /> | ||
| </AlertsBody> |
There was a problem hiding this comment.
better keep this out of the alert card, use separate component
| @@ -189,7 +289,7 @@ export const StatusCard: React.FC = () => { | |||
| </GalleryItem> | |||
| </Gallery> | |||
| </HealthBody> | |||
| <CephAlerts /> | |||
| <CephAlerts data={data} /> | |||
There was a problem hiding this comment.
dont keep this part of alert card
|
@TimothyAsirJeyasing Also, please check why the build is failing. And every PR in UI needs Unit testing and e2e testing. |
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
from
e4fd09f to
ca42c91
Compare
|
|
| @@ -33,6 +33,7 @@ import { | |||
| CardHeader, | |||
| CardTitle, | |||
| } from '@patternfly/react-core'; | |||
| import CephClusterOSDMigrationStatus from '../../../modals/osd-migration/osd-migration-status'; | |||
There was a problem hiding this comment.
| import CephClusterOSDMigrationStatus from '../../../modals/osd-migration/osd-migration-status'; | |
| import {OSDMigrationStatus} from '../../../modals/osd-migration/osd-migration-status'; |
| @@ -189,6 +190,7 @@ export const StatusCard: React.FC = () => { | |||
| </GalleryItem> | |||
| </Gallery> | |||
| </HealthBody> | |||
| <CephClusterOSDMigrationStatus data={data} /> | |||
There was a problem hiding this comment.
| <CephClusterOSDMigrationStatus data={data} /> | |
| <OSDMigrationStatus data={cephDetails} /> |
There was a problem hiding this comment.
OSD migration details are coming from data.storage.... Whereas cephDetails are pointing to data.status
| import { InProgressIcon } from '@patternfly/react-icons'; | ||
| import { OSDMigrationStatus, getOSDMigrationStatus } from '../../utils/ocs'; | ||
|
|
||
| const CephClusterOSDMigrationStatus: React.FC<CephClusterOSDMigrationStatusProps> = |
There was a problem hiding this comment.
| const CephClusterOSDMigrationStatus: React.FC<CephClusterOSDMigrationStatusProps> = | |
| const OSDMigrationStatus: React.FC<OSDMigrationStatusProps> = |
| const CephClusterOSDMigrationStatus: React.FC<CephClusterOSDMigrationStatusProps> = | ||
| ({ data }) => { | ||
| const { t } = useCustomTranslation(); | ||
| const osdMigrationStatus = getOSDMigrationStatus(data[0]); |
There was a problem hiding this comment.
| const osdMigrationStatus = getOSDMigrationStatus(data[0]); | |
| const osdMigrationStatus = getOSDMigrationStatus(data); |
There was a problem hiding this comment.
change to migrationStatus or status
packages/ocs/utils/ocs.ts
Outdated
| const BLUESTORE_RDR = 'bluestore-rdr'; | ||
| const BLUESTORE = 'bluestore'; | ||
|
|
||
| export const getOSDMigrationStatus = (ceph: CephClusterKind) => { |
There was a problem hiding this comment.
const OSDMigrationModal: React.FC = ({ cephData }) => {
There was a problem hiding this comment.
move this inside utils folder under ocs.ts, And rename this function as getOSDMigrationStatus
| .mco-osd-migration-description { | ||
| display: flex; | ||
| flex-direction: column; | ||
| margin-left: 15px; | ||
| } |
There was a problem hiding this comment.
| .mco-osd-migration-description { | |
| display: flex; | |
| flex-direction: column; | |
| margin-left: 15px; | |
| } | |
| use <Flux> component |
|
|
||
| export default CephClusterOSDMigrationStatus; | ||
|
|
||
| type CephClusterOSDMigrationStatusProps = { |
There was a problem hiding this comment.
| type CephClusterOSDMigrationStatusProps = { | |
| type OSDMigrationStatusProps = { |
| export default CephClusterOSDMigrationStatus; | ||
|
|
||
| type CephClusterOSDMigrationStatusProps = { | ||
| data?: K8sResourceKind[]; |
There was a problem hiding this comment.
| data?: K8sResourceKind[]; | |
| data?: CephClusterKind; |
| ); | ||
| }; | ||
|
|
||
| export default CephClusterOSDMigrationStatus; |
There was a problem hiding this comment.
| export default CephClusterOSDMigrationStatus; |
| import { InProgressIcon } from '@patternfly/react-icons'; | ||
| import { OSDMigrationStatus, getOSDMigrationStatus } from '../../utils/ocs'; | ||
|
|
||
| const CephClusterOSDMigrationStatus: React.FC<CephClusterOSDMigrationStatusProps> = |
There was a problem hiding this comment.
| const CephClusterOSDMigrationStatus: React.FC<CephClusterOSDMigrationStatusProps> = | |
| export const CephClusterOSDMigrationStatus: React.FC<CephClusterOSDMigrationStatusProps> = |
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
from
ca42c91 to
5ef290c
Compare
| @@ -189,6 +190,7 @@ export const StatusCard: React.FC = () => { | |||
| </GalleryItem> | |||
| </Gallery> | |||
| </HealthBody> | |||
| <OSDMigrationStatus data={data[0]} /> | |||
There was a problem hiding this comment.
<OSDMigrationStatus data={data?.[0]}
| @@ -189,6 +190,7 @@ export const StatusCard: React.FC = () => { | |||
| </GalleryItem> | |||
| </Gallery> | |||
| </HealthBody> | |||
| <OSDMigrationStatus data={data[0]} /> | |||
There was a problem hiding this comment.
| <OSDMigrationStatus data={data[0]} /> | |
| <OSDMigrationStatus data={data?.[0]} /> |
| .mco-osd-migration { | ||
| margin-left: 5px; | ||
| } |
There was a problem hiding this comment.
| .mco-osd-migration { | |
| margin-left: 5px; | |
| } |
| @@ -0,0 +1,117 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
osd-migration-status.tsx is not a modal, dont keep this file inside a modal folder
| @@ -0,0 +1,117 @@ | |||
| import * as React from 'react'; | |||
| import { DOC_LINKS, ViewDocumentation } from '@odf/mco/utils'; | |||
There was a problem hiding this comment.
Don't import anything from MCO. if you want to use doc_utils then move it to shared and refactor doc-utils.ts. Also, It should not have any info about MCO or ODF.
| import { | ||
| COMPLETED, | ||
| PENDING, | ||
| FAILED, | ||
| BLUESTORE, | ||
| BLUESTORE_RDR, | ||
| getOSDMigrationStatus, | ||
| } from '../../utils/osd-migration'; |
There was a problem hiding this comment.
keep these inside enum and move the enum to dataProtection.ts
| doclink={DOC_LINKS.APPLY_POLICY} | ||
| /> | ||
| {t( | ||
| ` ${osdMigrationData.percentageComplete}% completed (${osdMigrationData.migratedDevices}/${osdMigrationData.totalOsd} remaining)` |
There was a problem hiding this comment.
| ` ${osdMigrationData.percentageComplete}% completed (${osdMigrationData.migratedDevices}/${osdMigrationData.totalOsd} remaining)` | |
This is not the right way to translate, please refer other places
| {migrationStatus === COMPLETED && ( | ||
| <FlexItem> | ||
| <Button variant="plain" aria-label="Close" onClick={handleClose}> | ||
| <span aria-hidden>✕</span> |
There was a problem hiding this comment.
| <span aria-hidden>✕</span> | |
what is this X ?
There was a problem hiding this comment.
i think you are using a wrong patternfly component to implement this
There was a problem hiding this comment.
you should use X icon
| }; | ||
|
|
||
| type OSDMigrationStatusProps = { | ||
| data?: CephClusterKind; |
There was a problem hiding this comment.
| data?: CephClusterKind; | |
| cephData?: CephClusterKind; |
packages/ocs/utils/osd-migration.ts
Outdated
| export const IN_PROGRESS = 'In Progress'; | ||
| export const PENDING = 'Pending'; | ||
| export const COMPLETED = 'Completed'; | ||
| export const FAILED = 'Failed'; |
There was a problem hiding this comment.
convert this status into Enum and move inside constant folder
packages/ocs/utils/osd-migration.ts
Outdated
| export const BLUESTORE_RDR = 'bluestore-rdr'; | ||
| export const BLUESTORE = 'bluestore'; |
There was a problem hiding this comment.
move inside constant folder
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
from
5ef290c to
092577f
Compare
| import { | ||
| ViewDocumentation, | ||
| DOC_LINKS, | ||
| } from '../../../../../shared/src/doc/doc-utils'; |
There was a problem hiding this comment.
| } from '../../../../../shared/src/doc/doc-utils'; | |
| } from '@odf/shared/doc/doc-utils'; |
packages/mco/utils/index.ts
Outdated
| @@ -1,3 +1,3 @@ | |||
| export * from './disaster-recovery'; | |||
| export * from './common'; | |||
| export * from './doc-utils'; | |||
| export * from '../../shared/src/doc/doc-utils'; | |||
There was a problem hiding this comment.
| export * from '../../shared/src/doc/doc-utils'; |
| const [migratedDevices, totalOsd, percentageComplete] = React.useMemo( | ||
| () => calculateOSDMigration(cephData), | ||
| [cephData] | ||
| ); | ||
|
|
||
| const migrationStatus: string = React.useMemo( | ||
| () => getOSDMigrationStatus(cephData), | ||
| [cephData] | ||
| ); |
There was a problem hiding this comment.
| const [migratedDevices, totalOsd, percentageComplete] = React.useMemo( | |
| () => calculateOSDMigration(cephData), | |
| [cephData] | |
| ); | |
| const migrationStatus: string = React.useMemo( | |
| () => getOSDMigrationStatus(cephData), | |
| [cephData] | |
| ); | |
| const migrationStatus: StatusType= React.useMemo( | |
| () => ({ | |
| ...calculateOSDMigration(cephData), | |
| status: getOSDMigrationStatus(cephData) | |
| }) | |
| [cephData] | |
| ); |
you can do this inside one useMemo and return as a single object. Becuase when cephData changed, both the hooks will be triggered.
There was a problem hiding this comment.
const StatusType = {
migratedDevices: number;
totalOsd: number;
percentageComplete: number;
status: OSD_Migration_Status;
};
| }; | ||
|
|
||
| type OSDMigrationStatusProps = { | ||
| cephData?: CephClusterKind; |
There was a problem hiding this comment.
| cephData?: CephClusterKind; | |
| cephData: CephClusterKind; |
| export enum osdMigration { | ||
| IN_PROGRESS = 'In Progress', | ||
| PENDING = 'Pending', | ||
| COMPLETED = 'Completed', | ||
| FAILED = 'Failed', | ||
| } |
There was a problem hiding this comment.
| export enum osdMigration { | |
| IN_PROGRESS = 'In Progress', | |
| PENDING = 'Pending', | |
| COMPLETED = 'Completed', | |
| FAILED = 'Failed', | |
| } | |
| export enum OSD_MIGRATION_STATUS { | |
| IN_PROGRESS = 'In Progress', | |
| PENDING = 'Pending', | |
| COMPLETED = 'Completed', | |
| FAILED = 'Failed', | |
| } |
There was a problem hiding this comment.
done as per 1st PR review suggestion to this name
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
from
092577f to
862c3ac
Compare
| import { cleanup, render, screen, waitFor } from '@testing-library/react'; | ||
| import '@testing-library/jest-dom/extend-expect'; | ||
| import { MemoryRouter } from 'react-router-dom'; | ||
| import * as migrationStatus from '../../../../utils/osd-migration'; |
There was a problem hiding this comment.
| import * as migrationStatus from '../../../../utils/osd-migration'; | |
| import { getOSDMigrationStatus } from '../../../../utils/osd-migration'; |
There was a problem hiding this comment.
doc-utils.ts has a lot of MCO and ACM-related constants, you have to move these to MCO constants. Shared component should be generic.
| )} | ||
|
|
||
| {migrationStatus === OSDMigrationStatus.FAILED && ( | ||
| <> |
There was a problem hiding this comment.
Could you give some more light on this
| /> | ||
| <ViewDocumentation | ||
| text={t('Setting up disaster recovery')} | ||
| doclink={DOC_LINKS.APPLY_POLICY} |
There was a problem hiding this comment.
Why we are using the apply policy document here? is this discussed with UX?
| )} | ||
|
|
||
| {migrationStatus === OSDMigrationStatus.FAILED && ( | ||
| <> |
There was a problem hiding this comment.
Could you give some more light on this
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
2 times, most recently
from
a0ebcb6 to
d04b1f5
Compare
| {[ | ||
| OSDMigrationStatus.IN_PROGRESS, | ||
| OSDMigrationStatus.COMPLETED, | ||
| OSDMigrationStatus.FAILED, | ||
| ].includes(migrationStatus as OSDMigrationStatus) && <Divider />}{' '} |
There was a problem hiding this comment.
if we are not displaying the divider only for the pending, then lets check !OSDMigrationStatus.PENDING.
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
from
d04b1f5 to
e6bf1fb
Compare
| }; | ||
|
|
||
| return ( | ||
| isMigrationStatusVisible && ( |
There was a problem hiding this comment.
can users close pending and failed status as well ?? (isn't UX only allow it for completed status ??
There was a problem hiding this comment.
the close icon will be available only for complete status, so user will not have any close options for other status.
There was a problem hiding this comment.
but that's not happening here, right ?
u are showing it for all statuses...
| }; | ||
|
|
||
| return ( | ||
| isMigrationStatusVisible && ( |
There was a problem hiding this comment.
this will not work, what if I close dashboard and visit it again (FC will be un-mounted and mounted again), isMigrationStatusVisible will again be set to true and u will see both status and close button...
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
from
e6bf1fb to
64f24be
Compare
| React.useEffect(() => { | ||
| // If it's the initial mount and the status is COMPLETED, hide the migration status | ||
| if ( | ||
| !isMigrationStatusVisible && | ||
| migrationStatus === OSDMigrationStatus.COMPLETED | ||
| ) { | ||
| return; | ||
| } | ||
| // For updates, trigger visibility | ||
| setIsMigrationStatusVisible(true); |
There was a problem hiding this comment.
did u test ur PR ?? this is not gonna make any difference to the original issue we discussed #1097 (comment)
There was a problem hiding this comment.
this is not correct...
There was a problem hiding this comment.
maybe we can remove close button entirely (discussed with @GowthamShanmugam), need to be verified with UX (asap)...
There was a problem hiding this comment.
for now let's remove it and if needed to absolutely add it we can do so as a bug fix... no need to block this PR any longer...
There was a problem hiding this comment.
Sure, removed the button and updated
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
2 times, most recently
from
02d1b20 to
b596ace
Compare
|
LGTM |
| return [migratedDevices, totalOsd, percentageComplete]; | ||
| }; | ||
|
|
||
| export const OSDMigration: React.FC<OSDMigrationStatusProps> = ({ |
There was a problem hiding this comment.
to match it with file name and props' type name...
| export const OSDMigration: React.FC<OSDMigrationStatusProps> = ({ | |
| export const OSDMigrationStatus: React.FC<OSDMigrationStatusProps> = ({ |
There was a problem hiding this comment.
OSDMigrationStatus name used for "enum OSDMigrationStatus" in data-protection.ts and its used here, So any suggestion please ...
There was a problem hiding this comment.
so ? rename the enum or rename ur props' type/file name to match with the FC name...
| doclink: string; | ||
| text?: string; | ||
| padding?: string; | ||
| }; |
There was a problem hiding this comment.
export from index file...
| @@ -1,8 +1,9 @@ | |||
| import * as React from 'react'; | |||
| import { DOC_LINKS } from '@odf/mco/constants/doc'; | |||
| import { ViewDocumentation } from '@odf/shared/utils/doc-utils'; | |||
There was a problem hiding this comment.
| import { ViewDocumentation } from '@odf/shared/utils/doc-utils'; | |
| import { ViewDocumentation } from '@odf/shared/utils'; |
| @@ -1,8 +1,9 @@ | |||
| import * as React from 'react'; | |||
| import { ViewDocumentation } from '@odf/shared/utils/doc-utils'; | |||
There was a problem hiding this comment.
| import { ViewDocumentation } from '@odf/shared/utils/doc-utils'; | |
| import { ViewDocumentation } from '@odf/shared/utils'; |
| import { RedExclamationCircleIcon } from '@odf/shared/status/icons'; | ||
| import { CephClusterKind } from '@odf/shared/types'; | ||
| import { useCustomTranslation } from '@odf/shared/useCustomTranslationHook'; | ||
| import { ViewDocumentation } from '@odf/shared/utils/doc-utils'; |
There was a problem hiding this comment.
| import { ViewDocumentation } from '@odf/shared/utils/doc-utils'; | |
| import { ViewDocumentation } from '@odf/shared/utils'; |
| } | ||
|
|
||
| export const BLUESTORE_RDR = 'bluestore-rdr'; | ||
| export const BLUESTORE = 'bluestore'; |
There was a problem hiding this comment.
plz rename this file to data-protection
Signed-off-by: Timothy Asir Jeyasingh <[email protected]>
TimothyAsirJeyasing
force-pushed
the
brownfiled-migration-status-alert
branch
from
b596ace to
50e0947
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GowthamShanmugam, SanjalKatiyar, TimothyAsirJeyasing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test odf-console-e2e-aws |
openshift-merge-bot
bot
merged commit 9b55fef
into
red-hat-storage:master
No description provided.