[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user asfgit closed the pull request at: https://github.com/apache/cloudstack/pull/685 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user karuturi commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-138771684 :+1: --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-138591416 ok, LGTM at least we now have success and failure tests and all checks pass. We can add the more trivial tests for checkForKeyByPublicKey in a later PR --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-138492797 @DaanHoogland @kishankavala Have made some changes to test cases. Please review. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-137254065 so now we are back to, "I would like to see two unit tests for checkForKeyByPublicKey. The succes and the failure cases. I am not sure if this check shows a very good pattern but that is out of scope for this PR. Other then unit tests missing, LGTM." I won't stop stop this with a :-1: but I think it can be augmented with more tests. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-136934002 @DaanHoogland Removed the duplicate tests. Please have a look. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
Re: [GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
I agree On Mon, Aug 31, 2015 at 6:45 AM, kansal wrote: > Github user kansal commented on the pull request: > > https://github.com/apache/cloudstack/pull/685#issuecomment-136250704 > > @DaanHoogland Yes the first test in a way does the same thing only. > The thing is that the second test is more intuitive in a way that it firsts > registers a key-pair (the registration is successful) and then registers > another one with different name but same key (Registration fails here). So > the "Existing pair" check which the first one wants to test is > automatically tested. In my opinion the first test can be removed. Your > opinion? > > > --- > If your project is set up for it, you can reply to this email and have your > reply appear on GitHub as well. If your project does not have this feature > enabled and wishes so, or if the feature is enabled but not working, please > contact infrastructure at infrastruct...@apache.org or file a JIRA ticket > with INFRA. > --- > -- Daan
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-136250704 @DaanHoogland Yes the first test in a way does the same thing only. The thing is that the second test is more intuitive in a way that it firsts registers a key-pair (the registration is successful) and then registers another one with different name but same key (Registration fails here). So the "Existing pair" check which the first one wants to test is automatically tested. In my opinion the first test can be removed. Your opinion? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-135775262 @kansal it looks to me the new test tests exactly the same functionality as the previous one. Can you elaborate on the difference? And more specifically can we delete the old one? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r38192659 --- Diff: setup/db/db/schema-452to460.sql --- @@ -354,6 +354,10 @@ CREATE VIEW `cloud`.`user_vm_view` AS left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +---Additional checks to ensure duplicate keys are not registered and remove the previously stored duplicate keys. +DELETE `s1` FROM `ssh_keypairs` `s1`, `ssh_keypairs` `s2` WHERE `s1`.`id` > `s2`.`id` AND `s1`.`public_key` = `s2`.`public_key` AND `s1`.`account_id` = `s2`.`account_id`; +ALTER TABLE `ssh_keypairs` ADD UNIQUE `unique_index`(`fingerprint`,`account_id`); --- End diff -- May be name unique_index can be better renamed? By default UNIQUE constraint signifies that. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-135747132 @DaanHoogland @kishankavala Added test case for checking the duplicate key-pair registrations. Please review. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-134919051 Can you add some tests, we'll need to test it before merging. @DaanHoogland have you looked at it, since you had recently made some related changes or might have experience in the codebase around that? In general, LGTM. We definitely merge after you had added some tests. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-134088772 I would like to see two unit tests for checkForKeyByPublicKey. The succes and the failure cases. I am not sure if this check shows a very good pattern but that is out of scope for this PR. Other then unit tests missing, LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-134027948 @kishankavala @DaanHoogland Can you please review it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-132482850 @DaanHoogland @kishankavala Have made changes regarding allowing the duplicate keys for different accounts. DB checks are additional checks and API level checks are done separately. Please review!! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-131699503 @kishankavala @DaanHoogland @sedukull The API level check was already in place but was not working because the check was done on the public key passed from user before converting it to public_key form using the method getPublicKeyFromKeyKeyMaterial(). Have made a change regarding that. Also, I feel the additional DB checks would be good. Please review? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-130539282 @kishankavala The current implementation of the createSSHKeyPair checks for the keypair-name in the table ssh_keypairs and if that name exists it will return "A keypair with the exists." But at the time of inserting the keypairs it will not insert the same key once again because of the UNIQUE constraint that is added in the above commit. So In a way the duplication problem is automatically checked. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36940851 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; --- End diff -- @sedukull there are no constraints which refer to ssh_keypairs as the foreign key. Yes there are foreign key constraints on the columns of ssh_keypairs, but deleteing them won't create any problem as they are not referenced further. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kansal commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36858306 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; +ALTER TABLE ssh_keypairs ADD UNIQUE (fingerprint); --- End diff -- @sedukullI have deleted the lines with the same public_key because we want that previous different name-same key registrations are handled. (I mean the ones people will already have in their DB). For putting the unique constraint, I added it on the fingerprint because the public_key is a VARCHAR(5120) and our DB doesn't allow unique key on such large values. It won't be a problem because finger print is generated from the public_key only. Coming to deleting the rows, I am deleting the ones with newer(large) id's. If required, older ones can be deleted and newest can be kept.(no big deal). @DaanHoogland Being new to the community, I was not aware of the fact that we have take these to Mailing lists. If you want I can do that also. @sedukull as far as the api changes are concerned, I don't think that is necessary. Its not generating the duplicates in the main table. What happens is that in the logic, there is join between ssh_keypairs and user_vm_details to create user_vm_view on the basis of public_key. Since public_key is not a foreign key, such kind of issues are arising. For "cascading delete logic" I will get back to you in detail. But it worked fine for me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36857039 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; --- End diff -- It seems ssh_keypairs table, has foreign key constraints with cascading delete logic, as per table definition. Will these delete statements effect the rows in other tables say account and domain? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user DaanHoogland commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36856027 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; +ALTER TABLE ssh_keypairs ADD UNIQUE (fingerprint); --- End diff -- @sedukull It was said they should be announced on the ML and that they occur on .0 releases only unless it is a bugfix. please take it on list, though. It will never hurt. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36855563 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; +ALTER TABLE ssh_keypairs ADD UNIQUE (fingerprint); --- End diff -- As well, there was a discussion on the mailing list that all DB changes be discussed on the ML. Can we discuss this issue on ML first? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36855515 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; +ALTER TABLE ssh_keypairs ADD UNIQUE (fingerprint); --- End diff -- Can we indent these two lines better, currently it shows as part of view creation? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36855422 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; +ALTER TABLE ssh_keypairs ADD UNIQUE (fingerprint); --- End diff -- Here, we added unique constraint on fingerprint column, but in previous one you deleted the rows where public_key column values are same. The bug mentions that "previous" ones added with same names are deleted, here it seems "id" is an auto increment column and is deleting rows which are added "later", not the "previous" ones. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user sedukull commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/685#discussion_r36855234 --- Diff: setup/db/db/schema-452to460.sql --- @@ -353,6 +353,8 @@ CREATE VIEW `cloud`.`user_vm_view` AS `cloud`.`user_vm_details` `custom_speed` ON (((`custom_speed`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_speed`.`name` = 'CpuSpeed'))) left join `cloud`.`user_vm_details` `custom_ram_size` ON (((`custom_ram_size`.`vm_id` = `cloud`.`vm_instance`.`id`) and (`custom_ram_size`.`name` = 'memory'))); +delete s1 from ssh_keypairs s1, ssh_keypairs s2 where s1.id > s2.id and s1.public_key = s2.public_key; --- End diff -- I believe, its an api change required rather adding the constraint at the db level, otherwise it will mask the actual issue in the code where these duplicates are getting added. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
Github user kishankavala commented on the pull request: https://github.com/apache/cloudstack/pull/685#issuecomment-130282420 @kansal Can you add a similar check for duplicates keys in the createSSHKeyPair API validation? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8727: API call listVirtualMach...
GitHub user kansal opened a pull request: https://github.com/apache/cloudstack/pull/685 CLOUDSTACK-8727: API call listVirtualMachines returns same keypair Currently the user can register same key with different names. Upon listing the VM's the name which got registered first is being returned and not the actual one. Anyhow this behavior is rare and not good. I have added a UNIQUE constraint on the ssh_keypairs table and also made sure that the previous registered keys(with duplicates) get deleted. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kansal/cloudstack CLOUDSTACK-8727 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/685.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #685 commit ee45f299f4df9b51ba3c530a9770392780a33dd5 Author: Kshitij Kansal Date: 2015-08-12T12:07:48Z CLOUDSTACK-8727: API call listVirtualMachines returns same keypair --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
