server: fix reset sshkey is broken in master/4.16 by weizhouapache · Pull Request #5390 · apache/cloudstack

weizhouapache · Aug 30, 2021

Description

This PR fixes the issue that reset sshkey is broken in master/4.16 branch.
It is a regression issue of #4819

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

weizhouapache · Aug 30, 2021

@nvazquez @GutoVeronezi @rhtyd
this is a blocker issue in master branch.

weizhouapache · Aug 30, 2021

@blueorangutan package

blueorangutan · Aug 30, 2021

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

blueorangutan · Aug 30, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1063

nvazquez

Thanks @weizhouapache just a minor comment

nvazquez · Aug 31, 2021

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

            throw new CloudRuntimeException("Failed to reset SSH Key for the virtual machine ");
        }

+        _vmDao.loadDetails(userVm);


I would prefer moving this line into the removeEncryptedPasswordFromUserVmVoDetails method if the VM details are empty

@nvazquez done.
details are loaded but not updated, therefore it does not contain the new ssh key.

yadvr · Aug 31, 2021

cc @GutoVeronezi

This reverts commit db278cf.

weizhouapache · Aug 31, 2021

@blueorangutan package

blueorangutan · Aug 31, 2021

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

blueorangutan · Aug 31, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1072

yadvr · Aug 31, 2021

@blueorangutan test

blueorangutan · Aug 31, 2021

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

weizhouapache · Aug 31, 2021

server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java


        final boolean canHandle = canHandle(network.getTrafficType());

