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

Fix handling of job ids in Lancium adapter #280

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

giffels
Copy link
Member

@giffels giffels commented Jan 26, 2023

Job id's at Lancium are integers and the Lancium adapter is treating them also as integers. In addition, the job id is used as remote_resource_uuid in TARDIS and is therefore stored in the persistent drone registry.
In the drone registry the remote_resource_uuid is handled as VARCHAR(255). Thus. when TARDIS is restarted after a shutdown, the remote_resource_uuid is treated as a string in TARDIS.

This pull request fixes the handling of jobd ids in the Lancium adapter by always converting the remote_resource_uuid to an integer before actually checking the status of the corresponding job in Lancium.

@giffels giffels added the bug Something isn't working label Jan 26, 2023
@giffels giffels changed the title Fix lancium adapter Fix handling of job ids in Lancium adapter Jan 26, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2023

Codecov Report

Base: 98.88% // Head: 98.88% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e2155d0) compared to base (a6566c4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #280   +/-   ##
=======================================
  Coverage   98.88%   98.88%           
=======================================
  Files          56       56           
  Lines        2325     2328    +3     
=======================================
+ Hits         2299     2302    +3     
  Misses         26       26           
Impacted Files Coverage Δ
tardis/adapters/sites/lancium.py 100.00% <100.00%> (ø)
tardis/adapters/sites/openstack.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@giffels giffels requested review from a team, rfvc and mschnepf and removed request for a team January 26, 2023 08:28
@giffels giffels marked this pull request as ready for review January 26, 2023 08:28
Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

What about the methods stop_resource and terminate_resourcein the lancium adaper? Is there the conversion also necessary?

@giffels
Copy link
Member Author

giffels commented Jan 26, 2023

What about the methods stop_resource and terminate_resourcein the lancium adaper? Is there the conversion also necessary?

Good point. I think it does not matter here, but it does not harm anyway. So, I have fixed it as well.

@giffels giffels requested a review from mschnepf January 26, 2023 08:57
Copy link
Member

@mschnepf mschnepf 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 your work. It looks good to me. 👍

Copy link
Contributor

@rfvc rfvc left a comment

Choose a reason for hiding this comment

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

Thanks, @giffels, looks good!

@giffels giffels merged commit 248cd6e into MatterMiners:master Jan 27, 2023
@giffels giffels deleted the fix-lancium-adapter branch January 27, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants