On Tue, Apr 25, 2017 at 11:05:28AM +0000, Daniel Alvarez wrote:
> This patch introduces a new type of OVN ports called "localport".
> These ports will be present in every hypervisor and may have the
> same IP/MAC addresses. They are not bound to any chassis and traffic
> to these ports will never go through a tunnel.
>
> Its main use case is the OpenStack metadata API support which relies
> on a local agent running on every hypervisor and serving metadata to
> VM's locally. This service is described in detail at [0].
>
> Signed-off-by: Daniel Alvarez <[email protected]>
Hi Daniel, thanks again for working on this feature. Russell gave me
enough context to review it, so here are some comments.
The commit message cites "[0]" but doesn't say what [0] is.
I'm a little surprised to see external-ids:ovn-localport-port in both
the Port and Interface tables. I think that so far we've applied our
special identifiers to one of these tables or the other, since most of
the time either one or the other is most appropriate. In this case, I
would be inclined to put it just on the Interface. Is there a reason
that it should be present in both?
In consider_local_datapath(), the ovn-localport-port logic only fires if
ovn-localport-port is completely unset. Usually, it's better to make
sure that everything that ovn-controller sets happens every time,
because this fixes up damage if any occurs. So, for example, in this
case, we would tend to always set ovn-localport-port. (Sometimes, this
can be expensive, and so we come up with ways to avoid it, but I don't
have a reason to believe that it is expensive in this case.)
The documentation for external-ids:ovn-localport-port should say what
the key's value is.
Actually, the purpose of external-ids:ovn-localport-port is not entirely
clear to me. The other documented keys in this category have the
following uses:
external-ids:ovn-localnet-port
external-ids:ovn-l2gateway-port
external-ids:ovn-l3gateway-port
ovn-controller creates these ports itself and it adds these
keys to them to indicate that it owns them; otherwise that
would be ambiguous and therefore it would be risky for
ovn-controller to later delete them.
external-ids:ovn-logical-patch-port
ovn-controller doesn't use these anymore; the documentation
is obsolete and should be removed.
But it looks to me that ovn-controller doesn't create this new kind of
port (and doesn't delete it either). It seems that, so far,
ovn-controller only uses this key to internally communicate with two
pieces of itself, which doesn't seem like a great approach to me.
It would be helpful to readers to mention the particular Neutron example
in the documentation, so that they can easily learn more about one use
case for this feature. Maybe it would be appropriate to reference the
OpenStack document that Russell mentioned, as a way to let them learn
more.
There is some unusual indentation in the test here--I would expect the
"ovs-vsctl" and "ovn-nbctl" command names to be indented at the same
level:
+ ovs-vsctl add-port br-int vif${i}1 -- \
+ set Interface vif${i}1 external-ids:iface-id=lp${i}1 \
+ options:tx_pcap=hv${i}/vif${i}1-tx.pcap \
+ options:rxq_pcap=hv${i}/vif${i}1-rx.pcap \
+ ofport-request=${i}1
+
+ ovn-nbctl lsp-add ls1 lp${i}1
+ ovn-nbctl lsp-set-addresses lp${i}1 f0:00:00:00:00:${i}1
+ ovn-nbctl lsp-set-port-security lp${i}1 f0:00:00:00:00:${i}1
+
+ ovn-sbctl list port_binding
+ ovn-sbctl show
+ ovn-sbctl list chassis
+ ovs-vsctl list interface
+ ovs-vsctl list open
+ ovn-nbctl lsp-get-up lp${i}1
This line in the test looks like it's probably something stray from
debugging that should be removed:
+ ps -ef |grep ovn-controller
Also, I noticed that this patch re-set physical_map_changed based on
update_ofports(), although it should have honored an existing 'true'
value there.
Finally, this patch reuses some code from physical_run() that was more
complicated than necessary. Not your fault--it was natural to reuse
it--but I decided to rewrite and simplify it. So, I sent this out as a
3-patch series with that simplification as the first 2 patches. Would
you mind picking it up from that series? It's posted starting here:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-May/331950.html
Thanks again!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
