Fixes #6835 - support filtered API for oVirt #3288

ares · 2016-03-08T12:09:59Z

No description provided.

ohadlevy · 2016-03-08T12:33:31Z

db/migrate/20160308100307_add_filtered_api_to_ovirt.rb

@@ -0,0 +1,5 @@
+class AddFilteredApiToOvirt < ActiveRecord::Migration
+  def change
+    add_column :compute_resources, :filtered_api, :boolean, :default => false, :null => false


you can use the serialized attribute instead of adding db columns that do not fit other compute resources?

I can, I don't want to :-) especially in this table I don't expect thousands of records and I think the number of downsides it brings is too big (migration touching serialized data, possible inconsistency between two objects of same class, faulty implementation that uses method_missing without defining respond_to?)

ares · 2016-03-08T16:30:17Z

@ohadlevy anything else I could do to get this ready for merge?

iNecas · 2016-03-11T13:42:46Z

Tested and works well. 👍 code-wise from me

ares · 2016-03-14T08:02:46Z

Thank you, setting ready to merge so someone with permissions could merge.

domcleal · 2016-03-14T08:14:36Z

Sorry, I'm going to unset it unless a committer's reviewed it. (Looks like that's @ohadlevy.)

ohadlevy · 2016-03-14T08:34:15Z

/me delegate to @tbrisker :)

ares · 2016-03-14T09:57:07Z

Great, it got things moving :-) Sorry if it was too bold.

tbrisker · 2016-03-14T13:44:22Z

app/views/compute_resources/form/_ovirt.html.erb

@@ -1,6 +1,7 @@
 <%= text_f f, :url, :size => "col-md-8", :help_block => _("e.g. https://ovirt.example.com/api"), :help_inline => documentation_button('5.2.7oVirt/RHEVNotes') %>
 <%= text_f f, :user, :help_block => _("e.g. admin@internal") %>
 <%= password_f f, :password, :unset => unset_password? %>
+<%= checkbox_f f, :filtered_api, :help_block => _("If you don't have admin permissions you can use slower filtered API, that works with user account."), :label => _('Filtered API') %>


How about making the help_block an inline popover?
Also, nitpick, you are repeating yourself in the text. I think "If you don't have admin permissions you can use slower filtered API." should be enough.

that was my first attempt but it looked weird

but I can add it if you think it's better

I think if you use :help_inline instead of :help_block it should look better.

I'm not sure if it looks any better

This is what we have for interfaces flag, also does not look great to me.

I think I find this the best.

Which one do you prefer?

tbrisker · 2016-03-14T14:02:37Z

@ares tested, this works just fine. Once the comment about the help text is fixed I'll merge.

ares · 2016-03-15T11:42:40Z

@tbrisker I improved error handling that was asked in #3308 (diff). I keep it in separate commit for easier reviewing, I'll squash after review.

ares · 2016-03-17T13:05:22Z

btw tests failures are unrelated

tbrisker · 2016-03-17T14:03:17Z

app/models/compute_resources/foreman/model/ovirt.rb

+          errors[:url] << _('HTTPS URL is required for API access')
+        when /insufficient permissions/
+          # ovirt does not add code 400 to error message
+          errors[:user] << e.message + ' ' + _('If your username and password is correct, try using user level API.')


Also make sure terminology here matches that on the checkbox.

ares · 2016-03-17T14:42:23Z

@tbrisker I've modified texts. Sadly, it's not well documented in oVirt docs and it's always the same API just with different header. Anyway I think this should be clear enough now. I also added the value to v2 API. Please re-review.

tbrisker · 2016-03-17T15:08:08Z

app/views/compute_resources/form/_ovirt.html.erb

@@ -1,6 +1,7 @@
 <%= text_f f, :url, :size => "col-md-8", :help_block => _("e.g. https://ovirt.example.com/api"), :help_inline => documentation_button('5.2.7oVirt/RHEVNotes') %>
 <%= text_f f, :user, :help_block => _("e.g. admin@internal") %>
 <%= password_f f, :password, :unset => unset_password? %>
+<%= checkbox_f f, :filtered_api, :help_inline => _("Use Filter header which makes API limited and slower but is available to non-admin users."), :label => _('User account') %>


Sorry to be nitpicking again on this, I think "Use the 'Filter' header for API calls, which makes them limited and slower, but available to non-admin users." would be clearer, with the label being Filter API calls.
Also the help text now breaks a line and looks bad, please reconsider using a popover.

I wonder if we should consider this a plugin vs something we want to maintain in foreman itself, after talking to a view ovirt guys, they recommended we do not use the filtered api at all.

No plugin please. This pretty much limits the usage of ovirt just to ones with admin rights. If something changes on ovirt side, we can reconsider, but right now, it's really solving real issue for us (and I expect others are in a situation they can't have admin account)

@tbrisker no need to applologize, I'm glad we're moving somewhere. Fixed.

@ohadlevy I'm not sure I properly understand your suggestion, create ovirt plugin that adds this checkbox? On our side it does not add much to maintain. If it does not work on ovirt side, I hope they are willing to fix it.

the problem is that it present a follow up problems, for example: - if you are a large env, and has a lot of hosts, the vm list might not work - associate vms will not work as well either (both listing the vms). this also limit us to support better leverage ovirt, for example, we want for associate vms to use search to find relevant mac addresses instead of in memory searching for all macs, since search is not exposed, we would not be able to use that. I wonder if the better way will be to ask for an admin account - note that in oVirt, admin does not have a full permissions (e.g. superuser) just labeled as admin I also don't like this, but I'm worried that a limitation on oVirt side will create confusion for our users.

@ares I'm not aware they prioritize fixing this issue, maybe @oourfali can expand on it?

@ohadlevy adding such features is great but I'm not sure whether big setup (probably lower % of users) should cut support for feature that would work only with smaller setups (higher number of users I guess)

I can imagine introducing some capabilities model that would tell us what a given CR is capable of, it could be handy also to manage versions of CR (with different feature sets)

tbrisker · 2016-03-17T15:37:56Z

This works and code & text are good to merge as far as I'm concerned once tests finish.
However, we need to make a decision whether we want to make this change at all.
I'm afraid I do not have enough experience with oVirt and I don't know enough about users' use cases to make that call, so I will leave that up to those of you who know more to make the decision.

oourfali · 2016-03-17T15:47:25Z

There are many gaps in the filtered api. It wasn't designed for the use foreman does with the API.
IMHO it shouldn't be pushed, as it might cause other issues. Even if things will work, they will be less performant.

ares · 2016-03-17T16:10:08Z

@oourfali is there a different way to use ovirt API without admin account that foreman (or anything else) could use for automating the provisioning? There was some interest https://bugzilla.redhat.com/show_bug.cgi?id=1123676

iNecas · 2016-03-17T16:14:43Z

@oourfali out next step is to add jsessionid support (we already have support on rbovirt side), from the performance reasons.

oourfali · 2016-03-17T17:04:10Z

The performance aspects are different. Not related to the session id.
The user api works differently, running queries rather than using the search mechanism. So, for different things might be less performant, especially when combined with search.

iNecas · 2016-03-18T08:28:58Z