-        if (canHandle) {


since password is not reset when reset sshkey (in #4819 ), it is not needed to save password in vm details.

weizhouapache · Aug 31, 2021

@blueorangutan package

blueorangutan · Aug 31, 2021

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

blueorangutan · Aug 31, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1079

nvazquez · Aug 31, 2021

@blueorangutan test

blueorangutan · Aug 31, 2021

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

blueorangutan · Sep 1, 2021

Trillian test result (tid-1872)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 52611 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5390-t1872-kvm-centos7.zip
Smoke tests completed. 90 look OK, 3 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_deploy_vm_start_failure	`Error`	61.37	test_deploy_vm.py
test_deploy_vm_volume_creation_failure	`Error`	62.55	test_deploy_vm.py
test_vm_ha	`Error`	83.99	test_vm_ha.py
test_vm_sync	`Error`	134.20	test_vm_sync.py

blueorangutan · Sep 2, 2021

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

blueorangutan · Sep 2, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1108

weizhouapache · Sep 2, 2021

@blueorangutan test

blueorangutan · Sep 2, 2021

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

blueorangutan · Sep 2, 2021

Trillian Build Failed (tid-1938)

nvazquez · Sep 2, 2021

@blueorangutan test

blueorangutan · Sep 2, 2021

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

blueorangutan · Sep 2, 2021

Trillian test result (tid-1940)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36813 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5390-t1940-kvm-centos7.zip
Smoke tests completed. 88 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_list_snapshots_with_removed_data_store	`Error`	51.45	test_snapshots.py

blueorangutan · Sep 3, 2021

Trillian test result (tid-1942)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36167 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5390-t1942-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez

LGTM

blueorangutan · Sep 3, 2021

Trillian test result (tid-1939)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 69940 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5390-t1939-kvm-centos7.zip
Smoke tests completed. 74 look OK, 15 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestLoadBalance>:setup	`Error`	0.00	test_loadbalance.py
test_list_clusters_metrics	`Error`	1511.86	test_metrics_api.py
test_list_vms_metrics	`Error`	0.19	test_metrics_api.py
ContextSuite context=TestNetworkACL>:setup	`Error`	0.00	test_network_acl.py
test_delete_account	`Error`	1511.52	test_network.py
test_delete_network_while_vm_on_it	`Error`	1.13	test_network.py
test_deploy_vm_l2network	`Error`	1.12	test_network.py
test_l2network_restart	`Error`	2.18	test_network.py
ContextSuite context=TestPortForwarding>:setup	`Error`	3.33	test_network.py
ContextSuite context=TestPublicIP>:setup	`Error`	0.99	test_network.py
test_reboot_router	`Failure`	0.04	test_network.py
test_releaseIP	`Error`	0.48	test_network.py
ContextSuite context=TestRouterRules>:setup	`Error`	0.53	test_network.py
ContextSuite context=TestAdapterTypeForNic>:setup	`Error`	0.00	test_nic_adapter_type.py
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_02_deploy_and_upgrade_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_04_basic_lifecycle_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_05_delete_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_06_deploy_invalid_kubernetes_ha_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_01_add_delete_kubernetes_supported_version	`Error`	1802.29	test_kubernetes_supported_versions.py
ContextSuite context=TestListIdsParams>:setup	`Error`	0.00	test_list_ids_parameter.py
test_nic_secondaryip_add_remove	`Error`	1511.67	test_multipleips_per_nic.py
ContextSuite context=TestNestedVirtualization>:setup	`Error`	0.00	test_nested_virtualization.py
ContextSuite context=TestIsolatedNetworksPasswdServer>:setup	`Error`	0.00	test_password_server.py
ContextSuite context=TestPortForwardingRules>:setup	`Error`	0.00	test_portforwardingrules.py
ContextSuite context=TestPrivateGwACL>:setup	`Error`	0.00	test_privategw_acl.py
ContextSuite context=TestProjectSuspendActivate>:setup	`Error`	1518.22	test_projects.py
test_02_list_snapshots_with_removed_data_store	`Error`	53.61	test_snapshots.py

weizhouapache · Sep 3, 2021

@blueorangutan package

blueorangutan · Sep 3, 2021

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

blueorangutan · Sep 3, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1125

weizhouapache · Sep 3, 2021

@blueorangutan test

blueorangutan · Sep 3, 2021

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

blueorangutan · Sep 3, 2021

Trillian test result (tid-1960)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34860 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5390-t1960-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez · Sep 7, 2021

Ping @rhtyd @davidjumani @sureshanaparti @shwstppr can you please review?

shwstppr · Sep 7, 2021

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

    }

    protected void removeEncryptedPasswordFromUserVmVoDetails(UserVmVO userVmVo) {
+        _vmDao.loadDetails(userVmVo);


@weizhouapache I'm not sure but instead of loading all details, removing one detail and then storing all details, can we do:

userVmDetailsDao.removeDetail(userVmVo.getId(), VmDetailConstants.ENCRYPTED_PASSWORD); _vmDao.loadDetails(userVmVo);

Also in the caller of this method we load details (ln 859), not sure if that is needed

@shwstppr
user vm details is updated in line 883 (ssh key is added), but userVmVo does not contain it.
boolean result = resetVMSSHKeyInternal(vmId, sshPublicKey);
therefore we need to reload the details.
it is good idea to use userVmDetailsDao.removeDetail. I will change the code and test it. thanks.

@weizhouapache yes, VM details are updated by resetVMSSHKeyInternal and that is why I feel loadDetails at 859 can be removed as I don't see it being used anywhere.

@shwstppr removed line 859.

weizhouapache · Sep 7, 2021

@blueorangutan package

blueorangutan · Sep 7, 2021

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

blueorangutan · Sep 7, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1147

weizhouapache · Sep 7, 2021

@blueorangutan test

blueorangutan · Sep 7, 2021

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

blueorangutan · Sep 7, 2021

Trillian test result (tid-1979)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33340 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5390-t1979-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · Sep 7, 2021

Trillian test result (tid-1978)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34511 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5390-t1978-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

server: fix reset sshkey is broken in master/4.16

db278cf

weizhouapache requested review from GutoVeronezi, nvazquez and yadvr August 30, 2021 19:51

nvazquez added this to the 4.16.0.0 milestone Aug 31, 2021

nvazquez added the Severity:BLOCKER label Aug 31, 2021

nvazquez reviewed Aug 31, 2021

View reviewed changes

weizhouapache added 2 commits August 31, 2021 09:02

Revert "server: fix reset sshkey is broken in master/4.16"

24d91bb

This reverts commit db278cf.

update apache#5390

7069ddb

server: fix another regression of apache#4819 and apache#5274

542ec42

weizhouapache commented Aug 31, 2021

View reviewed changes

apache deleted a comment from weizhouapache Aug 31, 2021

apache deleted a comment from blueorangutan Aug 31, 2021

nvazquez approved these changes Sep 3, 2021

View reviewed changes

shwstppr reviewed Sep 7, 2021

View reviewed changes

update apache#5390

719acee

davidjumani approved these changes Sep 7, 2021

View reviewed changes

nvazquez merged commit b13930f into apache:main Sep 8, 2021

weizhouapache deleted the 4.16-fix-reset-sshkey branch December 9, 2022 08:46


		final boolean canHandle = canHandle(network.getTrafficType());

		if (canHandle) {

Search code, repositories, users, issues, pull requests...

Conversation

weizhouapache commented Aug 30, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

weizhouapache commented Aug 30, 2021

Uh oh!

weizhouapache commented Aug 30, 2021

Uh oh!

blueorangutan commented Aug 30, 2021

Uh oh!

blueorangutan commented Aug 30, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weizhouapache Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

yadvr commented Aug 31, 2021

Uh oh!

weizhouapache commented Aug 31, 2021

Uh oh!

blueorangutan commented Aug 31, 2021

Uh oh!

blueorangutan commented Aug 31, 2021

Uh oh!

yadvr commented Aug 31, 2021

Uh oh!

blueorangutan commented Aug 31, 2021

Uh oh!

weizhouapache Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 31, 2021

Uh oh!

blueorangutan commented Aug 31, 2021

Uh oh!

blueorangutan commented Aug 31, 2021

Uh oh!

nvazquez commented Aug 31, 2021

Uh oh!

blueorangutan commented Aug 31, 2021

Uh oh!

blueorangutan commented Sep 1, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

weizhouapache commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

nvazquez commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 3, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Sep 3, 2021

Uh oh!

weizhouapache commented Sep 3, 2021

nvazquez Aug 31, 2021 •

edited

Loading