<div dir="ltr">Hi Eugene thanks for bringing this up for discussion. My comments inline.<div>Thanks,</div><div>Armando</div><div><div class="gmail_extra"><br><div class="gmail_quote">On 5 November 2014 12:07, Eugene Nikanorov <span dir="ltr"><<a href="mailto:enikanorov@mirantis.com" target="_blank">enikanorov@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi folks,<div><br></div><div>I'd like to raise a discussion kept in irc and in gerrit recently: <a href="https://review.openstack.org/#/c/131944/" target="_blank">https://review.openstack.org/#/c/131944/</a></div><div><br></div><div>The intention of the patch is to clean up particular scheduling method/interface:</div><div>schedule_network.</div><div><br></div><div>Let me clarify why I think it needs to be done (beside code api consistency reasons):</div><div>Scheduling process is ultimately just a two steps:</div><div>1) choosing appropriate agent for the network</div><div>2) adding binding between the agent and the network</div><div>To perform those two steps one doesn't need network object, network_id is satisfactory for this need.</div></div></blockquote><div><br></div><div>I would argue that it isn't, actually. </div><div><br></div><div>You may need to know the state of the network to make that placement decision. Just passing the id may cause the scheduling logic to issue an extra DB query that can be easily avoided if the right interface between the caller of a scheduler and the scheduler itself was in place. For instance we cannot fix [1] (as you pointed out) today because the method only accepts a dict that holds just a partial representation of the network. If we had the entire DB object we would avoid that and just passing the id is going in the opposite direction IMO</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>However, there is a concern, that having full dict (or full network object) could allow us to do more flexible things in step 1 like deciding, whether network should be scheduled at all. </div></div></blockquote><div><br></div><div>That's the whole point of scheduling, is it not? If you are arguing that we should split the schedule method into two separate steps (get_me_available_agent and bind_network_to_agent), and make the caller of the schedule method carry out the two step process by itself, I think it could be worth exploring that, but at this point I don't believe this is the right refactoring. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>See the TODO for the reference:</div></div></blockquote><div><br></div><div>[1] </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><a href="https://github.com/openstack/neutron/blob/master/neutron/scheduler/dhcp_agent_scheduler.py#L64" target="_blank">https://github.com/openstack/neutron/blob/master/neutron/scheduler/dhcp_agent_scheduler.py#L64</a><br></div><div><br></div><div>However, this just puts an unnecessary (and actually, incorrect) requirement on the caller, to provide the network dict, mainly because caller doesn't know what content of the dict the callee (scheduler driver) expects.</div></div></blockquote><div><br></div><div>Why is it incorrect? We should move away from dictionaries and passing objects so that they can be reused where it makes sense without incurring in the overhead of re-fetching the object associated to the uuid when needed. We can even hide the complexity of refreshing the copy of the object every time it is accessed if needed. With information hiding and encapsulation we can wrap this logic in one place without scattering it around everywhere in the code base, like it's done today.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Currently scheduler is only interested in ID, if there is another scheduling driver,</div></div></blockquote><div><br></div><div>No, the scheduler needs to know about the state of the network to do proper placement. It's a side-effect of the default scheduling (i.e. random). If we want to do more intelligent placement we need the state of the network. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>it may now require additional parameters (like list of full subnet dicts) in the dict which may or may not be provided by the calling code.</div><div>Instead of making assumptions about what is in the dict, it's better to go with simpler and clearer interface that will allow scheduling driver to do whatever makes sense to it. In other words: caller provides id, driver fetches everything it</div><div>needs using the id. For existing scheduling drivers it's a no-op.</div></div></blockquote><div><br></div><div>Again, the problem lies with the fact that we're passing dictionaries around.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>I think l3 scheduling is an example of interface done in the more right way; to me it looks clearer and more consistent.</div></div></blockquote><div><br></div><div>I may argue that the l3 scheduling api is the bad example for the above mentioned reasons.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Thanks,</div><div>Eugene.</div></div></blockquote><div><br></div><div>At this point I am still not convinced by the arguments provided that the patch <a href="https://review.openstack.org/#/c/131944/" target="_blank">131944</a> should go forward as it is.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><br></div><div><br></div></div>
<br>_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br></blockquote></div></div></div></div>