Skip to content

Adds CRUD support for VolumeGroupSnapshot #14886

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
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Adds CRUD support for VolumeGroupSnapshot #14886

wants to merge 3 commits into from

Conversation

vbnrh
Copy link
Contributor

@vbnrh vbnrh commented Mar 18, 2025 •
edited
Loading

This commit adds support for CRUD operations on
VolumeGroupSnapshot, VolumeGroupSnapshotClass, VolumeGroupSnapshotContent

Declares types for the above and adds list, details and creation page for VGS.

Design
https://www.figma.com/design/hOKRxynqZ6EXfOkIy8RaVf/Multi-volume-consistency---Lifecycle-management?node-id=2048-4744&p=f&t=tcU5kHtdNgEzmFFB-0

Epic:
https://issues.redhat.com/browse/RHSTOR-4926

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2025
@openshift-ci openshift-ci bot requested review from Mylanos and spadgett March 18, 2025 07:45
@openshift-ci openshift-ci bot added component/core Related to console core functionality component/shared Related to console-shared labels Mar 18, 2025
@vbnrh vbnrh force-pushed the volume-group-snapshot-creation-page branch from 4d8d84e to 0dde9ab Compare March 18, 2025 07:56
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2025
bipuladh
bipuladh reviewed Mar 18, 2025
View reviewed changes
...ges/console-app/src/components/volume-group-snapshot/volume-group-snapshot-class-details.tsx Outdated
editYaml(),
events(ResourceEventStream),
];
return <DetailsPage {...props} menuActions={Kebab.factory.common} pages={pages} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use ActionMenu. Please refer #14878

...nd/packages/console-app/src/components/volume-group-snapshot/volume-group-snapshot-class.tsx Outdated
<TableData {...tableColumnInfo[1]}>{driver}</TableData>
<TableData {...tableColumnInfo[2]}>{deletionPolicy}</TableData>
<TableData {...tableColumnInfo[3]}>
<ResourceKebab
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use LazyActionMenu
see #14878

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
frontend/packages/console-app/console-extensions.json Outdated
Comment on lines 1354 to 1280
"flags": {
"required": [
"CAN_LIST_VGSC"
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need flags for other nav-items (VolumeGroupSnapshots & VolumeGroupSnapshotClasses) ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"A VolumeSnapshotContent is a snapshot taken from a volume in the cluster that has been provisioned by an administrator. It is a resource in the cluster just like a PersistentVolume is a cluster resource."

https://kubernetes.io/docs/concepts/storage/volume-snapshots/

This is actually the representation of Snapshot content in the backend containing the data. Hence it needs to be protected. Other resources are more of a logical representation that don't need to be protected. That is how i understand it

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
frontend/packages/console-app/console-extensions.json Outdated
Comment on lines 172 to 176
=======
"component": {
"$codeRef": "oauthConfigDetailsPage.default"
}
>>>>>>> 4d8d84e63c (Adds CRUD support for VolumeGroupSnapshot)
Copy link
Contributor

Choose a reason for hiding this comment

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

merge conflicts were not properly resolved...

Copy link
Contributor

Choose a reason for hiding this comment

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

there are multiple such instances in this file...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, something is wrong with the editor. It somehow stopped highlighting the conflicts

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
frontend/packages/console-app/console-extensions.json Outdated
{
"type": "console.navigation/resource-cluster",
"properties": {
"id": "volumegroupsnapshotcontents",
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need perspective here ??

Suggested change
"id": "volumegroupsnapshotcontents",
"id": "volumegroupsnapshotcontents",
"perspective": "admin",

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
...nd/packages/console-app/src/components/volume-group-snapshot/volume-group-snapshot-class.tsx Outdated
<TableData {...tableColumnInfo[0]}>
<ResourceLink name={name} kind={referenceForModel(VolumeGroupSnapshotClassModel)}>
{isDefaultSnapshotClass(obj) && (
<span className="small text-muted co-resource-item__help-text">&ndash; Default</span>
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
<span className="small text-muted co-resource-item__help-text">&ndash; Default</span>
<span className="small text-muted co-resource-item__help-text">&ndash; {t('public~Default')}</span>

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
frontend/public/models/index.ts Outdated
Comment on lines 1269 to 1276
labelPlural: 'VolumeGroupSnapshotContents',
labelPluralKey: 'public~VolumeGroupSnapshotContents',
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
labelPlural: 'VolumeGroupSnapshotContents',
labelPluralKey: 'public~VolumeGroupSnapshotContents',
labelPlural: 'VolumeGroupSnapshotContents',
// t('public~VolumeGroupSnapshotContents')
labelPluralKey: 'public~VolumeGroupSnapshotContents',

Copy link
Contributor

Choose a reason for hiding this comment

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

this is needed for parser...

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
frontend/public/models/index.ts Outdated
Comment on lines 1254 to 1259
labelPlural: 'VolumeGroupSnapshotClasses',
labelPluralKey: 'public~VolumeGroupSnapshotClasses',
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
labelPlural: 'VolumeGroupSnapshotClasses',
labelPluralKey: 'public~VolumeGroupSnapshotClasses',
labelPlural: 'VolumeGroupSnapshotClasses',
// t('public~VolumeGroupSnapshotClasses')
labelPluralKey: 'public~VolumeGroupSnapshotClasses',

Copy link
Contributor

Choose a reason for hiding this comment

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

this is needed for parser...

SanjalKatiyar
SanjalKatiyar reviewed Mar 18, 2025
View reviewed changes
frontend/public/models/index.ts Outdated
Comment on lines 1239 to 1242
labelPlural: 'VolumeGroupSnapshots',
labelPluralKey: 'public~VolumeGroupSnapshots',
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
labelPlural: 'VolumeGroupSnapshots',
labelPluralKey: 'public~VolumeGroupSnapshots',
labelPlural: 'VolumeGroupSnapshots',
// t('public~VolumeGroupSnapshots')
labelPluralKey: 'public~VolumeGroupSnapshots',

Copy link
Contributor

Choose a reason for hiding this comment

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

this is needed for parser...

@vbnrh vbnrh force-pushed the volume-group-snapshot-creation-page branch 2 times, most recently from 51e070c to c267a9e Compare March 19, 2025 11:54
@openshift-ci openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Mar 19, 2025
@vbnrh vbnrh force-pushed the volume-group-snapshot-creation-page branch 2 times, most recently from 6e2d762 to b5f4c54 Compare March 24, 2025 09:24
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 24, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bipuladh, vbnrh
Once this PR has been reviewed and has the lgtm label, please assign cajieh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@vbnrh vbnrh force-pushed the volume-group-snapshot-creation-page branch from b5f4c54 to d59afe4 Compare March 27, 2025 13:34
@vbnrh
Copy link
Contributor Author

vbnrh commented Mar 27, 2025

Screenshot 2025-03-27 at 7 06 13 PM
Screenshot 2025-03-27 at 7 06 38 PM
Screenshot 2025-03-27 at 7 06 53 PM

@vbnrh
Copy link
Contributor Author

vbnrh commented Mar 27, 2025

Screenshot 2025-03-27 at 7 10 40 PM

@vbnrh
Copy link
Contributor Author

vbnrh commented Apr 7, 2025

PR depends upon kubernetes-csi/external-snapshotter#1286

@vbnrh
Copy link
Contributor Author

vbnrh commented Apr 7, 2025 •
edited
Loading

@spadgett Please review. Its pending since some time

@vbnrh vbnrh force-pushed the volume-group-snapshot-creation-page branch from d59afe4 to 528d3c8 Compare April 9, 2025 11:50
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 9, 2025

@vbnrh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-console 528d3c8 link true /test e2e-gcp-console
ci/prow/frontend 528d3c8 link true /test frontend
ci/prow/okd-scos-e2e-aws-ovn 528d3c8 link false /test okd-scos-e2e-aws-ovn
ci/prow/analyze 528d3c8 link true /test analyze

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanjalKatiyar SanjalKatiyar SanjalKatiyar left review comments

@bipuladh bipuladh bipuladh approved these changes

@Mylanos Mylanos Awaiting requested review from Mylanos

@spadgett spadgett Awaiting requested review from spadgett

Assignees
No one assigned
Labels
component/core Related to console core functionality component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vbnrh @SanjalKatiyar @bipuladh @openshift-merge-robot