On Fri, Oct 19, 2012 at 1:19 PM, Ben Pfaff <span dir="ltr">&lt;<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>&gt;</span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Fri, Oct 19, 2012 at 11:06:27AM -0700, Gurucharan Shetty wrote:<br>
&gt; Add a new command - &quot;restart&quot; to ovs-ctl. Calling this command<br>
&gt; will save and restore the Openflow flows on each bridge while<br>
&gt; stopping and starting the userspace daemons respectively.<br>
&gt;<br>
&gt; Also, during a force-reload-kmod, save the flows and kernel datapath<br>
&gt; configuration. Use the saved datapath configuration while readding<br>
&gt; the kernel module and the flows while starting the userspace daemons.<br>
&gt;<br>
&gt; Feature #13555.<br>
&gt; Signed-off-by: Gurucharan Shetty &lt;<a href="mailto:gshetty@nicira.com">gshetty@nicira.com</a>&gt;<br>
<br>
</div>Thanks for doing this work.  Working on the startup scripts is<br>
difficult work.<br>
<br>
I think that &quot;ovs-save save-flows&quot; creates additional temporary files,<br>
with mktemp, that never get removed.  If so, one way to avoid that<br>
would be to write to a single script commands of the form:<br>
<br>
        ovs-ofctl add-flows &lt;bridge&gt; - &lt;&lt;EOF<br>
        ...flows...<br>
        EOF<br>
<br>
(In that case, logging the entire script for restoring the flows is no<br>
longer a good idea.)<br></blockquote><div>The reason I chose to go ahead with the undeleted tmp files model </div><div>was that the tmp files could be used to debug the issue</div><div>later if restoring flows fail for some reason. But I guess, it is bad</div>
<div>etiquette to leave them undeleted.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It looks like force-reload-kmod will create two temporary files and<br>
try to ensure that both of them are deleted with two separate commands<br>
like this:<br>
<div class="im">    trap &#39;rm -f &quot;${script_flows}&quot;&#39; 0 1 2 13 15<br>
</div>But each &quot;trap&quot; for a signal replaces the previous one, so only one of<br>
those files will actually be deleted.<br></blockquote><div>I will correct this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The logging seems more elaborate than I would expect in<br>
restore_datapath and in stop_forwarding.  These functions now log the<br>
commands that they are running and their success or failure.  Although<br>
that&#39;s reasonable to do, if we are going to do it for these particular<br>
actions then I&#39;d think we&#39;d want to do that for pretty much every<br>
action.  (If you think that&#39;s a good idea then I&#39;d be OK with it.)<br></blockquote><div><br></div><div>I looked at the model we have for &quot;Restoring interface configuration&quot;</div><div>action and I was confused about what to do for the 2 new additions</div>
<div>that I am making. I will get rid of the logging for the 2 new additions.</div><div>Should I keep the old logging intact?</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
I think that this will save the old flows even if one runs plain<br>
&quot;stop&quot;, which is just a waste in that case.  I think it would be<br>
better to only save them for the cases where they will be used<br>
(&quot;force-reload-kmod&quot; and &quot;restart&quot;, right?).<br></blockquote><div>I will correct this.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

In force_reload_kmod, this change moves stopping ovs-vswitchd from<br>
after starting and stopping the database to before starting and<br>
stopping the database.  That will extend the period of time when new<br>
flows cannot be set up.  Is it really necessary to do it?<br></blockquote><div>In stop_forwarding, I use &#39;ovs-vsctl list-br&#39; command which needs the</div><div>database. According to the comment in the code, starting a large database</div>
<div>may take time. So, I wanted to save the flows first and then stop the database.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
For the same reason, in the &quot;restart&quot; case, is it possible to change<br>
the order to stop/start ovsdb, then stop/start ovs-vswitchd?<br></blockquote><div>Out here too, I will need the bridges before hand. Wouldn&#39;t it delay things</div><div>if I start the database first? I can get the bridges first, and then stop the database.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In force_reload_kmod, it would be better, if it is possible, to save<br>
the datapaths after stopping ovs-vswitchd, because that avoids a race<br>
between saving the datapaths and ovs-vswitchd modifying them.<br></blockquote><div>I will correct this. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
It&#39;s not obvious to me why saving datapaths and saving interfaces are<br>
separate steps.  Can we combine these steps?<br></blockquote><div>Are you saying, make a single function call to ovs-save? Or make 2 function</div><div>calls, but use the same script to store the results?</div><div><br></div>
<div>Thanks,</div><div>Guru</div><div><br></div><div> </div></div><br>