You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by sureshanaparti <gi...@git.apache.org> on 2015/09/15 12:53:34 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8858: listVolumes API fails fo...
GitHub user sureshanaparti opened a pull request:
https://github.com/apache/cloudstack/pull/830
CLOUDSTACK-8858: listVolumes API fails for a particular domain with NPE.
CLOUDSTACK-8858: listVolumes API fails for a particular domain with NPE.
Summary: listVolumes API fails when volume associated vm instance has NULL or invalid state. Fix the code to guard this situation since this should not block volume listing.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sureshanaparti/cloudstack CLOUDSTACK-8858
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/830.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 #830
----
commit c3777632c5817cc1e635ee59eb18d6cb4f210e29
Author: Suresh Kumar Anaparti <su...@citrix.com>
Date: 2015-09-15T10:52:06Z
CLOUDSTACK-8858: listVolumes API fails for a particular domain with NPE.
Summary: listVolumes API fails when volume associated vm instance has NULL or invalid state. Fix the code to guard this situation since this should not block volume listing.
----
---
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-8858: listVolumes API fails fo...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162453199
Though I would like to see how this situation is reproduced, the code is fine and sensible.
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-8858: listVolumes API fails fo...
Posted by sureshanaparti <gi...@git.apache.org>.
Github user sureshanaparti commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162612642
@ustcweizhou , Yes. I agree. In case on instance id, vm_state is not 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-8858: listVolumes API fails fo...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162484552
@sureshanaparti How do we force such a situation to occur?
It seems to me that should be guarded on input and alerted on on output instead of silently filtered.
---
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-8858: listVolumes API fails fo...
Posted by sureshanaparti <gi...@git.apache.org>.
Github user sureshanaparti commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162494808
The volumes, in Ready state and are not attached to any VM have their vm_state has null and querying such volumes may result in NPE. These changes are only to safe guard.
---
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-8858: listVolumes API fails fo...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162486767
@sureshanaparti
mysql> SELECT id, name, volume_type, state, vm_state FROM cloud.volume_view where vm_state is null and vm_id is not null;
Empty set (0.00 sec)
even in vm_instance
mysql> select * from vm_instance where state is null;
Empty set (0.00 sec)
---
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-8858: listVolumes API fails fo...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-175667434
Merging as it has 2LGTMs
---
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-8858: listVolumes API fails fo...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162473175
I have the same concern with Daan. How to reproduce this issue ?
Check my env
mysql> select * from volumes where volume_type is null or provisioning_type is null or state is null;
Empty set (0.00 sec)
---
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-8858: listVolumes API fails fo...
Posted by sureshanaparti <gi...@git.apache.org>.
Github user sureshanaparti commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162483571
@DaanHoogland , @ustcweizhou
This issue is reproduced when the volume associated vm instance has null or invalid state (vm_state in volume_view). The code changes would guard this situation since it should not block volume listing.
mysql> SELECT id, name, volume_type, state, vm_state FROM cloud.volume_view where vm_state is null;
+----+----------------------+-------------+----------+----------+
| id | name | volume_type | state | vm_state |
+----+----------------------+-------------+----------+----------+
| 57 | ROOT-57 | ROOT | Expunged | NULL |
| 69 | mytestvolume | DATADISK | Ready | NULL |
| 71 | DATA-69 | DATADISK | Expunged | NULL |
| 73 | DATA-70 | DATADISK | Expunged | NULL |
| 78 | testvolcreated | DATADISK | Ready | NULL |
| 81 | test2volume | DATADISK | Destroy | NULL |
| 83 | testvol07 | DATADISK | Ready | NULL |
| 87 | testattachvolcreated | DATADISK | Ready | NULL |
+----+----------------------+-------------+----------+----------+
8 rows in set (0.00 sec)
---
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-8858: listVolumes API fails fo...
Posted by sureshanaparti <gi...@git.apache.org>.
Github user sureshanaparti commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162493826
@ustcweizhou , Same with my result set as well.
mysql> select * from vm_instance where state is null;
Empty set (0.00 sec)
mysql> SELECT id, name, volume_type, state, vm_state FROM cloud.volume_view where vm_state is null and vm_id is not null;
Empty set (0.00 sec)
---
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-8858: listVolumes API fails fo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/830
---
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-8858: listVolumes API fails fo...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162345934
PIng @DaanHoogland can you review this please? I'll run 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8858: listVolumes API fails fo...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-162520618
@sureshanaparti
I do not get NPE when listVolumes.
please notice the code:
long instanceId = volume.getVmId();
if (instanceId > 0 && volume.getState() != Volume.State.Destroy) {
...
if the volume is not attached, this judgment returns false.
---
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-8858: listVolumes API fails fo...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/830#issuecomment-174510528
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.
---