You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by sanju1010 <gi...@git.apache.org> on 2015/09/22 11:21:28 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
GitHub user sanju1010 opened a pull request:
https://github.com/apache/cloudstack/pull/871
CLOUDSTACK-8893: Fixing script as per the latest functionality
Please check https://issues.apache.org/jira/browse/CLOUDSTACK-8893 for more details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sanju1010/cloudstack vmsnap
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/871.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 #871
----
commit 85fad5ae76c97ca17d258245007906a96aa04c21
Author: sanjeev <sa...@apache.org>
Date: 2015-09-22T09:17:37Z
CLOUDSTACK-8893: Fixing script as per the latest functionality
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by sanju1010 <gi...@git.apache.org>.
Github user sanju1010 commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-142271544
@Desc: Test that Volume snapshot for root volume not allowed ... === TestName: test_01_test_vm_volume_snapshot | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 1 test in 165.417s
OK
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #871: CLOUDSTACK-8893: Fixing script as per the latest func...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the issue:
https://github.com/apache/cloudstack/pull/871
Hi @sanju1010,
@serg38 and I are working on this PR #1677 which solves vm snapshot creation issue. After running `test_vm_snapshots.py` we're getting exception on `test_01_test_vm_volume_snapshot` as it was expected before your changes in this PR.
We're getting: `'CloudstackAPIException: Execute cmd: createsnapshot failed, due to: errorCode: 431, errorText:Volume snapshot is not allowed, please detach it from VM with VM Snapshots\n']`
This is consistent with `VolumeApiServiceImpl.allocSnapshot` method, in which it is:
```
if (volume.getInstanceId() != null) {
// Check that Vm to which this volume is attached does not have VM Snapshots
if (_vmSnapshotDao.findByVm(volume.getInstanceId()).size() > 0) {
throw new InvalidParameterValueException("Volume snapshot is not allowed, please detach it from VM with VM Snapshots");
}
}
```
Do you want us to adapt `test_vm_snapshots.py` in mentioned PR to support this or would you like to address it yourself?
Thanks
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script
as per the...
Posted by Sanjeev N <sa...@apache.org>.
@pavanb018 good catch !! It is not part of the code but will change.
On Wed, Sep 23, 2015 at 10:52 AM, pavanb018 <gi...@git.apache.org> wrote:
> Github user pavanb018 commented on the pull request:
>
> https://github.com/apache/cloudstack/pull/871#issuecomment-142497091
>
> @Desc: Test that Volume snapshot for root volume not allowed ... ===
> TestName: test_01_test_vm_volume_snapshot | Status : SUCCESS ===
> ok
>
> Sanju1010 , is the above description part of script ? The description
> looks misleading with the current functionality. If it is not part of the
> code then LGTM !! , else please modify the description so that it wont be
> confusing.
>
>
> ---
> 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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by pavanb018 <gi...@git.apache.org>.
Github user pavanb018 commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-142497091
@Desc: Test that Volume snapshot for root volume not allowed ... === TestName: test_01_test_vm_volume_snapshot | Status : SUCCESS ===
ok
Sanju1010 , is the above description part of script ? The description looks misleading with the current functionality. If it is not part of the code then LGTM !! , else please modify the description so that it wont be confusing.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by pavanb018 <gi...@git.apache.org>.
Github user pavanb018 commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-142819336
An LGTM is given after going through the code and understanding what the code is accomplishing. The PR already has the test results and if i am not wrong an LGTM is for reviewing the code to make sure the functionality is working as expected ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by runseb <gi...@git.apache.org>.
Github user runseb commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-144322464
@pavanb018 it always helps other folks reviewing to know what you did. For example.
Did you just check the Travis green light ? Did you just check the code through github ? Did you pull the patch, apply and compile ? Did you do the build then run the tests. It may sound obvious to you what you did by giving the copy paste form a test, but writing a small friendly sentence for the next reviewer will go a long way to help us merge those fixes as a group.
thanks
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by pavanb018 <gi...@git.apache.org>.
Github user pavanb018 commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-142532171
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by nitt10prashant <gi...@git.apache.org>.
Github user nitt10prashant commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-142493294
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #871: CLOUDSTACK-8893: Fixing script as per the latest func...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the issue:
https://github.com/apache/cloudstack/pull/871
@jburwell @rhtyd @karuturi @nvazquez I assume the current code doesn\u2019t allow volume snapshots on the top of vmsnapshots on purpose. In that case is it best to revert PR871 or create a new PR to address the Marvin script?
From: Nicolas Vazquez <no...@github.com>
Reply-To: apache/cloudstack <re...@reply.github.com>
Date: Wednesday, September 21, 2016 at 11:15 AM
To: apache/cloudstack <cl...@noreply.github.com>
Cc: Sergey Levitskiy <Se...@autodesk.com>, Mention <me...@noreply.github.com>
Subject: Re: [apache/cloudstack] CLOUDSTACK-8893: Fixing script as per the latest functionality (#871)
Hi @sanju1010<https://github.com/sanju1010>,
@serg38<https://github.com/serg38> and I are working on this PR #1677<https://github.com/apache/cloudstack/pull/1677> which solves vm snapshot creation issue. After running test_vm_snapshots.py we're getting exception on test_01_test_vm_volume_snapshot as it was expected before your changes in this PR.
We're getting: 'CloudstackAPIException: Execute cmd: createsnapshot failed, due to: errorCode: 431, errorText:Volume snapshot is not allowed, please detach it from VM with VM Snapshots\n']
This is consistent with VolumeApiServiceImpl.allocSnapshot method, in which it is:
if (volume.getInstanceId() != null) {
// Check that Vm to which this volume is attached does not have VM Snapshots
if (_vmSnapshotDao.findByVm(volume.getInstanceId()).size() > 0) {
throw new InvalidParameterValueException("Volume snapshot is not allowed, please detach it from VM with VM Snapshots");
}
}
Do you want us to adapt test_vm_snapshots.py in mentioned PR to support this or would you like to address it yourself?
Thanks
\u2014
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/apache/cloudstack/pull/871#issuecomment-248696674>, or mute the thread<https://github.com/notifications/unsubscribe-auth/APbWPq2NlxyRQADHpD6ucL5jzTkZ_Zy1ks5qsXQpgaJpZM4GBeLo>.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/871
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by nitt10prashant <gi...@git.apache.org>.
Github user nitt10prashant commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-142820449
@remibergsma Next time sure I will add some details .for this PR i have updated my comments .
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8893: Fixing script as per the...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/871#issuecomment-142696052
@pavanb018 @nitt10prashant Could you please add what you tested next time? A LGTM without any explanation doesn't really tell me anything to be honest.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---