Skip to content

Allow renaming cluster, host, and storage#4165

Merged
yadvr merged 5 commits intoapache:masterfrom
CLDIN:rename-cluster-n-host
Aug 5, 2020
Merged

Allow renaming cluster, host, and storage#4165
yadvr merged 5 commits intoapache:masterfrom
CLDIN:rename-cluster-n-host

Conversation

@GabrielBrascher
Copy link
Member

@GabrielBrascher GabrielBrascher commented Jun 22, 2020

Description

This PR adds implementation for changing host and storage name, additionally, it fixes a Bug on cluster updateCluster API command. This PRs also enhances the UI by allowing editing field name on Host and Storage pool. Due to the fact that there is no support to editing cluster via UI, it was not edited.

TODO: I will address Host, Cluster, and Storage Pool name edition on CloudStack Primate once the API implementation gets merged.

Details:
Prior to this PR the following API commands did not offer support for updating name:

Additionally, updateCluster claims to support changing a cluster name (via clustername parameter); however, such operation did not work. (bug)

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)

How Has This Been Tested?

  • Via API, change host, cluster, and storage pool names.
  • Via UI, change host and storage pool names.

}

private void updateCluster(long clusterId, String state) {
private void updateCluster(long clusterId, String allocationState) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@nvazquez can you please take a look at it?

I changed API parameter handling from separate parameters on the method call to an object of UpdateClusterCmd class. Therefore, I needed to change this tiny piece of RollingMaintenanceManagerImpl.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @GabrielBrascher, looks good to me. The change needed in RollingMaintenanceManagerImpl will be just the same as you've done below

@GabrielBrascher GabrielBrascher requested a review from nvazquez June 22, 2020 20:05
Update Host name handling to update instead of persist

Allow update
 Storage name
@GabrielBrascher GabrielBrascher force-pushed the rename-cluster-n-host branch from 38e69e5 to bef567f Compare June 22, 2020 20:07
@wido
Copy link
Contributor

wido commented Jun 23, 2020

Looks good to me based on the code

@yadvr
Copy link
Member

yadvr commented Jun 24, 2020

Renaming of cluster on VMware could cause a problem if the cluster on VCenter is not mapped; on XenServer I'm not sure if there'll be any side-effects. On KVM LGTM

Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

Code looking good, @GabrielBrascher can you please add the small refactor on RollingMaintenanceManagerImpl as well?

@GabrielBrascher
Copy link
Member Author

@nvazquez thanks for the review!

I think that I already did the necessary changes in the method updateCluster at RollingMaintenanceManagerImpl. Please let me know if I am missing something.

@GabrielBrascher
Copy link
Member Author

@rhtyd thanks for the feedback, I will prevent cluster renaming on VMware then (maybe on Xenserver as well).

@GabrielBrascher GabrielBrascher force-pushed the rename-cluster-n-host branch from cea53fb to 74e9900 Compare June 24, 2020 20:53
@GabrielBrascher GabrielBrascher force-pushed the rename-cluster-n-host branch from 74e9900 to 9176e29 Compare June 24, 2020 20:54
@yadvr
Copy link
Member

yadvr commented Jul 4, 2020

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1528

Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

LGTM based on code review, haven't tested it

boolean doUpdate = false;

if (org.apache.commons.lang.StringUtils.isNotBlank(name)) {
if(cluster.getHypervisorType() == HypervisorType.VMware) {
Copy link
Member Author

@GabrielBrascher GabrielBrascher Jul 16, 2020

Choose a reason for hiding this comment

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

@rhtyd considering your concerns with issues caused by renaming VMware clusters I added this line.

@yadvr
Copy link
Member

yadvr commented Jul 27, 2020

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✖centos7 ✔debian. JID-1605

@yadvr
Copy link
Member

yadvr commented Jul 27, 2020

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1607

@yadvr
Copy link
Member

yadvr commented Jul 27, 2020

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

Trillian test result (tid-2205)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 66215 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4165-t2205-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 81 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_05_deploy_and_upgrade_kubernetes_ha_cluster Failure 2121.32 test_kubernetes_clusters.py
ContextSuite context=Test01DeployVM>:setup Error 0.00 test_vm_life_cycle.py
ContextSuite context=Test02VMLifeCycle>:setup Error 0.00 test_vm_life_cycle.py
ContextSuite context=Test03SecuredVmMigration>:setup Error 0.00 test_vm_life_cycle.py

@yadvr yadvr merged commit 2237486 into apache:master Aug 5, 2020
GabrielBrascher added a commit to PCextreme/cloudstack-primate that referenced this pull request Aug 24, 2020
GabrielBrascher added a commit to PCextreme/cloudstack-primate that referenced this pull request Aug 24, 2020
yadvr pushed a commit to apache/cloudstack-primate that referenced this pull request Aug 26, 2020
* Allow renaming cluster, host, and storage

CloudStack PR: apache/cloudstack#4165

* change clustername.label to Cluster Name
weizhouapache pushed a commit to apache/cloudstack-primate that referenced this pull request Jan 19, 2021
* Allow renaming cluster, host, and storage

CloudStack PR: apache/cloudstack#4165

* change clustername.label to Cluster Name
yadvr pushed a commit that referenced this pull request Jan 20, 2021
* Allow renaming cluster, host, and storage

CloudStack PR: #4165

* change clustername.label to Cluster Name

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants