[RHSTOR-5140] Adds Ceph mon count update for 5 zones #1117
Conversation
bipuladh
changed the title
bipuladh
changed the title
bipuladh
force-pushed
the
multi-mons
branch
2 times, most recently
from
2da5945 to
9ccc69f
Compare
| const [errorMessage, setErrorMessage] = React.useState(''); | ||
| const [inProgress, setProgress] = React.useState(false); | ||
|
|
||
| //Todo(bipuladh): Update it to use information from the alert |
There was a problem hiding this comment.
sorry for adding such a strange suggestion on the PR, but I am adding comments prefixed with term // ToDo (epic 4422) so that it will be easier to search for all such instances later and make updates related to multiple storagecluster epic...
| //Todo(bipuladh): Update it to use information from the alert | |
| // ToDo (epic 4422) (bipuladh): Update it to use information from the alert |
| direction={{ default: 'column' }} | ||
| spaceItems={{ default: 'spaceItemsNone' }} | ||
| > | ||
| <FlexItem>{t('Failure domains: 5')}</FlexItem> |
There was a problem hiding this comment.
just asking, should we determine the count from the rack/zone labels and then display, instead of exact value ?? assuming there can be more than 5 racks/FD as well (as per epic's title)...
| spaceItems={{ default: 'spaceItemsNone' }} | ||
| > | ||
| <FlexItem>{t('Failure domains: 5')}</FlexItem> | ||
| <FlexItem>{t('Monitor count: 3')}</FlexItem> |
There was a problem hiding this comment.
same here, just asking, should we read it from StorageCluster CR ?? we are already polling for it...
There was a problem hiding this comment.
the alert will only be raised for scenarios when mon count is 3. so this is safe.
| setProgress(false); | ||
| }) | ||
| .catch((err) => { | ||
| setErrorMessage(err); |
There was a problem hiding this comment.
no need to add setProgress(false); for error case?
| <Trans t={t as any} ns="plugin__odf-console"> | ||
| To enhance system resilience, align Ceph monitors with the | ||
| available node failure zones. | ||
| </Trans> |
There was a problem hiding this comment.
why tag here? can't we use a t function?
There was a problem hiding this comment.
It's the same thing. Used it here because the sentence is a bit long.
There was a problem hiding this comment.
I agree it is the same, But I prefer to use the t function rather than <Trans> So we can change the ns anytime. In case of refactoring, it will be easy to change the ns. I use <Trans" only when I have any HTML tags in between.
It just an optional suggestion
| </Text> | ||
| </FlexItem> | ||
| </Flex> | ||
| {errorMessage && ( |
There was a problem hiding this comment.
| {errorMessage && ( | |
| {!!errorMessage && ( |
| </Flex> | ||
| {errorMessage && ( | ||
| <Alert isInline variant="danger" title={t('An error occurred')}> | ||
| {(errorMessage as any)?.message || errorMessage} |
There was a problem hiding this comment.
I prefer to create one common util function to fetch error messages in a shared folder. but it is just an optional suggestion.
There was a problem hiding this comment.
I can take it up in a different PR.
bipuladh
force-pushed
the
multi-mons
branch
2 times, most recently
from
bbd3835 to
06c94ec
Compare
| useSafeK8sGet(StorageClusterModel, null, odfNamespace); | ||
| const [nodes, nodesLoaded, nodesLoadError] = useK8sList<NodeKind>(NodeModel); | ||
| const failureDomains = | ||
| nodesLoaded && !nodesLoadError ? groupNodesByZones(nodes)?.length : []; |
There was a problem hiding this comment.
not for this PR, but make sense to rename the function later...
| nodesLoaded && !nodesLoadError ? groupNodesByZones(nodes)?.length : []; | |
| nodesLoaded && !nodesLoadError ? groupNodesByFailureDomain(nodes)?.length : []; |
|
/retest |
1 similar comment
|
/retest |
|
CI un-blocked by: #1125 (PR#1125 is needed because of OCP react-router-dom updates, else components using imports will break). |
Signed-off-by: Bipul Adhikari <[email protected]>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, SanjalKatiyar 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 |
openshift-merge-bot
bot
merged commit 79af2c7
into
red-hat-storage:master
Adds support for updating Ceph Mon count for 5 zones
Screenshot: