Skip to content

feat(images): showing why cannot delete #859

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 5 commits into
base: main
Choose a base branch
from
Open

feat(images): showing why cannot delete #859

wants to merge 5 commits into from

Conversation

eofff
Copy link
Contributor

@eofff eofff commented Mar 17, 2025 •
edited
Loading

Description

Resources preventing the deletion of VirtualImage and ClusterVirtualImage are being displayed.

Why do we need it, and what problem does it solve?

What is the expected result?

Checklist

  • The code is covered by unit tests.
  • e2e tests passed.
  • Documentation updated according to the changes.
  • Changes were tested in the Kubernetes cluster manually.

Changelog entries

section: images
type: feature
summary: "Resources preventing the deletion of VirtualImage and ClusterVirtualImage are being displayed."

@eofff eofff force-pushed the feat/images/show-usages-while-terminating branch 25 times, most recently from 39993d5 to b98aa71 Compare March 24, 2025 08:06
@eofff eofff added this to the v0.18.0 milestone Mar 24, 2025
@eofff eofff force-pushed the feat/images/show-usages-while-terminating branch from b98aa71 to 592b0dc Compare March 24, 2025 13:40
@eofff eofff force-pushed the feat/images/show-usages-while-terminating branch from 592b0dc to b62ece7 Compare April 14, 2025 08:34
@eofff eofff added the e2e/run label Apr 14, 2025
@eofff eofff added the e2e/run label Apr 14, 2025
@eofff eofff added the e2e/run label Apr 14, 2025
@eofff eofff marked this pull request as ready for review April 15, 2025 07:28
@eofff eofff force-pushed the feat/images/show-usages-while-terminating branch from b25943b to 501885a Compare April 15, 2025 09:29
@eofff eofff added the e2e/run label Apr 15, 2025
Valeriy Khorunzhin added 4 commits April 15, 2025 12:30
feat
f28c02e 
Signed-off-by: Valeriy Khorunzhin <[email protected]>
fix merge conflicts
3c498f6 
Signed-off-by: Valeriy Khorunzhin <[email protected]>
fix
0964d5d 
Signed-off-by: Valeriy Khorunzhin <[email protected]>
refactoring
71ca641 
Signed-off-by: Valeriy Khorunzhin <[email protected]>
@eofff eofff force-pushed the feat/images/show-usages-while-terminating branch from 501885a to 71ca641 Compare April 15, 2025 09:30
@eofff eofff added e2e/run and removed e2e/run labels Apr 15, 2025
@z9r5 z9r5 requested a review from Lada7878 April 16, 2025 07:42
@Lada7878 Lada7878 requested review from mortelumina98 and removed request for Lada7878 April 16, 2025 09:07
@mortelumina98
crds review
d4b249d 
Signed-off-by: mortelumina98 <[email protected]>
yaroslavborbat
yaroslavborbat requested changes Apr 18, 2025
View reviewed changes
images/virtualization-artifact/pkg/controller/cvi/internal/deletion.go
Comment on lines +50 to +54
inUseCondition, _ := conditions.GetCondition(cvicondition.InUseType, cvi.Status.Conditions)
if inUseCondition.Status == metav1.ConditionTrue && conditions.IsLastUpdated(inUseCondition, cvi) {
controllerutil.AddFinalizer(cvi, virtv2.FinalizerCVICleanup)
return reconcile.Result{}, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Shall do cleanup and drop finalaizer if resource termigating

images/virtualization-artifact/pkg/controller/cvi/internal/handler_test.go
limitations under the License.
*/

package internal
Copy link
Member

Choose a reason for hiding this comment

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

rename file to suite_test.go

images/virtualization-artifact/pkg/controller/cvi/internal/inuse_test.go
"github.com/deckhouse/virtualization/api/core/v1alpha2/cvicondition"
)

