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

Hv placement switch #9

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Hv placement switch #9

wants to merge 3 commits into from

Conversation

DavidFair
Copy link
Collaborator

@DavidFair DavidFair commented Jan 3, 2025

  • Drop support for getting VCPUs / Memory / Disk Usage from Hypervisor, as these are deprecated and return None
  • Drop the enums to fail loud downstream as these will never work again
  • Add placement query object with support for getting usage
  • Major version bump to reflect API breaking changes to HypervisorQuery

TODO pending draft feedback:

  • Docs
  • Unit tests

Testing

from openstackquery import PlacementQuery

if __name__ == "__main__":
    query = PlacementQuery()
    query.select("name", "id")
    query.where("equal_to", "name", value="hv519.nubes.rl.ac.uk")
    query.run("dev")

    results = query.to_objects()
    print(results)

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 69.09091% with 34 lines in your changes missing coverage. Please review.

Project coverage is 98.76%. Comparing base (ba9d83e) to head (7e80e57).

Files with missing lines Patch % Lines
openstackquery/runners/placement_runner.py 47.36% 20 Missing ⚠️
openstackquery/enums/props/placement_properties.py 73.07% 7 Missing ⚠️
openstackquery/mappings/placement_mapping.py 80.00% 6 Missing ⚠️
openstackquery/api/query_objects.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
- Coverage   99.55%   98.76%   -0.80%     
==========================================
  Files         111      115       +4     
  Lines        4244     4282      +38     
==========================================
+ Hits         4225     4229       +4     
- Misses         19       53      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DavidFair DavidFair force-pushed the HV_placement_switch branch from 8a896d9 to 196e886 Compare January 3, 2025 16:03
These resources all return None for HVs, as Yoga+ expects us to use a
resource object instead.

Remove these since they will not work again, and any downstream usage
will fail loudly when they try to import the dead enum.
This will require a major version bump which will come in the next
commit adding the placement replacement.
@DavidFair DavidFair force-pushed the HV_placement_switch branch from 196e886 to 5ce00c8 Compare January 3, 2025 16:06
In Yoga+ placement should be used for determining usage...etc. with
Horizon having switched to it upstream. Unfortunately, we don't have
full support in the latest SDK for getting usage, so add some static
methods and data classes to support this until we/someone else upstream
the work.
Update author to reflect entire group and bump to v1 with latest
breaking changes removing various HV presets
@DavidFair DavidFair force-pushed the HV_placement_switch branch from 5ce00c8 to 7e80e57 Compare January 3, 2025 16:09
@DavidFair
Copy link
Collaborator Author

Questions for draft feedback:

  • Any objections to the approach of getting the usage since the SDK doesn't support it (we can upstream this later), I've tried to match how upstream OpenStack SDK does it to make it easier to contribute
  • Are we happy with serialising to our own dataclass, or will this cause problems?
  • Should it be called Placement* or something else since we only really handle ResourceProviders from the Placement API?

@meoflynn
Copy link
Contributor

meoflynn commented Jan 3, 2025

  • Should it be called Placement* or something else since we only really handle ResourceProviders from the Placement API?

I think it depends on whether we will add more features that interact with the Placement API in the future. If there are other features we want to add that will interact with Placement, then calling it Placement* would be okay. However if we are only going to be interacting with this specific part of the API we should call it something else.

@gmatthews20
Copy link
Collaborator

The switch to using the placement api for hypervisor resource usage was made in #1 which fixed the missing resource fields and was implemented so it was backwards compatible with the hypervisor query. After investigating the issue I think you were having with the find empty hypervisor stackstorm action there was a missing catch for KeyError in the client side filter handler when the resource is not found as is the case for ironic hypervisors.
I have fixed it here #10 and tested that it works with the find empty hypervisor action.

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