You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by koushik-das <gi...@git.apache.org> on 2015/09/09 09:39:09 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8826: XenServer - Use device i...
GitHub user koushik-das opened a pull request:
https://github.com/apache/cloudstack/pull/792
CLOUDSTACK-8826: XenServer - Use device id passed as part of attach v…
…olume API properly
If device id passed as part of API and available then use it otherwise fallback on XS to automatically assign one.
For ISO device id used is 3 and it is processed before any other entry to avoid conflict.
This is only specific to XS, ran the following tests:
BVT: test/integration/smoke/test_volumes.py
Test Volume creation for all Disk Offerings (incl. custom) ... === TestName: test_01_create_volume | Status : SUCCESS ===
ok
Attach a created Volume to a Running VM ... === TestName: test_02_attach_volume | Status : SUCCESS ===
ok
Download a Volume attached to a VM ... === TestName: test_03_download_attached_volume | Status : SUCCESS ===
ok
Delete a Volume attached to a VM ... === TestName: test_04_delete_attached_volume | Status : SUCCESS ===
ok
Detach a Volume attached to a VM ... === TestName: test_05_detach_volume | Status : SUCCESS ===
ok
Download a Volume unattached to an VM ... === TestName: test_06_download_detached_volume | Status : SUCCESS ===
ok
Test resize (negative) non-existent volume ... === TestName: test_07_resize_fail | Status : SUCCESS ===
ok
Test resize a volume ... === TestName: test_08_resize_volume | Status : SUCCESS ===
ok
Delete a Volume unattached to an VM ... === TestName: test_09_delete_detached_volume | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 9 tests in 1116.972s
OK
Regression: test/integration/component/test_volumes.py
Test Volume attach/detach to VM (5 data volumes) ... === TestName: test_01_volume_attach_detach | Status : SUCCESS ===
ok
Test Attach volumes (max capacity) ... === TestName: test_01_volume_attach | Status : SUCCESS ===
ok
Test attach volumes (more than max) to an instance ... === TestName: test_02_volume_attach_max | Status : SUCCESS ===
ok
Test Volumes and ISO attach ... === TestName: test_01_volume_iso_attach | Status : SUCCESS ===
ok
Test custom disk sizes beyond range ... === TestName: test_deployVmWithCustomDisk | Status : SUCCESS ===
ok
@Desc:Volume is not retaining same uuid when migrating from one ... SKIP: No suitable storage pools found for volume migration. Skipping
Attach a created Volume to a Running VM ... === TestName: test_01_attach_volume | Status : SUCCESS ===
ok
Detach a Volume attached to a VM ... === TestName: test_02_detach_volume | Status : SUCCESS ===
ok
Delete a Volume unattached to an VM ... === TestName: test_03_delete_detached_volume | Status : SUCCESS ===
ok
Create a volume under a non-root domain as non-root-domain user ... === TestName: test_create_volume_under_domain | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 10 tests in 1750.686s
OK (SKIP=1)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/koushik-das/cloudstack CLOUDSTACK-8826
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/792.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 #792
----
commit 1dd27bd449a8e3d70b011e66af431e866c7627c2
Author: Koushik Das <ko...@apache.org>
Date: 2015-09-09T07:30:17Z
CLOUDSTACK-8826: XenServer - Use device id passed as part of attach volume API properly
If device id passed as part of API and available then use it otherwise fallback on XS to automatically assign one.
For ISO device id used is 3 and it is processed before any other entry to avoid conflict.
----
---
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-8826: XenServer - Use device i...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-139524545
code 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-8826: XenServer - Use device i...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-142236155
Merging as 2 LGTMs.
---
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-8826: XenServer - Use device i...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-140359521
one remark, 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-8826: XenServer - Use device i...
Posted by pdion891 <gi...@git.apache.org>.
Github user pdion891 commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-212423092
@koushik-das does this fix got tested with HVM vm having more than 4 VDI? because we are experiencing issue where an HVM vm on XenServer 6.5 having 4 vdi (1 root + 3 datadisk) fail to start after a shutdown and it seams to be related to this part of code. This is currently working on 4.4.x version of ACS.
---
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-8826: XenServer - Use device i...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/792#discussion_r39827592
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -3407,6 +3390,18 @@ public void handleSrAndVdiDetach(final String iqn, final Connection conn) throws
removeSR(conn, sr);
}
+ protected void destroyUnattachedVBD(Connection conn, VM vm) {
+ try {
+ for (VBD vbd : vm.getVBDs(conn)) {
+ if (Types.VbdType.DISK.equals(vbd.getType(conn)) && !vbd.getCurrentlyAttached(conn)) {
+ vbd.destroy(conn);
+ }
+ }
+ } catch (final Exception e) {
+ s_logger.debug("destroyUnattachedVBD failed due to " + e.toString());
--- End diff --
@DaanHoogland Are you suggesting to log the full exception? Currently only the message is getting logged. The destroyUnattachedVBD is a best-effort call to cleanup VBDs.
---
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-8826: XenServer - Use device i...
Posted by simongodard <gi...@git.apache.org>.
Github user simongodard commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-212573040
I confirm what @pdion891 described in the previous comment.
http://markmail.org/thread/4nmyra6aofxtu3o2
---
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-8826: XenServer - Use device i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/792
---
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-8826: XenServer - Use device i...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/792#discussion_r39839158
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -3407,6 +3390,18 @@ public void handleSrAndVdiDetach(final String iqn, final Connection conn) throws
removeSR(conn, sr);
}
+ protected void destroyUnattachedVBD(Connection conn, VM vm) {
+ try {
+ for (VBD vbd : vm.getVBDs(conn)) {
+ if (Types.VbdType.DISK.equals(vbd.getType(conn)) && !vbd.getCurrentlyAttached(conn)) {
+ vbd.destroy(conn);
+ }
+ }
+ } catch (final Exception e) {
+ s_logger.debug("destroyUnattachedVBD failed due to " + e.toString());
--- End diff --
@koushik-das I would catch only specific exceptions and give them specific messages and report on results in a finally block. the present state is that any numberformat, memory, stackoverflow or devidebyzero will be caught here, where it is not relevant. My initial remark was to make sure we know what kind of exception is caught at least. This is not always clear from the message which may be null.
---
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-8826: XenServer - Use device i...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/792#discussion_r39962919
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -3407,6 +3390,18 @@ public void handleSrAndVdiDetach(final String iqn, final Connection conn) throws
removeSR(conn, sr);
}
+ protected void destroyUnattachedVBD(Connection conn, VM vm) {
+ try {
+ for (VBD vbd : vm.getVBDs(conn)) {
+ if (Types.VbdType.DISK.equals(vbd.getType(conn)) && !vbd.getCurrentlyAttached(conn)) {
+ vbd.destroy(conn);
+ }
+ }
+ } catch (final Exception e) {
+ s_logger.debug("destroyUnattachedVBD failed due to " + e.toString());
--- End diff --
@DaanHoogland Now logging the full exception
---
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-8826: XenServer - Use device i...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-212753295
@pdion891 This wasn't tested with HVM VMs.
---
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-8826: XenServer - Use device i...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/792#discussion_r39499607
--- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java ---
@@ -3407,6 +3390,18 @@ public void handleSrAndVdiDetach(final String iqn, final Connection conn) throws
removeSR(conn, sr);
}
+ protected void destroyUnattachedVBD(Connection conn, VM vm) {
+ try {
+ for (VBD vbd : vm.getVBDs(conn)) {
+ if (Types.VbdType.DISK.equals(vbd.getType(conn)) && !vbd.getCurrentlyAttached(conn)) {
+ vbd.destroy(conn);
+ }
+ }
+ } catch (final Exception e) {
+ s_logger.debug("destroyUnattachedVBD failed due to " + e.toString());
--- End diff --
maybe add the exception or/and catch more specific exceptions
---
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-8826: XenServer - Use device i...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-138819369
I had to undo changes from PR https://github.com/apache/cloudstack/pull/773 to run the 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8826: XenServer - Use device i...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/792#issuecomment-140317652
Can anyone review this?
---
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.
---