server,cks: check if vm is cks node during vm destroy by shwstppr · Pull Request #9057 · apache/cloudstack

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?

(local) 🐱 > list kubernetesclusters filter=id,virtualmachines
{
  "count": 1,
  "kubernetescluster": [
    {
      "id": "a2cd4198-55be-43da-9a3a-1a05ebb9745d",
      "virtualmachines": [
        {
          "account": "ACSUser",
          "affinitygroup": [],
          "created": "2024-06-05T07:51:22+0000",
          "details": {},
          "displayname": "test1-control-18fe761907b",
          "domain": "ROOT",
          "domainid": "8da34656-08a3-11ef-9779-1e00a7000c5f",
          "guestosid": "8da8cd0e-08a3-11ef-9779-1e00a7000c5f",
          "haenable": false,
          "hasannotations": false,
          "hostcontrolstate": "Enabled",
          "hypervisor": "KVM",
          "id": "77919cbe-e0de-48c9-aa97-fe789f7ccbab",
          "isdynamicallyscalable": false,
          "lastupdated": "2024-06-05T07:51:34+0000",
          "name": "test1-control-18fe761907b",
          "nic": [
            {
              "broadcasturi": "vlan://2007",
              "deviceid": "0",
              "extradhcpoption": [],
              "gateway": "10.1.1.1",
              "id": "0d04047c-7888-4c8f-8a85-da2d1d0c7aac",
              "ipaddress": "10.1.1.130",
              "isdefault": true,
              "isolationuri": "vlan://2007",
              "macaddress": "02:01:00:d7:00:02",
              "netmask": "255.255.255.0",
              "networkid": "e8e2095e-6575-4639-8dbd-411e14b6753f",
              "networkname": "test1-network",
              "secondaryip": [],
              "traffictype": "Guest",
              "type": "Isolated"
            }
          ],
          "osdisplayname": "Debian GNU/Linux 5.0 (64-bit)",
          "ostypeid": "8da8cd0e-08a3-11ef-9779-1e00a7000c5f",
          "pooltype": "NetworkFilesystem",
          "readonlydetails": "dataDiskController, rootDiskController",
          "receivedbytes": 0,
          "securitygroup": [],
          "sentbytes": 0,
          "state": "Running",
          "tags": [],
          "userid": "a709d3e3-3254-40b2-bdc7-79c269e84ac7",
          "username": "user",
          "zoneid": "31c7d02e-7734-4bef-ab3c-e48a244025ce",
          "zonename": "pr8601-t6927-kvm-centos7"
        },
        {
          "account": "ACSUser",
          "affinitygroup": [],
          "created": "2024-06-05T07:51:34+0000",
          "details": {
            "deployvm": "true"
          },
          "displayname": "test1-node-18fe761c02e",
          "domain": "ROOT",
          "domainid": "8da34656-08a3-11ef-9779-1e00a7000c5f",
          "guestosid": "8da8cd0e-08a3-11ef-9779-1e00a7000c5f",
          "haenable": false,
          "hasannotations": false,
          "hypervisor": "KVM",
          "id": "8fa9e120-cc14-4973-95f9-bd68776aae16",
          "isdynamicallyscalable": false,
          "lastupdated": "2024-06-05T07:51:37+0000",
          "name": "test1-node-18fe761c02e",
          "nic": [
            {
              "deviceid": "0",
              "extradhcpoption": [],
              "gateway": "10.1.1.1",
              "id": "9e98a986-6d77-4083-aadb-8a447635d1bd",
              "ipaddress": "10.1.1.112",
              "isdefault": true,
              "macaddress": "02:01:00:d7:00:03",
              "netmask": "255.255.255.0",
              "networkid": "e8e2095e-6575-4639-8dbd-411e14b6753f",
              "networkname": "test1-network",
              "secondaryip": [],
              "traffictype": "Guest",
              "type": "Isolated"
            }
          ],
          "osdisplayname": "Debian GNU/Linux 5.0 (64-bit)",
          "ostypeid": "8da8cd0e-08a3-11ef-9779-1e00a7000c5f",
          "pooltype": "NetworkFilesystem",
          "readonlydetails": "dataDiskController, rootDiskController",
          "receivedbytes": 0,
          "securitygroup": [],
          "sentbytes": 0,
          "state": "Stopped",
          "tags": [],
          "userid": "a709d3e3-3254-40b2-bdc7-79c269e84ac7",
          "username": "user",
          "zoneid": "31c7d02e-7734-4bef-ab3c-e48a244025ce",
          "zonename": "pr8601-t6927-kvm-centos7"
        }
      ]
    }
  ]
}
(local) 🐱 > destroy virtualmachine id=77919cbe-e0de-48c9-aa97-fe789f7ccbab expunge=true
{
  "account": "admin",
  "accountid": "aaa96ed7-08a3-11ef-9779-1e00a7000c5f",
  "cmd": "org.apache.cloudstack.api.command.admin.vm.DestroyVMCmdByAdmin",
  "completed": "2024-06-05T08:38:15+0000",
  "created": "2024-06-05T08:38:15+0000",
  "domainid": "8da34656-08a3-11ef-9779-1e00a7000c5f",
  "domainpath": "ROOT",
  "jobid": "5370d57d-0709-4463-b5cc-21a8fa34f3a0",
  "jobinstanceid": "77919cbe-e0de-48c9-aa97-fe789f7ccbab",
  "jobinstancetype": "VirtualMachine",
  "jobprocstatus": 0,
  "jobresult": {
    "errorcode": 530,
    "errortext": "Instance is a part of a Kubernetes cluster: test1. Use Instance delete option from Kubernetes cluster details or scale API for Kubernetes clusters with 'nodeids' to destroy the instance."
  },
  "jobresultcode": 530,
  "jobresulttype": "object",
  "jobstatus": 2,
  "userid": "aaaa743f-08a3-11ef-9779-1e00a7000c5f"
}
🙈 Error: async API failed for job 5370d57d-0709-4463-b5cc-21a8fa34f3a0

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 missing coverage. Please review.

Project coverage is 14.96%. Comparing base (87e7c57) to head (11b2156).
Report is 21 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 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?

No other specific use case from my side.

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

shwstppr · 2024-05-27T08:21:08Z

@blueorangutan test

blueorangutan · 2024-05-27T08:22:06Z

@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-05-27T20:30:41Z

[SF] Trillian test result (tid-10283)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42160 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9057-t10283-kvm-centos7.zip
Smoke tests completed. 131 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

shwstppr · 2024-06-05T08:40:36Z

Added test API calls in the PR description

weizhouapache

code lgtm

DaanHoogland

clgtm and

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

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

yadvr 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>

DaanHoogland linked an issue Jun 5, 2024 that may be closed by this pull request

CKS Cluster goes into alert state when a node is expunged #8902

Closed

shwstppr marked this pull request as ready for review June 5, 2024 08:40

weizhouapache approved these changes Jun 5, 2024

View reviewed changes

DaanHoogland self-assigned this Jun 6, 2024

DaanHoogland added the status:needs-testing label Jun 6, 2024

DaanHoogland approved these changes Jun 6, 2024

View reviewed changes

DaanHoogland removed the status:needs-testing label Jun 6, 2024

DaanHoogland removed their assignment Jun 6, 2024

DaanHoogland merged commit 91c7bc7 into apache:4.19 Jun 6, 2024

vishesh92 deleted the fix-vmdelete-cksnode branch June 6, 2024 08:54

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 17, 2024

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

54d552c

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

kiranchavala mentioned this pull request Jun 24, 2024

UI: Remove the delete action item for cks nodes #9293

Closed

Conversation

shwstppr commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

codecov-commenter commented May 8, 2024 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

weizhouapache commented May 8, 2024

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shwstppr commented May 8, 2024

Uh oh!

weizhouapache commented May 8, 2024

Uh oh!

shwstppr commented May 9, 2024

Uh oh!

weizhouapache commented May 9, 2024

Uh oh!

shwstppr commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

shwstppr commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

shwstppr commented May 24, 2024

Uh oh!

blueorangutan commented May 24, 2024

Uh oh!

blueorangutan commented May 24, 2024

Uh oh!

shwstppr commented May 27, 2024

Uh oh!

blueorangutan commented May 27, 2024

Uh oh!

blueorangutan commented May 27, 2024

Uh oh!

shwstppr commented Jun 5, 2024

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

shwstppr commented May 8, 2024 •

edited

Loading

codecov-commenter commented May 8, 2024 •

edited by codecov bot

Loading