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.

Sign up for GitHub

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

Jump to bottom

Bug 1871742: can't delete a PVC created using a data upload #6407

Merged
merged 1 commit into from Sep 1, 2020
Merged

Bug 1871742: can't delete a PVC created using a data upload #6407

merged 1 commit into from Sep 1, 2020

Conversation

glekner
Copy link
Contributor

@glekner glekner commented Aug 24, 2020 •
edited

For Normal PVCs the modal is the same:
Screen Shot 2020-08-24 at 16 36 25

For CDI Bound PVCs the modal is:
Screen Shot 2020-08-24 at 16 36 32

@matthewcarleton how do we want this to look?

@openshift-ci-robot
Copy link
Contributor

@glekner: This pull request references Bugzilla bug 1871742, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1871742: can't delete a PVC created using a data upload

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality labels Aug 24, 2020
@openshift-ci-robot openshift-ci-robot added component/kubevirt Related to kubevirt-plugin component/sdk Related to console-plugin-sdk labels Aug 24, 2020
@glekner
Copy link
Contributor Author

glekner commented Aug 24, 2020

@suomiy @rawagner

@openshift-ci-robot
Copy link
Contributor

@glekner: This pull request references Bugzilla bug 1871742, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1871742: can't delete a PVC created using a data upload

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jelkosz
Copy link

jelkosz commented Aug 24, 2020

I like it!

rawagner
rawagner suggested changes Aug 24, 2020
View reviewed changes
frontend/packages/console-plugin-sdk/src/typings/pvc.ts Outdated

export interface PVCDelete<T> {
/** predicate that tells if to use the extension or not */
predicate: (pvc: K8sResourceCommon) => boolean;
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
predicate: (pvc: K8sResourceCommon) => boolean;
predicate: (pvc: PersistentVolumeClaimKind) => boolean;

frontend/packages/console-plugin-sdk/src/typings/pvc.ts Outdated
/** predicate that tells if to use the extension or not */
predicate: (pvc: K8sResourceCommon) => boolean;
/** method for the pvc delete operation */
onPVCKill: (pvc: K8sResourceCommon) => Promise<void>;
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
onPVCKill: (pvc: K8sResourceCommon) => Promise<void>;
onPVCKill: (pvc: PersistentVolumeClaimKind) => Promise<void>;

frontend/packages/console-plugin-sdk/src/typings/pvc.ts Outdated
/** method for the pvc delete operation */
onPVCKill: (pvc: K8sResourceCommon) => Promise<void>;
/** alert for additional info */
alertLoader?: LazyLoader<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for generics. We know we always pass pvc object

Suggested change
alertLoader?: LazyLoader<T>;
alertLoader?: LazyLoader<{pvc: PersistentVolumeClaimKind}>;

maybe extract the type and export it so the extension consumers can reuse it

frontend/packages/kubevirt-plugin/src/components/cdi-upload-provider/pvc-delete-extension.tsx Outdated
</Alert>
);

type PVCDeleteAlertExtension = {
Copy link
Contributor

Choose a reason for hiding this comment

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

you wont have to define your own type once you remove the generics as I mentioned above

frontend/public/components/modals/delete-pvc-modal.tsx Outdated
import { PersistentVolumeClaimModel } from '../../models';
import { isPVCDelete, PVCDelete, useExtensions } from '@console/plugin-sdk';

const DeletePVCModal = withHandlePromise((props: DeletePVCModalProps) => {
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 DeletePVCModal = withHandlePromise((props: DeletePVCModalProps) => {
const DeletePVCModal = withHandlePromise<DeletePVCModalProps>((props) => {

frontend/public/components/modals/delete-pvc-modal.tsx Outdated
};

const alertComponents = pvcDeleteExtensions.map(
({ properties: { predicate, alertLoader } }, i) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

we have uid available now

Suggested change
({ properties: { predicate, alertLoader } }, i) =>
({ properties: { predicate, alertLoader }, uid }) =>
predicate(pvc) && <StackItem key={uid}><AsyncComponent loader={alertLoader} pvc={pvc} /></StackItem>,

frontend/public/components/modals/delete-pvc-modal.tsx Outdated
});

export type DeletePVCModalProps = {
pvc: K8sResourceCommon;
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
pvc: K8sResourceCommon;
pvc: PersistentVolumeClaim;

frontend/public/components/modals/delete-pvc-modal.tsx Outdated
</ModalTitle>
<ModalBody>
<Stack hasGutter>
{alertComponents.map((alert, i) => (
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
{alertComponents.map((alert, i) => (
{alertComponents}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to render a margin between every alert <StackItem>, so I need this map.

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment #6407 (comment)

frontend/packages/kubevirt-plugin/src/components/cdi-upload-provider/pvc-delete-extension.tsx Outdated

export const killCDIBoundPVC = (pvc: K8sResourceKind) => k8sKill(DataVolumeModel, pvc);

export const PVCDeleteAlertExtension: React.FC<PVCDeleteAlertExtension> = ({ pvc }) => (
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 also consider that the extension would define alertTitle: string, alertBody: LazyLoader<{pvc: PersistentVolumeClaimKind}>, alertType: AlertVariant.

It makes the extension less error prone, you dont have to specify className, isInline etc..all would be handled by pvc delete modal

@matthewcarleton
Copy link
Contributor

For Normal PVCs the modal is the same:
Screen Shot 2020-08-24 at 16 36 25

For CDI Bound PVCs the modal is:
Screen Shot 2020-08-24 at 16 36 32

@matthewcarleton how do we want this to look?

We need to ensure that if the PVC is used to back any OS then we highlight that here. It's possible that a user could delete this without knowing that's being cloned as an OS source somewhere else right?

@glekner
Copy link
Contributor Author

glekner commented Aug 25, 2020

/retest

@glekner
Copy link
Contributor Author

glekner commented Aug 25, 2020

fixed suggestions @rawagner
@matthewcarleton well yes, deleting is a serious thing but I don't think we should check for every VM that uses the specific PVC. We could maybe check if that PVC is a golden one and let the user know its important?

@matthewcarleton
Copy link
Contributor

fixed suggestions @rawagner
@matthewcarleton well yes, deleting is a serious thing but I don't think we should check for every VM that uses the specific PVC. We could maybe check if that PVC is a golden one and let the user know its important?

Ya that's what I was thinking. If it's that important we shouldn't assume the user is thinking about it that way. It would be a nice addition to alert them that is being used to support and OS as a backing source.

@glekner
Copy link
Contributor Author

glekner commented Aug 30, 2020

@matthewcarleton can you approve the text here?
Screen Shot 2020-08-30 at 12 27 13

@glekner
Copy link
Contributor Author

glekner commented Aug 30, 2020

/retest

@jelkosz
Copy link

jelkosz commented Aug 31, 2020

@glekner I dont think it will cause any malfunction. We clone the image. It will cause that the other VMs will not be able to use it but the ones which have already been deployed will be fine. Or am I missing something?

rawagner
rawagner reviewed Aug 31, 2020
View reviewed changes
frontend/public/components/modals/delete-pvc-modal.tsx Outdated
const alertComponents = pvcDeleteExtensions.map(
({ properties: { predicate, alert }, uid }) =>
predicate(pvc) && (
<Alert
Copy link
Contributor

Choose a reason for hiding this comment

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

If you wrap the alert in StackItem you wont need to map again later.

Suggested change
<Alert
<StackItem key={uid}>
<Alert

you can also move key from Alert to StackItem. There's also no need to make key more complicated than just uid. Doing pvc-alert-${uid} doesnt bring any value

frontend/packages/kubevirt-plugin/src/components/cdi-upload-provider/pvc-delete-extension.tsx Outdated
},
});

export const PVCDeleteAlertExtension = ({ pvc }) => {
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
export const PVCDeleteAlertExtension = ({ pvc }) => {
export const PVCDeleteAlertExtension: React.FC<{ pvc: PersistentVolumeClaimKind }> = ({ pvc }) => {

frontend/packages/console-plugin-sdk/src/typings/pvc.ts Outdated
/** alert for additional info */
alert?: {
/** alert title */
alertTitle: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

could be just title, 'type', 'Body'. Not a blocker though :)

@glekner
Copy link
Contributor Author

glekner commented Aug 31, 2020

/retest

@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 31, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

14 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@rawagner
Copy link
Contributor

rawagner commented Sep 1, 2020

/hold

needs rebase

@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 Sep 1, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2020
@glekner
Copy link
Contributor Author

glekner commented Sep 1, 2020

/retest

@rawagner
Copy link
Contributor

rawagner commented Sep 1, 2020

/hold cancel
/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 1, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, 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-merge-robot openshift-merge-robot merged commit 21e2880 into openshift:master Sep 1, 2020
@openshift-ci-robot
Copy link
Contributor

@glekner: All pull requests linked via external trackers have merged:

Bugzilla bug 1871742 has been moved to the MODIFIED state.

In response to this:

Bug 1871742: can't delete a PVC created using a data upload

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@spadgett spadgett added this to the v4.6 milestone Sep 1, 2020
This was referenced Sep 30, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rawagner rawagner rawagner requested changes

@spadgett spadgett Awaiting requested review from spadgett

@atiratree atiratree Awaiting requested review from atiratree

Assignees

@rawagner rawagner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality component/kubevirt Related to kubevirt-plugin component/sdk Related to console-plugin-sdk lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.6
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@glekner @openshift-ci-robot @jelkosz @matthewcarleton @rawagner @openshift-bot @spadgett @openshift-merge-robot