var _ = DescribeTable("InUseHandler Handle", func(args inUseHandlerTestArgs) {
Copy link
Member

Choose a reason for hiding this comment

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

Check unit tests in vm handlers. We have some helprers for register specs

images/virtualization-artifact/pkg/controller/cvi/internal/inuse_test.go
objects = append(objects, &cvi)
}

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).WithIndex(
Copy link
Member

Choose a reason for hiding this comment

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

see testutil package

images/virtualization-artifact/pkg/controller/cvi/internal/life_cycle.go
@@ -55,6 +55,14 @@ func (h LifeCycleHandler) Handle(ctx context.Context, cvi *virtv2.ClusterVirtual
}

if cvi.DeletionTimestamp != nil {
if readyCondition.Status == metav1.ConditionTrue {
Copy link
Member

Choose a reason for hiding this comment

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

why it is needed?

images/virtualization-artifact/pkg/controller/cvi/internal/inuse.go
Comment on lines +139 to +149
switch len(vds.Items) {
case 1:
messageBuilder.WriteString(fmt.Sprintf("ClusterVirtualImage is currently being used to create the VirtualDisk %s/%s", vds.Items[0].Namespace, vds.Items[0].Name))
case 2, 3:
var vdNamespacedNames []string
for _, vd := range vds.Items {
vdNamespacedNames = append(vdNamespacedNames, fmt.Sprintf("%s/%s", vd.Namespace, vd.Name))
}
messageBuilder.WriteString(fmt.Sprintf("ClusterVirtualImage is currently being used to create the VirtualDisks: %s", strings.Join(vdNamespacedNames, ", ")))
default:
messageBuilder.WriteString(fmt.Sprintf("ClusterVirtualImage is used to create %d VirtualDisks", len(vds.Items)))
Copy link
Member

Choose a reason for hiding this comment

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

This switch statement appears to be overly complex. I notice that you are writing separate messages for one item, two or three items, and a larger number of items. It seems this logic could be simplified into a single if len(...) <= 3 {} else {} structure.

images/virtualization-artifact/pkg/controller/cvi/internal/inuse.go
Comment on lines +119 to +129
switch len(vmUsedImage) {
case 1:
messageBuilder.WriteString(fmt.Sprintf("The ClusterVirtualImage is currently attached to the VirtualMachine %s/%s", vmUsedImage[0].Namespace, vmUsedImage[0].Name))
case 2, 3:
var vmNamespacedNames []string
for _, vm := range vmUsedImage {
vmNamespacedNames = append(vmNamespacedNames, fmt.Sprintf("%s/%s", vm.Namespace, vm.Name))
}
messageBuilder.WriteString(fmt.Sprintf("The ClusterVirtualImage is currently attached to the VirtualMachines: %s", strings.Join(vmNamespacedNames, ", ")))
default:
messageBuilder.WriteString(fmt.Sprintf("%d VirtualMachines are using the ClusterVirtualImage", len(vmUsedImage)))
Copy link
Member

Choose a reason for hiding this comment

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

This switch statement appears to be overly complex. I notice that you are writing separate messages for one item, two or three items, and a larger number of items. It seems this logic could be simplified into a single if len(...) <= 3 {} else {} structure.

images/virtualization-artifact/pkg/controller/cvi/internal/inuse.go

var vis virtv2.VirtualImageList
err = h.client.List(ctx, &vis, client.MatchingFields{
indexer.IndexFieldVIByCVIDataSourceNotReady: cvi.GetName(),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should filter ready sources here instead. This would eliminate the need for an additional indexer.

images/virtualization-artifact/pkg/controller/cvi/internal/inuse.go

var cvis virtv2.ClusterVirtualImageList
err = h.client.List(ctx, &cvis, client.MatchingFields{
indexer.IndexFieldCVIByCVIDataSourceNotReady: cvi.GetName(),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should filter ready sources here instead. This would eliminate the need for an additional indexer.

images/virtualization-artifact/pkg/controller/cvi/internal/inuse.go

var vds virtv2.VirtualDiskList
err = h.client.List(ctx, &vds, client.MatchingFields{
indexer.IndexFieldVDByCVIDataSourceNotReady: cvi.GetName(),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should filter ready sources here instead. This would eliminate the need for an additional indexer.

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

@yaroslavborbat yaroslavborbat yaroslavborbat requested changes

@z9r5 z9r5 z9r5 approved these changes

@mortelumina98 mortelumina98 mortelumina98 approved these changes

@Isteb4k Isteb4k Awaiting requested review from Isteb4k Isteb4k is a code owner

@fl64 fl64 Awaiting requested review from fl64 fl64 is a code owner

@goganat goganat Awaiting requested review from goganat goganat is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@eofff eofff

Labels
None yet
Projects
None yet
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.
5 participants
@eofff @z9r5 @yaroslavborbat @mortelumina98 @deckhouse-BOaTswain