Skip to content

Conversation

@gajeshbhat
Copy link
Contributor

@gajeshbhat gajeshbhat commented Aug 6, 2025

Description

This PR adds support for human-readable duration formats (e.g., 2h30m, 5h, 30m) for reservation timeouts in job definitions and API requests, while maintaining full backward compatibility with the existing integer seconds format.

Problem solved: Users currently need to manually calculate seconds for reservation timeouts (e.g., calculating that 2.5 hours = 9000 seconds), making job definitions less readable and error-prone.

Implementation approach:

  • Added a duration parser in testflinger-common that handles various time units (s/m/h/d)
  • Added DurationField to server schema for automatic parsing and validation
  • Server-only implementation to avoid snapcraft packaging limitations with CLI cross-directory dependencies
  • All components gracefully fall back to existing integer parsing for backward compatibility

Resolved issues

Fixes #121 (CERTTF-586)

Documentation

  • Existing docs/reference/test-phases.rst correctly documents server-side duration format usage
  • All public-facing changes are documented with clear examples showing job YAML usage
  • Comprehensive tests included covering:
    • Duration parsing (valid/invalid formats)
    • Server schema validation
    • Backward compatibility scenarios

Web service API changes

Schema changes:

  • Modified ReserveData.timeout field to accept both integers and duration strings
  • Added DurationField for automatic parsing: "2h30m"9000 seconds
  • Versioning: No new endpoints - existing /v1/job endpoint enhanced
  • Backward compatibility: All existing integer timeout values continue to work unchanged
  • Authorization: No changes to existing authorization requirements
  • Performance: No database query changes - only input validation enhancement

Request/Response examples:

// New duration format (converted to seconds internally)
{"reserve_data": {"timeout": "2h30m"}}  → timeout: 9000

// Existing formats still work
{"reserve_data": {"timeout": 3600}}     → timeout: 3600
{"reserve_data": {"timeout": "3600"}}   → timeout: 3600

// Invalid formats return 422 with a clear error message
{"reserve_data": {"timeout": "invalid"}} → 422 "Invalid duration format"

Tests

Testing performed:

  1. Unit tests: Duration parsing logic with comprehensive edge cases
  2. Schema validation: Server accepts/rejects appropriate formats with clear error messages
  3. Backward compatibility: All existing integer timeout formats continue to work
  4. Integration testing: Full job submission workflow with duration formats

Test commands to verify:

# Test duration parsing
cd common && uv run pytest src/testflinger_common/tests/test_duration.py -v

# Test server schema validation
cd server && uv run pytest src/testflinger/tests/test_duration_schema.py -v

# Run all component tests
cd common && uvx --with tox-uv tox
cd server && uvx --with tox-uv tox
cd cli && uvx --with tox-uv tox

Examples of supported formats:

  • 30s, 30sec (30 seconds)
  • 30m, 30min (30 minutes)
  • 5h, 5hour (5 hours)
  • 4d, 4day (4 days)
  • 2h30m (2 hours 30 minutes)
  • 1d5h30m (1 day 5 hours 30 minutes)

Usage in job definitions:

job_queue: myqueue
provision_data:
  url: http://image.url
reserve_data:
  ssh_keys:
    - "lp:user"
  timeout: 2h30m 

Notes

This implementation focuses on server-side parsing to avoid snapcraft packaging limitations while still providing the core benefit of human-readable duration formats in job definitions and API requests.

@gajeshbhat gajeshbhat force-pushed the feature/reservation-timeout-duration-format branch from 0ab473f to a5bcf9d Compare August 6, 2025 21:07
Copy link
Collaborator

@pedro-avalos pedro-avalos left a comment

Choose a reason for hiding this comment

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

Thank you for updating the PR. I just had a chance to look over your changes and have left some comments

@pedro-avalos
Copy link
Collaborator

Also, make sure to sign the CLA with your GitHub email: https://ubuntu.com/legal/contributors

@gajeshbhat
Copy link
Contributor Author

Also, make sure to sign the CLA with your GitHub email: https://ubuntu.com/legal/contributors

I have signed the CLA by submitting the form using the provided link. Thank you, @pedro-avalos for taking the time to review my PR. Please let me know if you have any further questions. Cheers.

@gajeshbhat gajeshbhat force-pushed the feature/reservation-timeout-duration-format branch from ee259e2 to 0293c93 Compare August 13, 2025 03:15
@codecov
Copy link

codecov bot commented Aug 14, 2025

Codecov Report

❌ Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.04%. Comparing base (22bd09d) to head (76f660c).
⚠️ Report is 49 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #772   +/-   ##
=======================================
  Coverage   64.03%   64.04%           
=======================================
  Files          94       94           
  Lines        7889     7905   +16     
  Branches      758      760    +2     
=======================================
+ Hits         5052     5063   +11     
- Misses       2691     2695    +4     
- Partials      146      147    +1     
Flag Coverage Δ *Carryforward flag
agent 69.65% <ø> (ø)
cli 78.41% <ø> (ø) Carriedforward from 22bd09d
device 51.47% <ø> (ø) Carriedforward from 22bd09d
server 84.88% <70.58%> (-0.24%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Components Coverage Δ
Agent 69.65% <ø> (ø)
CLI 78.41% <ø> (ø)
Common ∅ <ø> (∅)
Device Connectors 51.47% <ø> (ø)
Server 84.88% <70.58%> (-0.24%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gajeshbhat
Copy link
Contributor Author

@pedro-avalos, it appears that the CI checks are failing due to a repository policy compliance check. Can you kindly take a look? I was able to pass all the tests running locally.

Copy link
Contributor

@rene-oromtz rene-oromtz left a comment

Choose a reason for hiding this comment

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

Thanks for adding the modifications suggested by Pedro!
I added other nitpicks and suggestions I think can benefit the code, please let me know your thoughts on this.

Also, FYI, just a heads up, the common module is not currently automatically versioned, could you please also increase the version to 1.1.3 in pyproject.yaml? This might require a uv sync in common and server modules to use this new version.

Regarding the CI tests runs, I believe we will need to manually trigger the CI run but this needs to be done after the initial approval

@gajeshbhat
Copy link
Contributor Author

Thanks for adding the modifications suggested by Pedro! I added other nitpicks and suggestions I think can benefit the code, please let me know your thoughts on this.

Also, FYI, just a heads up, the common module is not currently automatically versioned. Could you please also increase the version to 1.1.3 in pyproject.yaml? This may require a uv sync in both common and server modules to utilize this new version.

Regarding the CI test runs, I believe we will need to manually trigger the CI run, but this needs to be done after the initial approval

@rene-oromtz Thank you for the thorough and practical feedback! I've addressed all your suggestions. All 104 server tests pass, including the new duration format tests I put in. Please let me know if you have further questions/queries. Cheers.

@gajeshbhat gajeshbhat requested a review from rene-oromtz August 16, 2025 00:52
@gajeshbhat gajeshbhat force-pushed the feature/reservation-timeout-duration-format branch from d1632f9 to 293ad9d Compare August 16, 2025 01:24
@rene-oromtz
Copy link
Contributor

Thank you for adding the suggested changes! Just one last thing, can you please rebase this branch with latest changes from main? There are conflicts related to common/pyproject.toml and common/uv.lock From what I could see, your branch was still using common v1.1.1 vs v1.1.2 which is available in main.

Besides that, it looks good to me! Thanks again

@gajeshbhat gajeshbhat force-pushed the feature/reservation-timeout-duration-format branch from 293ad9d to 3303c99 Compare August 18, 2025 19:38
@gajeshbhat
Copy link
Contributor Author

Thank you for adding the suggested changes! Just one last thing, can you please rebase this branch with latest changes from main? There are conflicts related to common/pyproject.toml and common/uv.lock From what I could see, your branch was still using common v1.1.1 vs v1.1.2 which is available in main.

Besides that, it looks good to me! Thanks again

Hey @rene-oromtz Thank you for catching that! I've successfully rebased the branch with the latest changes from main. Please let me know if you have any questions. Cheers.

Add support for human-readable duration formats (e.g., "2h30m", "5h", "4d")
in reservation timeout fields, while maintaining backward compatibility
with integer seconds.

Features:
- Parse duration strings with s/m/h/d suffixes and full words
- Support combined formats like "1h30m" or "2d5h30m"
- Comprehensive validation and error handling
- Server-only implementation per maintainer guidance
- Integration tests with job submission validation
- Moved tests to component-level structure for consistency
- Optimized parsing logic and removed redundant checks

Addresses reviewer feedback from Pedro and Rene:
- Simplified regex pattern and optimized negative number checks
- Updated to proper reStructuredText docstring format
- Bumped common module version to 1.1.3 with editable dependency
- Added practical parametrized tests in server/tests/test_v1.py

Fixes: canonical#121 (CERTTF-586)
@gajeshbhat gajeshbhat force-pushed the feature/reservation-timeout-duration-format branch from 62a8a2b to 76f660c Compare August 18, 2025 19:51
@gajeshbhat
Copy link
Contributor Author

The PR Checks need approvals to run. CC: @rene-oromtz

Copy link
Contributor

@rene-oromtz rene-oromtz left a comment

Choose a reason for hiding this comment

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

Unit Test are now passed! Thanks again for taking the time to contribute to this repo and for making the modification suggested by reviewers.

@gajeshbhat
Copy link
Contributor Author

Unit Test are now passed! Thanks again for taking the time to contribute to this repo and for making the modification suggested by reviewers.

Thank you, @rene-oromtz, for taking the time to review my Pull request. I think I might need approval from @pedro-avalos, as he requested changes as well. Testflinger was very helpful in my previous job for setting up hardware test lab automation. I am happy to contribute my time to improve this project. :) I also have another Pull Request for a feature that might be useful. #774. Kindly take a look when you have some free time. Cheers.

Copy link
Collaborator

@pedro-avalos pedro-avalos left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, and thank you @rene-oromtz for taking this PR up while I was out of the office.

@pedro-avalos pedro-avalos merged commit 0ce3b10 into canonical:main Aug 25, 2025
23 of 37 checks passed
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.

reserveration timeout should be possible for Minutes, Hours, or Days

3 participants