Skip to content

fix OpenStack_2_NodeDriver.detach_volume#1267

Merged
asfgit merged 1 commit intoapache:trunkfrom
vdloo:fix-detach-volume-OpenStack_2_NodeDriver
May 26, 2019
Merged

fix OpenStack_2_NodeDriver.detach_volume#1267
asfgit merged 1 commit intoapache:trunkfrom
vdloo:fix-detach-volume-OpenStack_2_NodeDriver

Conversation

@vdloo
Copy link
Copy Markdown
Member

@vdloo vdloo commented Dec 10, 2018

Since #1242 the OpenStack_2_NodeDriver uses a volume object returned via volumev2_connection. This object will not contain serverId but server_id. This PR makes the detach_volume method compatible with v2 volumes.

Fixes:

return connection.detach_volume(volume)
File "/usr/local/venv/project/src/apache-libcloud/libcloud/compute/drivers/openstack.py", line 328, in detach_volume
(attachment['serverId'], attachment['id']),
KeyError: 'serverId'

@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 10, 2018

Codecov Report

Merging #1267 into trunk will decrease coverage by <.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##            trunk   #1267      +/-   ##
=========================================
- Coverage   85.91%   85.9%   -0.01%     
=========================================
  Files         356     356              
  Lines       73444   73456      +12     
  Branches     6653    6655       +2     
=========================================
+ Hits        63099   63105       +6     
- Misses       7689    7694       +5     
- Partials     2656    2657       +1
Impacted Files Coverage Δ
libcloud/compute/drivers/openstack.py 85.24% <0%> (ø) ⬆️
libcloud/test/compute/test_openstack.py 94.89% <66.66%> (-0.21%) ⬇️
libcloud/test/compute/test_ec2.py 97.74% <0%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c96216...7b3db0e. Read the comment docs.

@vdloo
Copy link
Copy Markdown
Member Author

vdloo commented Dec 18, 2018

@micafer could you take a look at this one?

if not ex_node or ex_node.id == attachment['serverId']:
if not ex_node or ex_node.id == attachment.get(
'serverId'
) or attachment['server_id']:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison is not correct.
In case of passing an ex_node, if attachment['server_id'] is not None, the attachment is deleted using any node.
It should be something like:

if not ex_node or ex_node.id in [attachment.get('serverId'), attachment['server_id']]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, good catch! I'll fix that

@vdloo vdloo force-pushed the fix-detach-volume-OpenStack_2_NodeDriver branch 3 times, most recently from a825d14 to 1b5742c Compare December 19, 2018 10:05
@vdloo
Copy link
Copy Markdown
Member Author

vdloo commented Dec 20, 2018

failing tests because of #1271

edit: fixed in #1271

@vdloo vdloo force-pushed the fix-detach-volume-OpenStack_2_NodeDriver branch from 1b5742c to 5c75064 Compare December 20, 2018 15:45
@vdloo vdloo mentioned this pull request Dec 20, 2018
Since apache#1242 the
OpenStack_2_NodeDriver uses a volume object returned via
volumev2_connection. This object will not contain serverId
but server_id. This PR makes the detach_volume method
compatible with v2 volumes.

Fixes:
```
return connection.detach_volume(volume)
File "/usr/local/venv/project/src/apache-libcloud/libcloud/compute/drivers/openstack.py", line 328, in detach_volume
(attachment['serverId'], attachment['id']),
KeyError: 'serverId'
```
@vdloo vdloo force-pushed the fix-detach-volume-OpenStack_2_NodeDriver branch from 5c75064 to 7b3db0e Compare December 21, 2018 15:49
@tonybaloney
Copy link
Copy Markdown
Contributor

@micafer please could you review the changes again

Copy link
Copy Markdown
Contributor

@micafer micafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@asfgit asfgit merged commit 7b3db0e into apache:trunk May 26, 2019
asfgit pushed a commit that referenced this pull request May 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants