server,cks: check if vm is cks node during vm destroy #9057

shwstppr · 2024-05-08T09:14:45Z

Description

Fixes #8902

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Fixes apache#8902 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

codecov-commenter · 2024-05-08T09:24:09Z

Codecov Report

Attention: Patch coverage is 29.78723% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 14.96%. Comparing base (87e7c57) to head (11b2156).
Report is 5 commits behind head on 4.19.

Files	Patch %	Lines
...va/com/cloud/utils/component/ComponentContext.java	0.00%	14 Missing ⚠️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	0.00%	10 Missing ⚠️
...tes/cluster/dao/KubernetesClusterVmMapDaoImpl.java	0.00%	8 Missing ⚠️
...ubernetes/cluster/KubernetesClusterHelperImpl.java	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #9057      +/-   ##
============================================
- Coverage     14.96%   14.96%   -0.01%     
+ Complexity    10995    10988       -7     
============================================
  Files          5373     5373              
  Lines        469005   469071      +66     
  Branches      58953    61186    +2233     
============================================
- Hits          70198    70184      -14     
- Misses       391036   391120      +84     
+ Partials       7771     7767       -4

Flag	Coverage Δ
uitests	`4.31% <ø> (ø)`
unittests	`15.66% <29.78%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

weizhouapache · 2024-05-08T10:52:06Z

@shwstppr
I am not sure if we should disallow the operation.

would it be a valid use case for users to

drain the node in CKS
remove the node vm from cloudstack
scale the cks cluster to get a new node

for your information, in Autoscale VM group, users can destroy a specific VM when the vm group is disabled.
the corresponding reference/map in database and load balancer rules are also removed.
of course, cks cluster is totally different from AS vm group.

sureshanaparti

clgtm

sureshanaparti · 2024-05-08T10:56:05Z

...bernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterHelperImpl.java

+            return;
+        }
+        logger.error(String.format("VM ID: %s is a part of Kubernetes cluster ID: %d", userVm.getId(), vmMapVO.getClusterId()));
+        throw new CloudRuntimeException("Instance is a part of a Kubernetes cluster");


specify the Kubernetes cluster id/name in the msg passed to the response?

sureshanaparti · 2024-05-08T10:57:03Z

...bernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterHelperImpl.java


    @Override
    public ControlledEntity findByUuid(String uuid) {
        return kubernetesClusterDao.findByUuid(uuid);
    }

+    @Override
+    public void checkVmCanBeDestroyed(UserVm userVm) {


can destroy allowed with force option?

do you mean adding a new API param as there is no force param in destroyVirtualMachine?

yes, with force param in destroy call.

@sureshanaparti @vishesh92 I'm not completely in favour of adding the parameter. Deleting specific node of a k8s cluster is already covered by scaleKubernetesCluster API
see #9057 (comment)

Do we have any other specific case for which we should allow the operation?

shwstppr · 2024-05-08T11:06:45Z

@shwstppr I am not sure if we should disallow the operation.

would it be a valid use case for users to
* drain the node in CKS

* remove the node vm from cloudstack

* scale the cks cluster to get a new node
for your information, in Autoscale VM group, users can destroy a specific VM when the vm group is disabled. the corresponding reference/map in database and load balancer rules are also removed. of course, cks cluster is totally different from AS vm group.

@weizhouapache understood. Though destroying a valid cks node is also allowed right now.
@Pearl1594 @nvazquez do we plan to add an API to remove a VM/node from CKS cluster?
If there is, then the case mentioned by Wei should get handled? User can:

drain node
remove it from k8s cluster
destroy the vm

weizhouapache · 2024-05-08T11:29:44Z

@shwstppr I am not sure if we should disallow the operation.
would it be a valid use case for users to
* drain the node in CKS

* remove the node vm from cloudstack

* scale the cks cluster to get a new node
for your information, in Autoscale VM group, users can destroy a specific VM when the vm group is disabled. the corresponding reference/map in database and load balancer rules are also removed. of course, cks cluster is totally different from AS vm group.
@weizhouapache understood. Though destroying a valid cks node is also allowed right now. @Pearl1594 @nvazquez do we plan to add an API to remove a VM/node from CKS cluster? If there is, then the case mentioned by Wei should get handled? User can:

drain node

remove it from k8s cluster

destroy the vm

thanks @shwstppr
looks like a good idea to add a new API .

shwstppr · 2024-05-09T11:49:23Z

@weizhouapache on the special case discussed earlier, @Pearl1594 made me aware that we already have an option with scaleKubernetesCluster API to delete a specific node using nodeids param. We also show a delete action in the UI so the use-case that you mentioned should get covered by that,

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

weizhouapache · 2024-05-09T15:43:23Z

@weizhouapache on the special case discussed earlier, @Pearl1594 made me aware that we already have an option with scaleKubernetesCluster API to delete a specific node using nodeids param. We also show a delete action in the UI so the use-case that you mentioned should get covered by that,

That is great, thanks @shwstppr

Overall this pr looks good to me.

Can we add the information above in the error message, if user tries to delete a cks node?

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-05-22T06:06:44Z

@blueorangutan package

blueorangutan · 2024-05-22T06:08:06Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-05-22T07:18:47Z

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 9656

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-05-22T08:50:32Z

@blueorangutan package

blueorangutan · 2024-05-22T08:52:04Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-05-22T10:39:45Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9665

shwstppr · 2024-05-24T11:40:20Z

@blueorangutan package

blueorangutan · 2024-05-24T11:42:03Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-05-24T13:20:01Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9695

server,cks: check if vm is cks node during vm destroy

a8e2c13

Fixes apache#8902 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborg bot added component:api component:kubernetes labels May 8, 2024

fix

fbf310e

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

sureshanaparti approved these changes May 8, 2024

View reviewed changes

sureshanaparti reviewed May 8, 2024

View reviewed changes

sureshanaparti added this to the 4.19.1.0 milestone May 8, 2024

rohityadavcloud approved these changes May 8, 2024

View reviewed changes

shwstppr marked this pull request as draft May 9, 2024 10:58

find Kubernetes helper

687a95c

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborg bot added the component:build label May 9, 2024

add cluster name in error

7ef716b

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

additional message

00cc2cb

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

fix

11b2156

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server,cks: check if vm is cks node during vm destroy #9057

server,cks: check if vm is cks node during vm destroy #9057

shwstppr commented May 8, 2024

codecov-commenter commented May 8, 2024 •

edited by codecov bot

weizhouapache commented May 8, 2024

sureshanaparti left a comment

sureshanaparti May 8, 2024

shwstppr May 8, 2024

sureshanaparti May 8, 2024

shwstppr May 8, 2024

sureshanaparti May 8, 2024

vishesh92 May 9, 2024

shwstppr May 9, 2024

shwstppr commented May 8, 2024

weizhouapache commented May 8, 2024

shwstppr commented May 9, 2024

weizhouapache commented May 9, 2024

shwstppr commented May 22, 2024

blueorangutan commented May 22, 2024

blueorangutan commented May 22, 2024

shwstppr commented May 22, 2024

blueorangutan commented May 22, 2024

blueorangutan commented May 22, 2024

shwstppr commented May 24, 2024

blueorangutan commented May 24, 2024

blueorangutan commented May 24, 2024

server,cks: check if vm is cks node during vm destroy #9057

Are you sure you want to change the base?

server,cks: check if vm is cks node during vm destroy #9057

Conversation

shwstppr commented May 8, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov-commenter commented May 8, 2024 • edited by codecov bot

Codecov Report

weizhouapache commented May 8, 2024

sureshanaparti left a comment

Choose a reason for hiding this comment

sureshanaparti May 8, 2024

Choose a reason for hiding this comment

shwstppr May 8, 2024

Choose a reason for hiding this comment

sureshanaparti May 8, 2024

Choose a reason for hiding this comment

shwstppr May 8, 2024

Choose a reason for hiding this comment

sureshanaparti May 8, 2024

Choose a reason for hiding this comment

vishesh92 May 9, 2024

Choose a reason for hiding this comment

shwstppr May 9, 2024

Choose a reason for hiding this comment

shwstppr commented May 8, 2024

weizhouapache commented May 8, 2024

shwstppr commented May 9, 2024

weizhouapache commented May 9, 2024

shwstppr commented May 22, 2024

blueorangutan commented May 22, 2024

blueorangutan commented May 22, 2024

shwstppr commented May 22, 2024

blueorangutan commented May 22, 2024

blueorangutan commented May 22, 2024

shwstppr commented May 24, 2024

blueorangutan commented May 24, 2024

blueorangutan commented May 24, 2024

codecov-commenter commented May 8, 2024 •

edited by codecov bot