<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">
<div apple-content-edited="true"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><br class="Apple-interchange-newline">
</span><br class="Apple-interchange-newline">
</div>
<br>
<div>
<div>On Sep 6, 2013, at 15:36 , "Daniel P. Berrange" <<a href="mailto:berrange@redhat.com">berrange@redhat.com</a>></div>
<div> wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">On Fri, Sep 06, 2013 at 12:22:27PM +0000, Alessandro Pilotti wrote:<br>
<blockquote type="cite"><br>
On Sep 6, 2013, at 14:26 , "Daniel P. Berrange" <<a href="mailto:berrange@redhat.com">berrange@redhat.com</a><<a href="mailto:berrange@redhat.com">mailto:berrange@redhat.com</a>>><br>
wrote:<br>
<br>
<blockquote type="cite">On Fri, Sep 06, 2013 at 10:56:10AM +0000, Alessandro Pilotti wrote:<br>
The RemoteFX feature allows Hyper-V compute nodes to provide GPU acceleration to instances by sharing the host's GPU resources.<br>
<br>
Blueprint: <a href="https://blueprints.launchpad.net/nova/+spec/hyper-v-remotefx">
https://blueprints.launchpad.net/nova/+spec/hyper-v-remotefx</a><br>
<br>
This feature provides big improvements for VDI related scenarios based on OpenStack and Hyper-V compute nodes.<br>
It basically impacts on the driver code only plus an additional optional scheduler filter.<br>
<br>
A full architectural description has been added in the blueprint.<br>
<br>
The patch has been published during H3 on Aug 18th and initially reviewed<br>
on Sept 4th with some very good ideas for improvements at a larger scale,<br>
subsequently implemented on the same day, unfortunately too late in the cycle.<br>
</blockquote>
<br>
Simply adding the blueprint description is not sufficient to remove<br>
my objections. The patch as proposed is too incomplete to be merged IMHO.<br>
I pointed out a number of design flaws in the review. The updated info in<br>
the blueprint just re-inforces my review points, to further demonstrate<br>
why this patch should not be merged as it is coded today. It will require<br>
non-negliglable additional dev work to address this properly I believe,<br>
so I'm afraid that I think this is not suitable material for Havana at<br>
this point in time.<br>
<br>
I already committed an updated patch after your review on the 4th addressing<br>
your observations, agreeing that the initial implementation was missing<br>
flexibility (I only didn't commit the scheduler filter yet, as IMO it<br>
requires a separate dependent patch, but this can be done anytime).<br>
</blockquote>
<br>
IMHO the lack of schedular support is a blocker item. Running a VM with<br>
this feature requires that the scheduler be able to place the VM on a<br>
host which supports the feature. Without this, users are just relying on<br>
lucky placement when trying to boot a VM to use this feature.<br>
</blockquote>
<div><br>
</div>
<div>One detail that has been most probably misanderstood from my previous reply: with the following sentence I meant "now, as part of this blueprint", not "tacked on later". </div>
<div>So, scheduler support (as in a filter) is definitely going to be part of the blueprint in discussion, sorry if this was not clear.</div>
<div><br>
</div>
<div>
<blockquote type="cite">
<blockquote type="cite">(I only didn't commit the scheduler filter yet, as IMO it<br>
requires a separate dependent patch, but this can be done anytime).</blockquote>
</blockquote>
</div>
<div><br>
</div>
<div><br>
</div>
<blockquote type="cite">
<blockquote type="cite">To allow others to add their opinion easily, I'm following up here to
<br>
your objections in the blueprint which basically can be reduced (correct<br>
me if I'm wrong) to the following positions, beside the areas on which<br>
we already agreed regarding scheduling and which have already been<br>
implemented:<br>
<br>
1) You suggest to define the amount of RemoteFX GPU resources required in the flavour<br>
2) I suggest to provide those requirements in the image custom properties (which is how it is currently implemented, see blueprint description as well).<br>
<br>
Beside the fact that the two solutions are IMO not mutually exclusive<br>
as scheduling filters could simply apply both, the reason why I don't<br>
see how flavours should be considered for this patch is simple:<br>
</blockquote>
<br>
IIUC, this feature consumes finite host & network resources. Allowing<br>
users to request it via the image properties is fine, but on its own<br>
I consider that insecure. The administrator needs to be able to control<br>
how can access these finite resources, in the same way that you don't<br>
allow users to specify arbitrary amounts of memory, or as many vCPUS<br>
as they want. It seems that flavours are the only place to do this.<br>
Again, I consider this a blocker, not something to be tacked on later.<br>
<br>
<blockquote type="cite">AFAIK Nova flavors at the moment don't support custom properties<br>
(please correct me if I'm wrong here, I looked at the flavors APIs<br>
and implementation, but I admit my ignorance in the flavor internals), so<br>
</blockquote>
<br>
I'm honestly not sure, since I don't know enough about the flavours<br>
APIs.<br>
<br>
<blockquote type="cite">1) Adding the remotefx requisites at the system metadata level
<br>
<a href="https://github.com/openstack/nova/blob/master/nova/compute/flavors.py#L60">https://github.com/openstack/nova/blob/master/nova/compute/flavors.py#L60</a><br>
would be IMO wrong, as it would tightly couple a hypervisor specific limit<br>
with a generic compute API.<br>
2) Adding custom properties to flavors goes way beyond the scope of this blueprint.<br>
<br>
>From an administrative and ACL perspective, administrators can selectively provide access to users to a given image, thus limiting the access to RemoteFX GPU resources (video memory in primis).<br>
<br>
Being able to do it ALSO at the flavour level would add additional granularity,<br>
e.g. assigning different screen resolutions, using a single image. I personally<br>
see this as a separate blueprint as it impacts on the design of a fundamental<br>
Nova feature that will need quite some discussion.<br>
</blockquote>
<br>
That is quite likely/possibly correct.<br>
<br>
That we're having this level of design debate, is exactly why I think this<br>
is not suitable for a feature freeze exception. Freeze exception is for<br>
things that are basically complete, baring small bug fixes / changes.<br>
<br>
Daniel<br>
-- <br>
|: <a href="http://berrange.com">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/">http://www.flickr.com/photos/dberrange/</a> :|<br>
|: <a href="http://libvirt.org">http://libvirt.org</a> -o- <a href="http://virt-manager.org">http://virt-manager.org</a> :|<br>
|: <a href="http://autobuild.org">http://autobuild.org</a> -o- <a href="http://search.cpan.org/~danberr/">http://search.cpan.org/~danberr/</a> :|<br>
|: <a href="http://entangle-photo.org">http://entangle-photo.org</a> -o- <a href="http://live.gnome.org/gtk-vnc">http://live.gnome.org/gtk-vnc</a> :|<br>
<br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev<br>
</blockquote>
</div>
<br>
</body>
</html>