CLOUDSTACK-9727 Password reset discrepancy in RVR when one of the Rou… by bvbharatk · Pull Request #1965 · apache/cloudstack

bvbharatk · 2017-02-23T12:50:30Z

…ter is not in Running state.

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)

ustcweizhou · 2017-02-23T13:54:37Z

@bvbharatk can you format the code ?

bvbharatk · 2017-02-24T06:39:47Z

@ustcweizhou
Reformatted the code.
thanks.

jayapalu · 2017-03-07T06:06:15Z

Code changes LGTM

…ter is not in Running state.

ustcweizhou · 2017-03-07T07:24:50Z

@bvbharatk I think the new password should be saved to one of the running routers, not all running routers.
I disagree with change line 797.

bvbharatk · 2017-03-13T05:09:47Z

@ustcweizhou
Hi we are not saving the password to the router. we are saving the password in the VM details. When the VM starts we send the password to the router. Password reset cannot be called when the VM is running.
We save the password in the VM details if one of the routers is not running in case of rvr network. Even if the master went down and Backup came up at the time of userVM start, we will serve the correct password as we are saving it.

ustcweizhou · 2017-03-13T07:22:44Z

@bvbharatk Yes, you might know only the vm has sshkey attached will have the password in vm details. If the vm does not have ssh keypair, then the vm password will not saved into user_vm_details.

Actually the vm password should be synced between master and backup. Saving to only one of them or saving to both of them are not working fine.
for example, if we save password in master, but not save it in backup. Once the master is down, then vm cannot get password from backup vr.
another example is, if we save password on both of master and backup, if vm get the password from master and reset it, once the master is down (or master->backup switch) and we reboot vm later, the vm will get the old password from backup again.

bvbharatk · 2017-03-14T08:42:20Z

@ustcweizhou
We are saving the password in the user_vm_details explicitly. We are not checking if ssh key pair is set for this vm or not.

I agree that ideally we should sync the password between the master and backup, For any kind of sync to work we need to know if the password was read from one of the VRs and In cases when one of the Vr is Stopped we will have to clear the password from db when it is read from the other one. These type of changes add complexity to the simple task of setting a password. The next best thing is to make sure we save the same password in both the routers. This will fix will at least solve the problem of sending the correct password even if the master and backup change state before the VM starts.

Yes like you pointed out this will lead to the problem that the user might receive the old password when he stop starts the VM, In this case the user will get a notification in the UI that his password will be changed. So he at least knows what the password is and so he can log into the VM.

bvbharatk · 2017-05-10T04:33:14Z

Hi,
Testing this PR end to end may not be possible now as there is some issue with the password server in case of RVRs.(CLOUDSTACK-9385). I have also encounter a similar problem when testing this manually, I saw that the password serve r is running on a different ip than expected and so the password script in the User VM failed to retrieve the password from the password server(CLOUDSTACK-9360).

However I was able to verify the intended behavior because of this PR, i.e. saving the password to the RVRs was successful.

ustcweizhou · 2017-05-14T05:14:49Z

@bvbharatk
for this issue, I have some ideas.
(1) If nothing changes, the password will be applied to the first router. If the BACKUP VR is the first, then vm will not get new password.
(2) If this PR is merged, then password will be applied to both router. In case of master<-> backup switch, the password will be stored to MASTER VR (=old BACKUP), then vm password will be reset after reboot/restart.
(3) If password is only be applied to MASTER router, then there will be no password stored on BACKUP router. If vm password is reset (and stored to MASTER router), but vm is started after a MASTER<->BACKUP switch, then vm will not get new password.

Considering these three options, I think option 3 is best. What do you think ?

bvbharatk · 2017-05-17T05:58:57Z

@ustcweizhou
Even if we store the password only in master option 2 can happen. Suppose the routers change the state again before the user starts the VM, the current master will become backup and will have the password stored on it. At some point of time when the state change happens and the VM restarts the password on the user VM will be reset again, without the knowledge of the user. This happens because we store the password indefinitely until the user reads this password. Once the user reads the password it should not be there on both the VRs.

I think there is a design problem here and addressing this will need some fundamental changes, so rather than trying to force fit it, i think it is better if we leave it at this state and then document this behavior. The workaround for now would be to remove the password script from he userVM once he successfully logs in.

Thanks,
Bharat.

