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

Create a ConversionHost table #242

Merged
merged 5 commits into from
Aug 16, 2018

Conversation

agrare
Copy link
Member

@agrare agrare commented Aug 7, 2018

This adds a table for conversion hosts which can be polymorphically related to other inventory items i.e. Vms/Hosts.

@agrare
Copy link
Member Author

agrare commented Aug 7, 2018

@fdupont-redhat What other properties do we need to add to a conversion host?

@miq-bot miq-bot added the wip label Aug 7, 2018
@ghost
Copy link

ghost commented Aug 7, 2018

@agrare If it's a VM, how do we get the credentials ? For the host it's already there, but AFAIK it's not for the VM. Or at least it's not visible in the UI.

@agrare
Copy link
Member Author

agrare commented Aug 7, 2018

@fdupont-redhat that's a different issue :) Lets tackle that in a different PR/Trello card.

@ghost
Copy link

ghost commented Aug 7, 2018

@agrare LGTM, then.

@agrare
Copy link
Member Author

agrare commented Aug 7, 2018

@fdupont-redhat really nothing else? Maybe a version string?

@ghost
Copy link

ghost commented Aug 8, 2018

We don't really have a notion of version, but capabilities might be good. Currently, we use tags to store the transport method, but it could be an attribute. Could it be a JSON field ? I don't know what we could store in there: virt-v2v capabilities, transport methods, other things that we don't know yet.

@nyoxi @aufi any idea ?

@aufi
Copy link
Member

aufi commented Aug 9, 2018

E.g. some Conversion host build number would be nice to detect old ones..

@nyoxi
Copy link

nyoxi commented Aug 10, 2018

I can imagine storing various capabilities and versions could make sense. But that brings the question how will you keep them up-to-date with what's really on the host?

@agrare
Copy link
Member Author

agrare commented Aug 10, 2018

@nyoxi that would have to be done by whoever communicates with the code on the conversion host. This won't be able to be updated by inventory refresh because it won't be based on properties available to the RefreshWorker in provider inventory.

@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

Okay @nyoxi I added a version string and a serialized capabilities so we can have some flexibility in what we store there. As long as we're not planning on doing any queries based on capabilities we should be good with that.

@ghost
Copy link

ghost commented Aug 14, 2018

@agrare we do requests based on capabilities. Currently, the only capability we use in transport_method, which can be vddk, ssh, whatever we come up with. IIUC, it might be a good idea to have a specific column for that one, isn't it ?

@agrare agrare removed the wip label Aug 14, 2018
@agrare agrare changed the title [WIP] Create a ConversionHost table Create a ConversionHost table Aug 14, 2018
@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

@bdunne can you take a look?

@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

Currently, the only capability we use in transport_method, which can be vddk, ssh, whatever we come up with. IIUC, it might be a good idea to have a specific column for that one, isn't it ?

Yes it would, I was going off of:

I can imagine storing various capabilities

which seemed pretty vague 😄

If a transport_method string column is all that you need then I'll add that.

@ghost
Copy link

ghost commented Aug 14, 2018

That's all I need. It's the only attribute that is used during host selection, apart from current number of migrations which is calculated.

@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

@fdupont-redhat is that going to be a single value or is it possible for multiple to be supported?

@ghost
Copy link

ghost commented Aug 14, 2018

In the current implementation, we use a single value as it also decides the transport method to use for the migration. But IMO, it should be a per-mapping/plan/vm choice. So, being able to use the same host for many transport methods should be possible, as I don't see why a conversion wouldn't be able to use more than one transport method.

Please make it a multiple value attribute.

@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

Please make it a multiple value attribute.

Going to need multiple columns then, don't want to be doing LIKE queries on a comma separated list :/

@ghost
Copy link

ghost commented Aug 14, 2018

You mean one boolean column per transport method, right ?

@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

@fdupont-redhat correct, is this something that can be inferred from the version? Or is it something that could be different for conversion hosts on the same version?

t.string :name
t.string :address
t.string :type
t.string :resource_type
Copy link
Member

Choose a reason for hiding this comment

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

t.references :resource, :type => :bigint, :polymorphic => true

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 slick

@ghost
Copy link

ghost commented Aug 14, 2018

@agrare It's not related to the version. Version will most likely refer to ovirt-ansible-v2v-conversion-host package version.

@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

@fdupont-redhat okay I added two columns to represent if vddk and/or ssh transport is supported, PTAL

@ghost
Copy link

ghost commented Aug 14, 2018

👍

@agrare
Copy link
Member Author

agrare commented Aug 14, 2018

Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

@bdunne what do you think about disabling this cop? I think this goes against how we want migrations to be written.

@@ -0,0 +1,6 @@
class AddConversionHostIdToMiqTasks < ActiveRecord::Migration[5.0]
def change
add_column :miq_tasks, :conversion_host_id, :bigint
Copy link
Member

Choose a reason for hiding this comment

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

This feels very specific, is there some doc / design I can look describing how this will be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

We wanted to add a method for throttling how many tasks run on a conversion host without having to loop through all active tasks and pull the conversion host out of the task options.

I don't see a trello card yet (/cc @fdupont-redhat) but there is an ongoing design doc here https://docs.google.com/document/d/1ANBPzDLF-id3MS9_8ztmrNjNwh3FJ6yGCvA2BUE808c/edit#heading=h.lzsppid3wjo9

Since this is still in design maybe we should hold off on this and come back and add this later.

Copy link

Choose a reason for hiding this comment

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

@miq-bot
Copy link
Member

miq-bot commented Aug 14, 2018

Checked commits agrare/manageiq-schema@ad331a4~...cf1a489 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 2 offenses detected

db/migrate/20180807153714_add_conversion_host_table.rb

@agrare agrare closed this Aug 14, 2018
@agrare agrare reopened this Aug 14, 2018
@bdunne bdunne merged commit ef7ea8b into ManageIQ:master Aug 16, 2018
@agrare agrare deleted the add_conversion_host_table branch August 16, 2018 13:52
@bdunne bdunne added this to the Sprint 93 Ending Aug 27, 2018 milestone Aug 16, 2018
@ghost
Copy link

ghost commented Oct 2, 2018

@agrare As the resource attribute is polymorphic, what is the purpose of address ? I'd expect it to be the IP address to connect to the conversion host, but for an oVirt host it maps to resource.ipaddress, while for an OpenStack VM it could map to resource.ipaddresses.first or resource.ipv4_address...

How would you implement the polymorphism ? In the ConversionHost class using send and resource_type ?

@agrare
Copy link
Member Author

agrare commented Oct 2, 2018

@fdupont-redhat it was for the case where the conversion host is not in MIQ inventory e.g. a bare metal conversion host.

It looks like both the host and the vm models have an #ipaddresses method that you can use. I'd recommend a method on ConversionHost#ipaddress which could do address || resource.ipaddresses.first that should cover all cases

@ghost
Copy link

ghost commented Oct 2, 2018

@agrare Thanks for the refresh. I'll create IP address methods in ConversionHost class.

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.

6 participants