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 best fit API for transformations moving vms to openstack #455

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

@bdunne
Copy link
Member Author

bdunne commented Aug 21, 2018

@agrare Please review

{
:source_href => mapping["source_href"],
:best_fit => Api::Utils.build_href_slug(Flavor, fit.best_fit_flavor.try(:id)),
:all_fit => fit.available_fit_flavors.collect { |f| Api::Utils.build_href_slug(Flavor, f.id) }
Copy link
Member Author

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯ Naming is hard... if anyone has better suggestions I'm all ears 😄

@bdunne
Copy link
Member Author

bdunne commented Aug 29, 2018

@abellotti Please review

@abellotti
Copy link
Member

I think this is fine, maybe take care of the rubocop warning above.

@bdunne bdunne force-pushed the v2v_cloud_best_fit branch from 40b23c3 to a0065a7 Compare August 29, 2018 18:32
def vm_flavor_fit_resource(_type, _id, data)
data["mappings"].collect do |mapping|
source = Api::Utils.resource_search_by_href_slug(mapping["source_href"])
destination = Api::Utils.resource_search_by_href_slug(mapping["destination_href"])
Copy link
Member

Choose a reason for hiding this comment

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

hmm, the above 2 seems to be confusing to be. *_href implies an API href, i.e. http://localhost:3000/api/vms/1 and not the slug, which is normally named href_slug (for vms/1).

If those are expected to be href_slugs, you can rename then accordingly, i.e. mapping["source_href_slug"] and mapping["destination_href_slug"].

Optionally, you can take an object reference which can take either, so mapping["source"] and mapping["destination"] which identify the object as desired

{ "href" : "http://localhost:3000/api/vms/1" }

or

{ "href_slug" : "vms/1" }

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have an example of the last alternative you mentioned (object reference)?

Copy link
Member

Choose a reason for hiding this comment

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

look at http://manageiq.org/docs/reference/latest/api/reference/users#creating-users for group, it's just a hash which can be one of many identifiers, id, href, description, etc. or zone reference for creating a provider http://manageiq.org/docs/reference/latest/api/reference/providers#creating-providers, same idea. you can look at any of the helper methods for those parse_. In this case you'd add support for href_slug in addition to href for example.

@@ -3366,6 +3366,8 @@
:identifier: transformation_mapping_new
- :name: delete
:identifier: transformation_mapping_delete
- :name: vm_flavor_fit
:identifier: transformation_mapping_new
Copy link
Member

Choose a reason for hiding this comment

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

only folks that can create new mappings can run this action ?

Copy link
Member Author

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯ That's the only use case for it at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh ok 👍 we can always change later if need be. Thanks!!

@bdunne bdunne force-pushed the v2v_cloud_best_fit branch from a0065a7 to 0157bae Compare August 30, 2018 20:56
@miq-bot
Copy link
Member

miq-bot commented Aug 30, 2018

Checked commit bdunne@0157bae with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@abellotti abellotti self-assigned this Aug 31, 2018
@abellotti abellotti requested a review from agrare August 31, 2018 12:30
@abellotti
Copy link
Member

Thanks @bdunne for the update.

@agrare could you take a 👀 when you get a chance ? Thanks.

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@abellotti abellotti added this to the Sprint 94 Ending Sept 10, 2018 milestone Aug 31, 2018
@abellotti
Copy link
Member

Thanks @agrare for the 👀 !!

Thanks @bdunne for the API Enhancement. LGTM!! 👍

@abellotti abellotti merged commit 14b36c3 into ManageIQ:master Aug 31, 2018
@bdunne bdunne deleted the v2v_cloud_best_fit branch August 31, 2018 17:44
@AparnaKarve
Copy link
Contributor

AparnaKarve commented Sep 10, 2018

@bdunne Thank you for this PR.

Recently we completed integrating this API with our frontend work.
While testing, we came across a couple edge cases where best_fit was nil and all_fit was an empty array.

The current UI design expects best_fit to always have some value.
That said, would it be possible to return the largest possible flavor in a situation where the best-fit-flavor logic is not able to determine the best fit flavor?
The largest flavor, as I understand in the context of v2v, would be the flavor with the largest root_disk_size

@bdunne
Copy link
Member Author

bdunne commented Sep 10, 2018

@AparnaKarve At the moment, we are not comparing the root_disk_size of the flavor, only the memory and CPU's. It sounds like the VM in your edge case is larger in some way (memory and/or CPU) than any of the flavors in your Openstack environment. all_fit will be an array of all of the flavors that match or exceed the current memory and CPU allocated to the VM. best_fit will be the smallest of the flavors in all_fit or nil (when the array is empty). I don't think it would be appropriate to return a flavor that does not at least match the currently allocated VM specs.

@AparnaKarve
Copy link
Contributor

Ok, I guess we would have to discuss this use case in detail (possibly during our Wednesday call)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants