Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virt.pool_delete fast parameter is inversed and useless #54474

Closed
cbosdo opened this issue Sep 13, 2019 · 2 comments
Closed

virt.pool_delete fast parameter is inversed and useless #54474

cbosdo opened this issue Sep 13, 2019 · 2 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P4 Priority 4 stale
Milestone

Comments

@cbosdo
Copy link
Contributor

cbosdo commented Sep 13, 2019

Description of Issue

Introduced in 2019.2.0, the virt.pool_delete() function has a fast parameter. There are two issues with this parameter:

  • it is inversed with its meaning. fast=True actually adds the libvirt.VIR_STORAGE_POOL_DELETE_ZEROED flag, that should zero the pool and make the delete operation much slower.

  • After investigations, none of the libvirt storage backends actually implements this flag, which means that in all cases the default virt.pool_delete() call will raise a libvirt exception like the following one:

Failed deleting pool: unsupported flags (0x1) in function virStorageBackendDeleteLocal

The proposed solution would be to completely remove the fast parameter. In all cases changing this part of the API shouldn't hurt, since defaults don't work at all.

@xeacott xeacott added fixed-pls-verify fix is linked, bug author to confirm fix Bug broken, incorrect, or confusing behavior P4 Priority 4 and removed needs-triage labels Oct 14, 2019
@xeacott xeacott added this to the Approved milestone Oct 14, 2019
cbosdo added a commit to cbosdo/salt that referenced this issue Nov 26, 2019
There are two reasons to remove this parameter without deprecating it:
 * the meaning has been mistakenly inversed
 * fast=True will raise an exception for every libvirt storage backend
since that flag has never been implemented in any of those.

Fixes issue saltstack#54474
cbosdo added a commit to cbosdo/salt that referenced this issue Dec 6, 2019
There are two reasons to remove this parameter without deprecating it:
 * the meaning has been mistakenly inversed
 * fast=True will raise an exception for every libvirt storage backend
since that flag has never been implemented in any of those.

Fixes issue saltstack#54474
@stale
Copy link

stale bot commented Jan 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot added the stale label Jan 7, 2020
@cbosdo
Copy link
Contributor Author

cbosdo commented Jan 8, 2020

Fixed now

@cbosdo cbosdo closed this as completed Jan 8, 2020
admd pushed a commit to admd/salt-1 that referenced this issue Jan 27, 2020
There are two reasons to remove this parameter without deprecating it:
 * the meaning has been mistakenly inversed
 * fast=True will raise an exception for every libvirt storage backend
since that flag has never been implemented in any of those.

Fixes issue saltstack#54474
agraul pushed a commit to agraul/salt that referenced this issue Apr 22, 2020
There are two reasons to remove this parameter without deprecating it:
 * the meaning has been mistakenly inversed
 * fast=True will raise an exception for every libvirt storage backend
since that flag has never been implemented in any of those.

Fixes issue saltstack#54474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior fixed-pls-verify fix is linked, bug author to confirm fix P4 Priority 4 stale
Projects
None yet
Development

No branches or pull requests

3 participants