ustcweizhou · 2017-05-17T06:56:22Z

@bvbharatk to be honest, we used option 2 on our production before and received some complain all vms' password are reset after reboot.

Then we used option 3. if customer cannot get new password, they can stop vm and reset vm password again (this happens not often, because normally customer start vm sooner after vm password reset so there is no master<->backup switch in this period).

Comparing these above 3 options, I do think option 3 has less impact and is more wise. VMpassword should be stored to only one place (yes I mean MASTER vr) if there is no sync.

We are using password sync between master and backup in our fork now. However, I have no time to create a PR.

bvbharatk · 2017-05-19T06:28:57Z

@ustcweizhou
Made changes as per your suggestion, Will send the password only to master router now. However we may still fail if router changes state while the password reset operation is in progress.

ustcweizhou · 2017-05-19T10:00:49Z

        final UserVmVO userVM = userdata.getUserVM();

-        _commandSetupHelper.createPasswordCommand(router, profile, nicVo, commands);
+        if (router.getRedundantState() == VirtualRouter.RedundantState.MASTER) {


line 69 should be changed to

if (!router.getIsRedundantRouter() || router.getRedundantState() == RedundantState.MASTER) {

2, this works in a new isolated network ( with RVR), but not working with a new VPC (with RVR). It might because there is always a guest network in new isolated network so VRs are MASTER/BACKUP, but for VPC they are BACKUP/BACKUP. I have no idea how to fix it, maybe we do not change this part and let password stored in both......actually only the first vm in a vpc.

@ustcweizhou
I am also not sure about the VPC behavior, so for now i will not alter the behavior for vpc networks. I have added the changes to ignore non redundant vrs and vpc vrs.

ustcweizhou · 2017-05-19T10:02:44Z

+                    }
+                    return true;
+                }
        }


line 781 to line 794 can be more simple like

for (final VirtualRouter router : routers) { if (router.getState() == State.Running && (!router.getIsRedundantRouter() || router.getRedundantState() == RedundantState.MASTER)) { return networkTopology.savePasswordToRouter(network, nic, uservm, router); } }

@ustcweizhou
modified this code.

ustcweizhou · 2017-05-19T10:04:04Z

+                    return true;
+                }
        }
+        //save password


save password should be moved to a seperate method, as password should be saved to user_vm_details table if it is not applied to VR, at around line 773.

@ustcweizhou
moved this code to a method.

cloudmonger · 2017-05-22T14:37:50Z

ACS CI BVT Run

Sumarry:
Build Number 736
Hypervisor xenserver
NetworkType Advanced
Passed=110
Failed=2
Skipped=12

Link to logs Folder (search by build_no): https://www.dropbox.com/sh/r2si930m8xxzavs/AAAzNrnoF1fC3auFrvsKo_8-a?dl=0

Failed tests:

test_volumes.py
test_06_download_detached_volume Failed
test_routers_network_ops.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true Failing since 2 runs

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_01_verify_libvirt
test_02_verify_libvirt_after_restart
test_03_verify_libvirt_attach_disk
test_04_verify_guest_lspci
test_05_change_vm_ostype_restart
test_06_verify_guest_lspci_again
test_static_role_account_acls
test_11_ss_nfs_version_on_ssvm
test_nested_virtualization_vmware
test_3d_gpu_support
test_deploy_vgpu_enabled_vm

Passed test suits:
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vm_snapshots.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_loadbalance.py
test_routers.py
test_reset_vm_on_reboot.py
test_deploy_vms_with_varied_deploymentplanners.py
test_network.py
test_router_dns.py
test_non_contigiousvlan.py
test_login.py
test_deploy_vm_iso.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_metrics_api.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py

bvbharatk · 2017-05-25T05:46:15Z

@ustcweizhou
Hi,
I have made the suggested changes, Can you please review?.

bvbharatk · 2017-06-13T05:14:46Z

@ustcweizhou
Can you please review this, I have made the suggested changes.

yadvr · 2017-11-23T21:13:11Z

@blueorangutan package

blueorangutan · 2017-11-23T21:13:19Z

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

blueorangutan · 2017-11-23T22:21:15Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1295

yadvr · 2018-01-02T12:52:01Z

Ping, any updates? I'll remove this from 4.11 milestone due to lack of engagement and discussions.

DaanHoogland · 2020-03-09T08:52:11Z

@weizhouapache is this still needed in your opinion?

yadvr · 2021-06-17T07:10:22Z

@weizhouapache is this fixed in recent version or is this still an issue?

weizhouapache · 2021-06-17T08:17:36Z

@rhtyd
I think it has been fixed by #3903

yadvr · 2021-06-17T08:27:02Z

Thanks @weizhouapache closing on your remark
@bvbharatk pl raise a new PR if you still find the issue

bvbharatk force-pushed the CLOUDSTACK-9727 branch 2 times, most recently from d953816 to 1a0e116 Compare February 24, 2017 06:33

bvbharatk force-pushed the CLOUDSTACK-9727 branch from 1a0e116 to 682d7bd Compare February 27, 2017 02:02

bvbharatk force-pushed the CLOUDSTACK-9727 branch 3 times, most recently from 0566c76 to 98bfb94 Compare March 7, 2017 06:05

CLOUDSTACK-9727 Password reset discrepancy in RVR when one of the Rou…

98bfb94

…ter is not in Running state.

kiwiflyer added the type:bug label May 11, 2017

ustcweizhou reviewed May 19, 2017

View reviewed changes

bvbharatk force-pushed the CLOUDSTACK-9727 branch 3 times, most recently from 81273dd to 4788ad6 Compare May 22, 2017 05:43

send password only to master router

4788ad6

yadvr added this to the 4.11 milestone Dec 10, 2017

yadvr removed this from the 4.11 milestone Jan 2, 2018

yadvr added this to the 4.11.1.0 milestone May 1, 2018

yadvr self-assigned this May 9, 2018

yadvr removed this from the 4.11.1.0 milestone May 10, 2018

yadvr modified the milestone: 4.13.0.0 Jun 26, 2019

yadvr mentioned this pull request Jul 3, 2019

Password Reset ignores redundant VR status when saving new password #3144

Closed

DaanHoogland marked this pull request as draft May 7, 2020 09:40

ACSGitBot added status:wip age:2years_plus labels Nov 3, 2020

yadvr closed this Jun 17, 2021

Conversation

bvbharatk commented Feb 23, 2017 • edited by PaulAngus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

Uh oh!

ustcweizhou commented Feb 23, 2017

Uh oh!

bvbharatk commented Feb 24, 2017

Uh oh!

jayapalu commented Mar 7, 2017

Uh oh!

ustcweizhou commented Mar 7, 2017

Uh oh!

bvbharatk commented Mar 13, 2017

Uh oh!

ustcweizhou commented Mar 13, 2017

Uh oh!

bvbharatk commented Mar 14, 2017

Uh oh!

bvbharatk commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ustcweizhou commented May 14, 2017

Uh oh!

bvbharatk commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ustcweizhou commented May 17, 2017

Uh oh!

bvbharatk commented May 19, 2017

Uh oh!

ustcweizhou May 19, 2017

Choose a reason for hiding this comment

Uh oh!

bvbharatk May 22, 2017

Choose a reason for hiding this comment

Uh oh!

ustcweizhou May 19, 2017 • 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!

bvbharatk May 22, 2017

Choose a reason for hiding this comment

Uh oh!

ustcweizhou May 19, 2017

Choose a reason for hiding this comment

Uh oh!

bvbharatk May 22, 2017

Choose a reason for hiding this comment

Uh oh!

cloudmonger commented May 22, 2017

ACS CI BVT Run

Uh oh!

bvbharatk commented May 25, 2017

Uh oh!

bvbharatk commented Jun 13, 2017

Uh oh!

yadvr commented Nov 23, 2017

Uh oh!

blueorangutan commented Nov 23, 2017

Uh oh!

blueorangutan commented Nov 23, 2017

Uh oh!

yadvr commented Jan 2, 2018

Uh oh!

DaanHoogland commented Mar 9, 2020

Uh oh!

yadvr commented Jun 17, 2021

Uh oh!

weizhouapache commented Jun 17, 2021

Uh oh!

yadvr commented Jun 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

bvbharatk commented Feb 23, 2017 •

edited by PaulAngus

Loading

bvbharatk commented May 10, 2017 •

edited

Loading

bvbharatk commented May 17, 2017 •

edited

Loading

ustcweizhou May 19, 2017 •

edited

Loading