Regardless the performance limitations, I still think not providing the user-level accounts an ability to use Foreman with this credentials is a serious limitation. We have even customer cases already demanding this. If the ovirt has performance limitations on this use-case, I would rather see improvements on the ovirt side to improve this, rather then avoiding using it at all. If the user-level API was officially deprecated and communicated, that this will not be supported anymore (and the question would be when, because if it's too far away, it still might be worth having it here until the day comes).

If there are use-cases that the filtered API is not good for, in ideal world the API would be explicit enough about that, when this use comes. On the other hand, I can't think of more basic use-case for a virtualization platform than provisioning a host. I can imagine limiting the other cases (such as listing VMs) not being available in case the calls are deprecated/show serious issues. But the basic end-to-end provisioning via API seems to be valid use-case that I would expect from ovirt to handle without need for admin-level account.

From my perspective, this is pretty small change, that unblocks non-admin users using it and can be reverted/disable anytime an Ovirt version is released that deprecates it.

What about a note about about the lower performance in the description?

oourfali · 2016-03-18T09:15:17Z

Foreman is in your hands... We support the user-level API completely, but some things work differently, and some functionality doesn't exist there, by design. we might not be able to handle performance/functionality issues.

You recently added paging support by using our search API, which isn't fully supported and won't be supported in the user-level API.
You are also supposed to change the VM MAC address search to be based on our search, rather than searching locally.

You can use that without paging and search and that will probably work (wasn't tested, though) , but as you rely more on the API in other use-cases in Foreman, you might run into using something that doesn't exist in the user level api, and you'll need to block stuff on your side, or suggest alternatives, if the user chose the filtered view.

In my opinion the requirement to be admin is reasonable. We require the same on the other side of the integration. Some API calls require you to be an admin.
Admin doesn't mean super user, btw, you just need to have one admin role assigned to one object, so in case you're managing things through Foreman - the ovirt admin should give you this minimum permission so you can work with it. Thus, I don't see this as a serious limitation.
How many customers asked for that?

Bottom line, as one of the owners of the oVirt API, my recommendation is not to support that, or support it but block functionality, and in that case we can't promise a blocked functionality will be added, and a performance issue at scale will be resolved.

ares · 2016-03-29T08:57:42Z

Closing, we'll document what admin roles/permissions are required instead.

theforeman-bot added Not yet reviewed Needs testing labels Mar 8, 2016

ohadlevy reviewed Mar 8, 2016
View reviewed changes

domcleal added Needs re-review and removed Not yet reviewed labels Mar 9, 2016

ares added Ready to merge and removed Needs re-review Needs testing labels Mar 14, 2016

domcleal added Needs re-review and removed Ready to merge labels Mar 14, 2016

tbrisker self-assigned this Mar 14, 2016

tbrisker added Waiting on contributor Needs re-review and removed Needs re-review Waiting on contributor labels Mar 14, 2016

tbrisker reviewed Mar 14, 2016
View reviewed changes

tbrisker added Waiting on contributor and removed Needs re-review labels Mar 14, 2016

Fixes theforeman#6835 - support filtered API for oVirt

00b628f

ares force-pushed the fix/6835 branch from 7252b9e to 00b628f Compare March 14, 2016 16:17

theforeman-bot added Needs re-review and removed Waiting on contributor labels Mar 14, 2016

dLobatog removed Needs re-review Needs testing labels Mar 15, 2016

ares force-pushed the fix/6835 branch from a8b18e0 to aa908d5 Compare March 15, 2016 11:41

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 15, 2016

ares force-pushed the fix/6835 branch from aa908d5 to 865e094 Compare March 15, 2016 11:43

tbrisker mentioned this pull request Mar 16, 2016

Added support for VMS search options max/page abenari/rbovirt#102

Closed

tbrisker reviewed Mar 17, 2016
View reviewed changes

ares force-pushed the fix/6835 branch from 865e094 to ceb4ad1 Compare March 17, 2016 14:41

tbrisker reviewed Mar 17, 2016
View reviewed changes

Fixes theforeman#6835 - improve error detection and help text

5f4456b

ares force-pushed the fix/6835 branch from ceb4ad1 to 5f4456b Compare March 17, 2016 15:12

tbrisker added Ready to merge Reached an impasse Needs broader discussion and removed Needs re-review Needs testing labels Mar 17, 2016

ares closed this Mar 29, 2016

ares deleted the fix/6835 branch March 29, 2016 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #6835 - support filtered API for oVirt #3288

Fixes #6835 - support filtered API for oVirt #3288

ares commented Mar 8, 2016

ohadlevy Mar 8, 2016

ares Mar 8, 2016

ares commented Mar 8, 2016

iNecas commented Mar 11, 2016

ares commented Mar 14, 2016

domcleal commented Mar 14, 2016

ohadlevy commented Mar 14, 2016

ares commented Mar 14, 2016

tbrisker Mar 14, 2016

ares Mar 14, 2016

tbrisker Mar 14, 2016

ares Mar 15, 2016

tbrisker commented Mar 14, 2016

ares commented Mar 15, 2016

ares commented Mar 17, 2016

tbrisker Mar 17, 2016

ares commented Mar 17, 2016

tbrisker Mar 17, 2016

ohadlevy Mar 17, 2016 via email

iNecas Mar 17, 2016

ares Mar 17, 2016

ohadlevy Mar 17, 2016 via email

ohadlevy Mar 17, 2016

ares Mar 17, 2016

tbrisker commented Mar 17, 2016

oourfali commented Mar 17, 2016

ares commented Mar 17, 2016

iNecas commented Mar 17, 2016

oourfali commented Mar 17, 2016

iNecas commented Mar 18, 2016

oourfali commented Mar 18, 2016

ares commented Mar 29, 2016

Fixes #6835 - support filtered API for oVirt #3288

Fixes #6835 - support filtered API for oVirt #3288

Conversation

ares commented Mar 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares commented Mar 8, 2016

iNecas commented Mar 11, 2016

ares commented Mar 14, 2016

domcleal commented Mar 14, 2016

ohadlevy commented Mar 14, 2016

ares commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker commented Mar 14, 2016

ares commented Mar 15, 2016

ares commented Mar 17, 2016

Choose a reason for hiding this comment

ares commented Mar 17, 2016

Choose a reason for hiding this comment

ohadlevy Mar 17, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy Mar 17, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker commented Mar 17, 2016

oourfali commented Mar 17, 2016

ares commented Mar 17, 2016

iNecas commented Mar 17, 2016

oourfali commented Mar 17, 2016

iNecas commented Mar 18, 2016

oourfali commented Mar 18, 2016

ares commented Mar 29, 2016