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

Add timeout for "qemu-img convert" #3521

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

pevogam
Copy link
Contributor

@pevogam pevogam commented Sep 9, 2022

Signed-off-by: Plamen Dimitrov plamen.dimitrov@intra2net.com

Copy link
Contributor

@chunfuwen chunfuwen left a comment

Choose a reason for hiding this comment

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

LGTM

virttest/qemu_storage.py Outdated Show resolved Hide resolved
LOG.info("Convert image %s from %s to %s", self.image_filename,
self.image_format, convert_image.image_format)
process.run(convert_cmd)
timeout = params.get_numeric("conversion_timeout", 12000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use convert_params instead of params to get a fine-grained manipulation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC my probelm with convert_params was that it wasn't exactly filtered version of params but a smaller set of parameters introduced locally. But even then what you say would make sense so I will check in more detail.

virttest/qemu_storage.py Outdated Show resolved Hide resolved
virttest/qemu_storage.py Outdated Show resolved Hide resolved
virttest/qemu_storage.py Outdated Show resolved Hide resolved
Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
@pevogam pevogam changed the title Add timeout and allow space containing paths for "qemu-img convert" Add timeout for "qemu-img convert" Oct 21, 2022
Comment on lines +989 to +990
timeout = convert_params.get_numeric("image_conversion_timeout", -1)
timeout = None if timeout == -1 else timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe it seems to be the time to introduce a dedicated method to Param for getting timeouts, how does that sound? And later we may consider to add more features to support:

  • setting timeouts in different units of time (e.g. 5m, 1h)
  • basic calculations (with constants, e.g. TEST_TIMEOUT * 0.8)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I am not sure how Params object enhancements relates to the current pull request which aims specifically at qemu-img but bringing myself to talk about this new topic I would still say I am not sure whether introducing such methods in Params is not too data specific. The current type-handling methods are general enough and a small easy to maintain selection. Returning -1 is a very easy way to handle not just timeouts but various numeric parameters that might end up including infinity and the above is a simple solution that only uses -1 default value, something I hardly see as needing an entire new method with lots of added assumptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning -1 is a very easy way to handle not just timeouts but various numeric parameters that might end up including infinity and the above is a simple solution that only uses -1 default value, something I hardly see as needing an entire new method with lots of added assumptions.

Then, almost all the cases of handling timeouts may just have like this two lines approach (let's recall the situation of handling numerics before having get_numeric). Anyway, I understand that it's not a must to this PR, so I'd leave the proposal for future discussion, thanks.

Copy link
Contributor

@luckyh luckyh left a comment

Choose a reason for hiding this comment

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

LGTM

@pevogam
Copy link
Contributor Author

pevogam commented Nov 4, 2022

@chunfuwen Thanks for rechecking and reviewing again the previous pull request, this one also has two approvals. Thanks!

@pevogam
Copy link
Contributor Author

pevogam commented Dec 5, 2022

From what I can see this pull request now has enough many approvals. Thanks both!

@pevogam
Copy link
Contributor Author

pevogam commented Feb 7, 2023

From what I can see this pull request now has enough many approvals. Thanks both!

ping @luckyh

@luckyh
Copy link
Contributor

luckyh commented Feb 7, 2023

Alright, let's merge it.

@luckyh luckyh merged commit 270b6b6 into avocado-framework:master Feb 7, 2023
@pevogam pevogam deleted the qemu-convert-improvements branch February 7, 2023 11:52
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.

3 participants