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

Add option to create snapshot for PVC #5893

Merged
merged 1 commit into from Jul 21, 2020

Conversation

a2batic
Copy link
Contributor

@a2batic a2batic commented Jul 5, 2020

'Create Snapshot` action in kebab menu for PVC will allow
user to create in-time images of snapshot.

Screenshot from 2020-07-08 02-20-20

@bipuladh @cloudbehl
Signed-off-by: Kanika kmurarka@redhat.com

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/ceph Related to ceph-storage-plugin labels Jul 5, 2020
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Jul 5, 2020
@a2batic
Copy link
Contributor Author

a2batic commented Jul 5, 2020

/assign @rawagner @gnehapk @afreen23

@a2batic a2batic marked this pull request as draft July 5, 2020 20:40
@a2batic a2batic marked this pull request as ready for review July 5, 2020 20:41
@cloudbehl
Copy link
Contributor

@a2batic

  1. In PVC details we should have a name also for the PVC.
  2. Creating a snapshot for {PVC} Claim? Also, we should point on the it will take time for snapshot to be in ready state.
  3. Filtering snapshot classes by PVC provisioner is something we need to do.
  4. Save should be Create to be consistent.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020
@a2batic a2batic changed the title [WIP] Add option to create snapshot for PVC Add option to create snapshot for PVC Jul 9, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2020
@cloudbehl
Copy link
Contributor

Folder name inside the console app should be volume-snapshot & it should hold all the related components. I see some inconsistency in your & bipul's PR's related to naming.

}

.co-volume-snapshot__form {
margin: 20px 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use PF variables, also for co-volume-snapshot__details-body

namespace: string;
pvc: string;
status: string;
persistentVolumeClaim: K8sResourceKind;
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to create a PersistentVolumeClaimKind

export const PVCDropdown: React.FC<PVCDropdownProps> = (props) => {
const kind = 'PersistentVolumeClaim';
const { namespace, selectedKey } = props;
const resources = [{ kind, namespace }];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const resources = [{ kind, namespace }];
const resources = [{ PersistentVolumeClaimModel.kind, namespace }];

);
};

// eslint-disable-next-line no-underscore-dangle
Copy link
Contributor

Choose a reason for hiding this comment

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

you should not disable the rule
the VolumeSnapshot_ can be written without dongle (and the current one is also missing types)

Suggested change
// eslint-disable-next-line no-underscore-dangle
export const VolumeSnapshot = connectToPlural<VolumeSnapshotProps>(
({ kindObj, kindsInFlight, match: { params } }) => {
if (!kindObj && kindsInFlight) {
return <LoadingBox />;
}
return (
<CreateSnapshotForm namespace={params.ns} resourceName={params.name} kindObj={kindObj} />
);
},
);
type VolumeSnapshotProps = WithPluralProps & {
match: match<{ name: string; ns: string }>;
};

const { selectedKey, pvcProvisioner } = props;
const resources = [{ kind }];
const provisionerFilter = (snapshotClass: VolumeSnapshotClassKind) => {
return pvcProvisioner && pvcProvisioner.includes(snapshotClass?.driver);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it can be returned directly. No need to use return.

const [snapshotClassName, setSnapshotClassName] = React.useState<string>();
const [error, setError] = React.useState('');
const [inProgress, setInProgress] = React.useState(false);
const [pvcStatus, setPVCStatus] = React.useState<string>(null);
Copy link
Contributor

@gnehapk gnehapk Jul 11, 2020

Choose a reason for hiding this comment

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

Shouldn't this be initialized with empty string instead of null here? I don't see that the value null is being used anywhere. Same for pvcProvisioner

event.preventDefault();
setInProgress(true);
const snapshotTemplate: K8sResourceKind = {
apiVersion: `${VolumeSnapshotModel.apiGroup}/${VolumeSnapshotModel.apiVersion}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
apiVersion: `${VolumeSnapshotModel.apiGroup}/${VolumeSnapshotModel.apiVersion}`,
apiVersion: `apiVersionForModel(VolumeSnapshotModel)

}
};

setValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setValue();
updateValues();

})
.catch((err) => {
setError(err.message || 'Could not create volume snapshot');
setInProgress(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can create a finally block for setInProgress(false)

.finally(() => setInProgress(false);

});
};

const title = 'Create VolumeSnapshot';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this variable definition after states declaration

Creating snapshot for claim <strong>{resourceName}</strong>
</p>
)}
{pvcStatus && pvcStatus !== 'Bound' && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can directly use pvcStatus !== 'Bound' here once you define the pvsStatus with empty string

@bipuladh
Copy link
Contributor

/hold
Let's push it after #5902 as pushing this without the list page doesn't make sense.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2020

export type VolumeSnapshotClassKind = K8sResourceCommon & {
driver: string;
deletePolicy: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
deletePolicy: string;
deletionPolicy: string;

Comment on lines 18 to 28
@media (max-width: 769px) {
display: none;
}

@media (max-width: 992px) {
display: none;
}

@media (min-width: 1200px) {
max-width: 50%;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume .pf-u-display-none and its breakpoint based variants should do the trick?

} from '@console/internal/models';
import { PVCDropdown } from '@console/internal/components/utils/pvc-dropdown';

import './_create-volume-snapshot.scss';
Copy link
Contributor

Choose a reason for hiding this comment

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

Move it to the end of the import declarations.

persistentVolumeClaim,
}) => {
const storageClass = persistentVolumeClaim?.spec?.storageClassName || '-';
const requestedCapacity = persistentVolumeClaim?.status?.capacity?.storage || '-';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we get these values as G/T/Mi let's convert to G/T/MiB before showing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We get the value as a string (eg. 1Gi ), so we need not convert.

Copy link
Contributor

@bipuladh bipuladh Jul 14, 2020

Choose a reason for hiding this comment

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

I understand that but when we are displaying it to the user lets show it with B since that's the console standard.

status,
persistentVolumeClaim,
}) => {
const storageClass = persistentVolumeClaim?.spec?.storageClassName || '-';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const storageClass = persistentVolumeClaim?.spec?.storageClassName || '-';
const storageClass = persistentVolumeClaim?.spec?.storageClassName;

Isn't SC a required field for PVC creation?

Comment on lines 239 to 240
status={pvcStatus}
pvc={pvcName}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
status={pvcStatus}
pvc={pvcName}

setPVCStatus(pvc?.metadata?.deletionTimestamp ? 'Terminating' : pvc?.status?.phase);
setPVCProvisioner(sc?.provisioner);
} catch (err) {
setError(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't think we should be showing the error if we cannot fetch the PVC list.

Comment on lines 113 to 128
const setValue = async () => {
try {
const pvc = await fetchK8s<K8sResourceKind>(PersistentVolumeClaimModel, pvcName, namespace);
const sc = await fetchK8s<StorageClassResourceKind>(
StorageClassModel,
pvc?.spec?.storageClassName,
);
setPVCObj(pvc);
setPVCStatus(pvc?.metadata?.deletionTimestamp ? 'Terminating' : pvc?.status?.phase);
setPVCProvisioner(sc?.provisioner);
} catch (err) {
setError(err);
}
};

setValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const setValue = async () => {
try {
const pvc = await fetchK8s<K8sResourceKind>(PersistentVolumeClaimModel, pvcName, namespace);
const sc = await fetchK8s<StorageClassResourceKind>(
StorageClassModel,
pvc?.spec?.storageClassName,
);
setPVCObj(pvc);
setPVCStatus(pvc?.metadata?.deletionTimestamp ? 'Terminating' : pvc?.status?.phase);
setPVCProvisioner(sc?.provisioner);
} catch (err) {
setError(err);
}
};
setValue();
( async () => {
try {
const pvc = await fetchK8s<K8sResourceKind>(PersistentVolumeClaimModel, pvcName, namespace);
const sc = await fetchK8s<StorageClassResourceKind>(
StorageClassModel,
pvc?.spec?.storageClassName,
);
setPVCObj(pvc);
setPVCStatus(pvc?.metadata?.deletionTimestamp ? 'Terminating' : pvc?.status?.phase);
setPVCProvisioner(sc?.provisioner);
} catch (err) {
setError(err);
}
})();


@media (max-width: 992px) {
display: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't 992 cover 769 as well?

}
if (resourceKind?.kind === VolumeSnapshotModel.kind) {
return [RestorePVC, DeleteSnapshot];
return [ClonePVC];
Copy link
Contributor

Choose a reason for hiding this comment

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

@cloudbehl I hope you have considered the creation of clone in your PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes & removal of stale objects is also taken care.

@bipuladh
Copy link
Contributor

/hold cancel
#5902 has merged.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@a2batic a2batic requested a review from bipuladh July 16, 2020 09:09
.then((sc) => setSCObj(sc))
.catch((error) =>
// eslint-disable-next-line no-console
console.error(error),
Copy link
Contributor

Choose a reason for hiding this comment

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

we should have a better error handling, console logging is not very user friendly

);
};

const CreateSnapshotForm = withHandlePromise((props: SnapshotResourceProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const CreateSnapshotForm = withHandlePromise((props: SnapshotResourceProps) => {
const CreateSnapshotForm = withHandlePromise<SnapshotResourceProps>((props) => {

};
}, [namespace]);

const [data, loaded, loadError] = useK8sWatchResource(resourceWatch);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [data, loaded, loadError] = useK8sWatchResource(resourceWatch);
const [data, loaded, loadError] = useK8sWatchResource<PersistentVolumeClaimKind[]>(resourceWatch);


React.useEffect(() => {
if (!_.isEmpty(data)) {
const currentPVC = (data as PersistentVolumeClaimKind[]).find(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const currentPVC = (data as PersistentVolumeClaimKind[]).find(
const currentPVC = data.find(

(pvc) => pvc.metadata.name === pvcName,
);
setPVCObj(currentPVC);
loadError || setError(loadError);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be outside of if. If error happens, data can be empty.

/* eslint-disable jsx-a11y/label-has-associated-control */
<>
<label className="control-label co-required" html-for="claimName">
Persistent Volume claim
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Persistent Volume claim
{PersistentVolumeClaimModel.label}


const handlePVCName = (name: string) => {
if (!_.isEmpty(data)) {
const currentPVC = (data as PersistentVolumeClaimKind[]).find(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const currentPVC = (data as PersistentVolumeClaimKind[]).find(
const currentPVC = data.find(

})
.catch((e) => {
// eslint-disable-next-line no-console
console.error(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

needs better handling

);
});

const VolumeSnapshotComponent = (props: VolumeSnapshotComponentProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const VolumeSnapshotComponent = (props: VolumeSnapshotComponentProps) => {
const VolumeSnapshotComponent: React.FC<VolumeSnapshotComponentProps> = (props) => {


React.useEffect(() => {
k8sGet(StorageClassModel, pvcSC)
.then((sc) => setSCObj(sc))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.then((sc) => setSCObj(sc))
.then(setSCObj)

const [pvcName, setPVCName] = React.useState(resourceName);
const [pvcObj, setPVCObj] = React.useState(null);
const [snapshotName, setSnapshotName] = React.useState(`${pvcName || 'pvc'}-snapshot`);
const [snapshotClassName, setSnapshotClassName] = React.useState<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [snapshotClassName, setSnapshotClassName] = React.useState<string>();
const [snapshotClassName, setSnapshotClassName] = React.useState<string>('');

const [pvcObj, setPVCObj] = React.useState(null);
const [snapshotName, setSnapshotName] = React.useState(`${pvcName || 'pvc'}-snapshot`);
const [snapshotClassName, setSnapshotClassName] = React.useState<string>();
const [error, setError] = React.useState(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [error, setError] = React.useState(null);
const [error, setError] = React.useState('');

Comment on lines 143 to 149
const resourceWatch = React.useMemo(() => {
return {
kind: PersistentVolumeClaimModel.kind,
namespace,
isList: true,
};
}, [namespace]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const resourceWatch = React.useMemo(() => {
return {
kind: PersistentVolumeClaimModel.kind,
namespace,
isList: true,
};
}, [namespace]);
const resourceWatch = React.useMemo(() => {
return Object.assign({
kind: PersistentVolumeClaimModel.kind,
namespace,
isList: true,
}, pvcName ? { name: pvcName } : null);
}, [namespace, pvcName]);

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2020
@a2batic a2batic force-pushed the snapshot-modal branch 2 times, most recently from 203b188 to 6f65827 Compare July 20, 2020 18:28
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2020
const { resourceName, namespace, kindObj, handlePromise, inProgress, errorMessage } = props;

const [pvcName, setPVCName] = React.useState(resourceName);
const [pvcObj, setPVCObj] = React.useState(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [pvcObj, setPVCObj] = React.useState(null);
const [pvcObj, setPVCObj] = React.useState<PersistentVolumeClaimKind>(null);

Comment on lines 179 to 181
match: {
params: {
ns: string;
plural: string;
};
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
match: {
params: {
ns: string;
plural: string;
};
};
};
match: match<{ns?: string, plural?:string}>;

Comment on lines 204 to 206
.catch((e) => {
throw e;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why catch just to throw again?

@a2batic
Copy link
Contributor Author

a2batic commented Jul 21, 2020

@bipuladh as per offline conversations, have resolved some comments, and addressed other comments. Please review.

@bipuladh
Copy link
Contributor

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
PersistentVolumeModel,
} from '@console/internal/models';
import { PVCDropdown } from '@console/internal/components/utils/pvc-dropdown';
import { apiVersionForModel } from '@console/kubevirt-plugin/integration-tests/tests/utils/selectors';
Copy link
Contributor

Choose a reason for hiding this comment

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

dont import from kubevirt-plugin

};

const PVCSummary: React.FC<PVCSummaryProps> = ({ persistentVolumeClaim }) => {
const accessModeLabels = Object.freeze({
Copy link
Contributor

Choose a reason for hiding this comment

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

can you reuse https://github.com/openshift/console/blob/master/frontend/public/components/storage/shared.ts#L21 ?

user to create in-time images of snapshot.

Jira: https://issues.redhat.com/browse/RHSTOR-1154
Signed-off-by: Kanika <kmurarka@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, bipuladh, cloudbehl, rawagner

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2020
@@ -991,3 +991,19 @@ export type VolumeSnapshotClassKind = K8sResourceCommon & {
deletionPolicy: string;
driver: string;
};

export type PersistentVolumeClaimKind = K8sResourceCommon & {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add source object to accommodate cloning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, as per offline converstaion, will be taking it up in follow-up PR.


const PVCSummary: React.FC<PVCSummaryProps> = ({ persistentVolumeClaim }) => {
const storageClass = persistentVolumeClaim?.spec?.storageClassName;
const requestedCapacity = persistentVolumeClaim?.spec?.resources?.requests?.storage;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This is required in the PVC object so we don't need to check all entities.

Suggested change
const requestedCapacity = persistentVolumeClaim?.spec?.resources?.requests?.storage;
const requestedCapacity = persistentVolumeClaim?.spec.resources.requests.storage;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, as per offline converstaion, will be taking it up in follow-up PR.

@openshift-merge-robot openshift-merge-robot merged commit 33ee022 into openshift:master Jul 21, 2020
@spadgett spadgett added this to the v4.6 milestone Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants