| Message ID | 20211122112256.2011194-9-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | python: add flow parsing library | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
Just some small comments below, and the request to fix up the comments. //Eelco On 22 Nov 2021, at 12:22, Adrian Moreno wrote: > A ODPFlow is a Flow with the following sections: > ufid > info (e.g: bytes, packets, dp, etc) > match > actions > > Use a factory class ODPFlowFactory to cache the decoder objects. > > Only three datapath actions require special handling: > gre: because it has double parenthesys Parenthesis > geneve: because it supports many concatenated lists of options > nat: we reuse the decoder used for openflow actions > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > python/automake.mk | 3 +- > python/ovs/flows/odp.py | 723 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 725 insertions(+), 1 deletion(-) > create mode 100644 python/ovs/flows/odp.py > > diff --git a/python/automake.mk b/python/automake.mk > index d1464d7f6..8b0713cfc 100644 > --- a/python/automake.mk > +++ b/python/automake.mk > @@ -48,7 +48,8 @@ ovs_pyfiles = \ > python/ovs/flows/list.py \ > python/ovs/flows/flow.py \ > python/ovs/flows/ofp.py \ > - python/ovs/flows/ofp_act.py > + python/ovs/flows/ofp_act.py \ > + python/ovs/flows/odp.py Add in alphabetical order. > # These python files are used at build time but not runtime, > # so they are not installed. > diff --git a/python/ovs/flows/odp.py b/python/ovs/flows/odp.py > new file mode 100644 > index 000000000..8fe721f86 > --- /dev/null > +++ b/python/ovs/flows/odp.py > @@ -0,0 +1,723 @@ > +""" Defines an Openvswitch Datapath Flow Defines an Open vSwitch Datapath Flow. > +""" > +import re > +from functools import partial > + > +from ovs.flows.flow import Flow, Section > + > +from ovs.flows.kv import ( > + KVParser, > + KVDecoders, > + nested_kv_decoder, > + decode_nested_kv, > +) > +from ovs.flows.decoders import ( > + decode_default, > + decode_time, > + decode_int, > + decode_mask, > + Mask8, > + Mask16, > + Mask32, > + Mask64, > + Mask128, > + IPMask, > + EthMask, > + decode_free_output, > + decode_flag, > + decode_nat, > +) > + > + > +class ODPFlow(Flow): > + def __init__(self, sections, raw="", id_=None): > + """Constructor""" > + super(ODPFlow, self).__init__(sections, raw, id_) > + > + def __str__(self): > + if self._orig: > + return self._orig > + else: > + return self.to_string() > + > + def to_string(self): > + """Print a text representation of the flow""" > + string = "Info: {}\n" + self.info > + string += "Match : {}\n" + self.match > + string += "Actions: {}\n " + self.actions > + return string > + Trying to understand why you have a special ODPFlowFactory class to return an ODPFLow() object from a string? Can you not just add a static method to the ODPFlow class, so it would simply become a matter of calling: ODPFlow.from_string(string, id=id_data) Or probably more OOO is to override the __init__ function of ODPFlow() to take only the string and the id. Don’t think there is a use case to call ODPFlow() with sections and raw. > +class ODPFlowFactory: > + """Datapath Flow""" > + > + def __init__(self): > + self.info_decoders = self._info_decoders() > + self.match_decoders = self._match_decoders() > + self.action_decoders = self._action_decoders() > + > + def from_string(self, odp_string, id=None): > + """Parse a odp flow string > + > + The string is expected to have the follwoing format: > + [ufid], [match] [flow data] actions:[actions] > + > + Args: > + odp_string (str): a datapath flow string > + > + Returns: > + an ODPFlow instance > + """ > + > + sections = [] > + > + # If UFID present, parse it and > + ufid_pos = odp_string.find("ufid:") > + if ufid_pos >= 0: > + ufid_string = odp_string[ > + ufid_pos : (odp_string[ufid_pos:].find(",") + 1) > + ] > + ufid_parser = KVParser(KVDecoders({"ufid": decode_default})) > + ufid_parser.parse(ufid_string) > + if len(ufid_parser.kv()) != 1: > + raise ValueError("malformed odp flow: %s" % odp_string) > + sections.append( > + Section("ufid", ufid_pos, ufid_string, ufid_parser.kv()) > + ) > + > + action_pos = odp_string.find("actions:") > + if action_pos < 0: > + raise ValueError("malformed odp flow: %s" % odp_string) > + > + # rest of the string is between ufid and actions > + rest = odp_string[ > + (ufid_pos + len(ufid_string) if ufid_pos >= 0 else 0) : action_pos > + ] > + > + action_pos += 8 # len("actions:") > + actions = odp_string[action_pos:] > + > + field_parts = rest.lstrip(" ").partition(" ") > + > + if len(field_parts) != 3: > + raise ValueError("malformed odp flow: %s" % odp_string) > + > + match = field_parts[0] > + info = field_parts[2] > + > + iparser = KVParser(KVDecoders(self.info_decoders)) > + iparser.parse(info) Looking at the two lines above, to me, it looks like the object might not have been defined right (but I’m not an OOO expert). I would have designed it as, KVParser(string, decoders=None), this way you would have a single object initialization, so: parser = KVParser(info, KVDecoders(self.info_decoders)) Also because the parse() method does not allow re-use of the object, meaning parse another string with the same instance. Or was there a specific reason to split this in two stages? > + isection = Section( > + name="info", > + pos=odp_string.find(info), > + string=info, > + data=iparser.kv(), > + ) > + sections.append(isection) > + > + mparser = KVParser(KVDecoders(self.match_decoders)) > + mparser.parse(match) > + msection = Section( > + name="match", > + pos=odp_string.find(match), > + string=match, > + data=mparser.kv(), > + ) > + sections.append(msection) > + > + aparser = KVParser( > + KVDecoders(self.action_decoders, default_free=decode_free_output) > + ) > + aparser.parse(actions) > + asection = Section( > + name="actions", > + pos=action_pos, > + string=actions, > + data=aparser.kv(), > + is_list=True, > + ) > + sections.append(asection) > + > + return ODPFlow(sections, odp_string, id) > + > + @classmethod Any reason why all of these are @classmethod, and not @staticmethod, as they do not seem to need a reference to self? Guess this is also true in some of the previous patches, but I did not notice ;) > + def _action_decoders(cls): > + _decoders = { > + "drop": decode_flag, > + "lb_output": decode_int, > + "trunc": decode_int, > + "recirc": decode_int, > + "userspace": nested_kv_decoder( > + KVDecoders( > + { > + "pid": decode_int, > + "sFlow": nested_kv_decoder( > + KVDecoders( > + { > + "vid": decode_int, > + "pcp": decode_int, > + "output": decode_int, > + } > + ) > + ), > + "slow_path": decode_default, > + "flow_sample": nested_kv_decoder( > + KVDecoders( > + { > + "probability": decode_int, > + "collector_sed_id": decode_int, > + "obs_domain_id": decode_int, > + "obs_point_id": decode_int, > + "output_port": decode_default, > + "ingress": decode_flag, > + "egress": decode_flag, > + } > + ) > + ), > + "ipfix": nested_kv_decoder( > + KVDecoders( > + { > + "output_port": decode_default, > + } > + ) > + ), > + "controller": nested_kv_decoder( > + KVDecoders( > + { > + "reason": decode_int, > + "dont_send": decode_int, > + "continuation": decode_int, > + "recirc_id": decode_int, > + "rule_cookie": decode_int, > + "controller_id": decode_int, > + "max_len": decode_int, > + } > + ) > + ), > + "userdata": decode_default, > + "actions": decode_flag, > + "tunnel_out_port": decode_int, > + "push_eth": nested_kv_decoder( > + KVDecoders( > + { > + "src": EthMask, > + "dst": EthMask, > + "type": decode_int, > + } > + ) > + ), > + "pop_eth": decode_flag, > + } > + ) > + ), > + "set": nested_kv_decoder(KVDecoders(cls._field_decoders())), > + "push_vlan": nested_kv_decoder( > + KVDecoders( > + { > + "vid": decode_int, > + "pcp": decode_int, > + "cfi": decode_int, > + "tpid": decode_int, > + } > + ) > + ), > + "pop_vlan": decode_flag, > + "push_nsh": nested_kv_decoder( > + KVDecoders( > + { > + "flags": decode_int, > + "ttl": decode_int, > + "mdtype": decode_int, > + "np": decode_int, > + "spi": decode_int, > + "si": decode_int, > + "c1": decode_int, > + "c2": decode_int, > + "c3": decode_int, > + "c4": decode_int, > + "md2": decode_int, > + } > + ) > + ), > + "pop_nsh": decode_flag, > + "tnl_pop": decode_int, > + "ct_clear": decode_flag, > + "ct": nested_kv_decoder( > + KVDecoders( > + { > + "commit": decode_flag, > + "force_commit": decode_flag, > + "zone": decode_int, > + "mark": Mask32, > + "label": Mask128, > + "helper": decode_default, > + "timeout": decode_default, > + "nat": decode_nat, > + } > + ) > + ), > + **cls._tnl_action_decoder(), > + } > + > + _decoders["clone"] = nested_kv_decoder( > + KVDecoders(decoders=_decoders, default_free=decode_free_output) > + ) > + > + return { > + **_decoders, > + "sample": nested_kv_decoder( > + KVDecoders( > + { > + "sample": (lambda x: float(x.strip("%"))), > + "actions": nested_kv_decoder( > + KVDecoders( > + decoders=_decoders, > + default_free=decode_free_output, > + ) > + ), > + } > + ) > + ), > + "check_pkt_len": nested_kv_decoder( > + KVDecoders( > + { > + "size": decode_int, > + "gt": nested_kv_decoder( > + KVDecoders( > + decoders=_decoders, > + default_free=decode_free_output, > + ) > + ), > + "le": nested_kv_decoder( > + KVDecoders( > + decoders=_decoders, > + default_free=decode_free_output, > + ) > + ), > + } > + ) > + ), > + } > + > + @classmethod > + def _tnl_action_decoder(cls): > + return { > + "tnl_push": nested_kv_decoder( > + KVDecoders( > + { > + "tnl_port": decode_int, > + "header": nested_kv_decoder( > + KVDecoders( > + { > + "size": decode_int, > + "type": decode_int, > + "eth": nested_kv_decoder( > + KVDecoders( > + { > + "src": EthMask, > + "dst": EthMask, > + "dl_type": decode_int, > + } > + ) > + ), > + "ipv4": nested_kv_decoder( > + KVDecoders( > + { > + "src": IPMask, > + "dst": IPMask, > + "proto": decode_int, > + "tos": decode_int, > + "ttl": decode_int, > + "frag": decode_int, > + } > + ) > + ), > + "ipv6": nested_kv_decoder( > + KVDecoders( > + { > + "src": IPMask, > + "dst": IPMask, > + "label": decode_int, > + "proto": decode_int, > + "tclass": decode_int, > + "hlimit": decode_int, > + } > + ) > + ), > + "udp": nested_kv_decoder( > + KVDecoders( > + { > + "src": decode_int, > + "dst": decode_int, > + "dsum": Mask16, > + } > + ) > + ), > + "vxlan": nested_kv_decoder( > + KVDecoders( > + { > + "flags": decode_int, > + "vni": decode_int, > + } > + ) > + ), > + "geneve": nested_kv_decoder( > + KVDecoders( > + { > + "oam": decode_flag, > + "crit": decode_flag, > + "vni": decode_int, > + "options": partial( > + decode_geneve, False > + ), > + } > + ) > + ), > + "gre": decode_tnl_gre, > + "erspan": nested_kv_decoder( > + KVDecoders( > + { > + "ver": decode_int, > + "sid": decode_int, > + "idx": decode_int, > + "sid": decode_int, > + "dir": decode_int, > + "hwid": decode_int, > + } > + ) > + ), > + "gtpu": nested_kv_decoder( > + KVDecoders( > + { > + "flags": decode_int, > + "msgtype": decode_int, > + "teid": decode_int, > + } > + ) > + ), > + } > + ) > + ), > + "out_port": decode_int, > + } > + ) > + ) > + } > + > + @classmethod > + def _info_decoders(cls): > + return { > + "packets": decode_int, > + "bytes": decode_int, > + "used": decode_time, > + "flags": decode_default, > + "dp": decode_default, > + } > + > + @classmethod > + def _match_decoders(cls): > + return { > + **cls._field_decoders(), > + "encap": nested_kv_decoder(KVDecoders(cls._field_decoders())), > + } > + > + @classmethod > + def _field_decoders(cls): > + return { > + "skb_priority": Mask32, > + "skb_mark": Mask32, > + "recirc_id": decode_int, > + "dp_hash": Mask32, > + "ct_state": decode_default, # TODO: Parse flags Guess we might want to fix this before the merge? > + "ct_zone": Mask16, > + "ct_mark": Mask32, > + "ct_label": Mask128, > + "ct_tuple4": nested_kv_decoder( > + KVDecoders( > + { > + "src": IPMask, > + "dst": IPMask, > + "proto": Mask8, > + "tcp_src": Mask16, > + "tcp_dst": Mask16, > + } > + ) > + ), > + "ct_tuple6": nested_kv_decoder( > + KVDecoders( > + { > + "src": IPMask, > + "dst": IPMask, > + "proto": Mask8, > + "tcp_src": Mask16, > + "tcp_dst": Mask16, > + } > + ) > + ), > + "tunnel": nested_kv_decoder( > + KVDecoders( > + { > + "tun_id": Mask64, > + "src": IPMask, > + "dst": IPMask, > + "ipv6_src": IPMask, > + "ipv6_dst": IPMask, > + "tos": Mask8, > + "ttl": Mask8, > + "tp_src": Mask16, > + "tp_dst": Mask16, > + "erspan": nested_kv_decoder( > + KVDecoders( > + { > + "ver": Mask8, > + "idx": Mask32, > + "sid": decode_int, > + "dir": Mask8, > + "hwid": Mask8, > + } > + ) > + ), > + "vxlan": nested_kv_decoder( > + KVDecoders( > + { > + "gbp": nested_kv_decoder( > + KVDecoders( > + { > + "id": Mask16, > + "flags": Mask8, > + } > + ) > + ) > + } > + ) > + ), > + "geneve": partial(decode_geneve, True), > + "gtpu": nested_kv_decoder( > + KVDecoders( > + { > + "flags": Mask8, > + "msgtype": Mask8, > + } > + ) > + ), > + "flags": decode_default, > + } > + ) > + ), > + "in_port": decode_default, > + "eth": nested_kv_decoder( > + KVDecoders( > + { > + "src": EthMask, > + "dst": EthMask, > + } > + ) > + ), > + "vlan": nested_kv_decoder( > + KVDecoders( > + { > + "vid": Mask16, > + "pcp": Mask16, > + "cfi": Mask16, > + } > + ) > + ), > + "eth_type": Mask16, > + "mpls": nested_kv_decoder( > + KVDecoders( > + { > + "label": Mask32, > + "tc": Mask32, > + "ttl": Mask32, > + "bos": Mask32, > + } > + ) > + ), > + "ipv4": nested_kv_decoder( > + KVDecoders( > + { > + "src": IPMask, > + "dst": IPMask, > + "proto": Mask8, > + "tos": Mask8, > + "ttl": Mask8, > + "frag": decode_default, > + } > + ) > + ), > + "ipv6": nested_kv_decoder( > + KVDecoders( > + { > + "src": IPMask, > + "dst": IPMask, > + "label": decode_mask(20), > + "proto": Mask8, > + "tclass": Mask8, > + "hlimit": Mask8, > + "frag": decode_default, > + } > + ) > + ), > + "tcp": nested_kv_decoder( > + KVDecoders( > + { > + "src": Mask16, > + "dst": Mask16, > + } > + ) > + ), > + "tcp_flags": decode_default, > + "udp": nested_kv_decoder( > + KVDecoders( > + { > + "src": Mask16, > + "dst": Mask16, > + } > + ) > + ), > + "sctp": nested_kv_decoder( > + KVDecoders( > + { > + "src": Mask16, > + "dst": Mask16, > + } > + ) > + ), > + "icmp": nested_kv_decoder( > + KVDecoders( > + { > + "type": Mask8, > + "code": Mask8, > + } > + ) > + ), > + "icmpv6": nested_kv_decoder( > + KVDecoders( > + { > + "type": Mask8, > + "code": Mask8, > + } > + ) > + ), > + "arp": nested_kv_decoder( > + KVDecoders( > + { > + "sip": IPMask, > + "tip": IPMask, > + "op": Mask16, > + "sha": EthMask, > + "tha": EthMask, > + } > + ) > + ), > + "nd": nested_kv_decoder( > + KVDecoders( > + { > + "target": IPMask, > + "sll": EthMask, > + "tll": EthMask, > + } > + ) > + ), > + "nd_ext": nested_kv_decoder( > + KVDecoders( > + { > + "nd_reserved": Mask32, > + "nd_options_type": Mask8, > + } > + ) > + ), > + "packet_type": nested_kv_decoder( > + KVDecoders( > + { > + "ns": Mask16, > + "id": Mask16, > + } > + ) > + ), > + "nsh": nested_kv_decoder( > + KVDecoders( > + { > + "flags": Mask8, > + "mdtype": Mask8, > + "np": Mask8, > + "spi": Mask32, > + "si": Mask8, > + "c1": Mask32, > + "c2": Mask32, > + "c3": Mask32, > + "c4": Mask32, > + } > + ) > + ), > + } > + > + > +def decode_geneve(mask, value): > + """ > + Decode geneve options. Used for both tnl_push(header(geneve(options()))) > + action and tunnel(geneve()) match. > + > + It has the following format: > + > + {class=0xffff,type=0x80,len=4,0xa} > + > + Args: > + mask (bool): Whether masking is supported > + value (str): The value to decode > + """ > + if mask: Create some consistency around the ending of doctext and the first line of code (in all patches). Sometimes you have an empty line, like in decode_tnl_gre below, and sometimes not like here. > + decoders = { > + "class": Mask16, > + "type": Mask8, > + "len": Mask8, > + } > + > + def free_decoder(value): > + return "data", Mask128(value) > + > + else: > + decoders = { > + "class": decode_int, > + "type": decode_int, > + "len": decode_int, > + } > + > + def free_decoder(value): > + return "data", decode_int(value) > + > + result = [] > + for opts in re.findall(r"{.*?}", value): > + result.append( > + decode_nested_kv( > + KVDecoders(decoders=decoders, default_free=free_decoder), > + opts.strip("{}"), > + ) > + ) > + return result > + > + > +def decode_tnl_gre(value): > + """ > + Decode tnl_push(header(gre())) action > + > + It has the following format: > + > + gre((flags=0x2000,proto=0x6558),key=0x1e241)) > + > + Args: > + value (str): The value to decode > + """ > + > + return decode_nested_kv( > + KVDecoders( > + { > + "flags": decode_int, > + "proto": decode_int, > + "key": decode_int, > + "csum": decode_int, > + "seq": decode_int, > + } > + ), > + value.replace("(", "").replace(")", ""), > + ) > -- > 2.31.1
On 12/23/21 14:08, Eelco Chaudron wrote: > Just some small comments below, and the request to fix up the comments. > Thanks. > > > Trying to understand why you have a special ODPFlowFactory class to return an ODPFLow() object from a string? > Can you not just add a static method to the ODPFlow class, so it would simply become a matter of calling: > > ODPFlow.from_string(string, id=id_data) > > Or probably more OOO is to override the __init__ function of ODPFlow() to take only the string and the id. > Don’t think there is a use case to call ODPFlow() with sections and raw. > I'll implement decoders caching via static class variables in the next version. >> +class ODPFlowFactory: >> + """Datapath Flow""" >> + >> + def __init__(self): >> + self.info_decoders = self._info_decoders() >> + self.match_decoders = self._match_decoders() >> + self.action_decoders = self._action_decoders() >> + >> + def from_string(self, odp_string, id=None): >> + """Parse a odp flow string >> + >> + The string is expected to have the follwoing format: >> + [ufid], [match] [flow data] actions:[actions] >> + >> + Args: >> + odp_string (str): a datapath flow string >> + >> + Returns: >> + an ODPFlow instance >> + """ >> + >> + sections = [] >> + >> + # If UFID present, parse it and >> + ufid_pos = odp_string.find("ufid:") >> + if ufid_pos >= 0: >> + ufid_string = odp_string[ >> + ufid_pos : (odp_string[ufid_pos:].find(",") + 1) >> + ] >> + ufid_parser = KVParser(KVDecoders({"ufid": decode_default})) >> + ufid_parser.parse(ufid_string) >> + if len(ufid_parser.kv()) != 1: >> + raise ValueError("malformed odp flow: %s" % odp_string) >> + sections.append( >> + Section("ufid", ufid_pos, ufid_string, ufid_parser.kv()) >> + ) >> + >> + action_pos = odp_string.find("actions:") >> + if action_pos < 0: >> + raise ValueError("malformed odp flow: %s" % odp_string) >> + >> + # rest of the string is between ufid and actions >> + rest = odp_string[ >> + (ufid_pos + len(ufid_string) if ufid_pos >= 0 else 0) : action_pos >> + ] >> + >> + action_pos += 8 # len("actions:") >> + actions = odp_string[action_pos:] >> + >> + field_parts = rest.lstrip(" ").partition(" ") >> + >> + if len(field_parts) != 3: >> + raise ValueError("malformed odp flow: %s" % odp_string) >> + >> + match = field_parts[0] >> + info = field_parts[2] >> + >> + iparser = KVParser(KVDecoders(self.info_decoders)) >> + iparser.parse(info) > > Looking at the two lines above, to me, it looks like the object might not have been defined right (but I’m not an OOO expert). > I would have designed it as, KVParser(string, decoders=None), this way you would have a single object initialization, so: > > parser = KVParser(info, KVDecoders(self.info_decoders)) > > Also because the parse() method does not allow re-use of the object, meaning parse another string with the same instance. > Or was there a specific reason to split this in two stages? > It's a remainder of a previous design. Right now I agree it doesn't make sense. I'll change it in the next version. >> + isection = Section( >> + name="info", >> + pos=odp_string.find(info), >> + string=info, >> + data=iparser.kv(), >> + ) >> + sections.append(isection) >> + >> + mparser = KVParser(KVDecoders(self.match_decoders)) >> + mparser.parse(match) >> + msection = Section( >> + name="match", >> + pos=odp_string.find(match), >> + string=match, >> + data=mparser.kv(), >> + ) >> + sections.append(msection) >> + >> + aparser = KVParser( >> + KVDecoders(self.action_decoders, default_free=decode_free_output) >> + ) >> + aparser.parse(actions) >> + asection = Section( >> + name="actions", >> + pos=action_pos, >> + string=actions, >> + data=aparser.kv(), >> + is_list=True, >> + ) >> + sections.append(asection) >> + >> + return ODPFlow(sections, odp_string, id) >> + >> + @classmethod > > Any reason why all of these are @classmethod, and not @staticmethod, as they do not seem to need a reference to self? > Guess this is also true in some of the previous patches, but I did not notice ;) Right. [...] >> + >> + @classmethod >> + def _field_decoders(cls): >> + return { >> + "skb_priority": Mask32, >> + "skb_mark": Mask32, >> + "recirc_id": decode_int, >> + "dp_hash": Mask32, >> + "ct_state": decode_default, # TODO: Parse flags > > Guess we might want to fix this before the merge? > Well, I've left it initially to confirm whether there is a strong need for this. Decoding ct_state with a integer allows you to set any ct_state, only you have to do it using the hexadecimal representation. Do you think we'll want to use string representation of flags? Same goes for other flags like tcp_flags. [...] >> +def decode_geneve(mask, value): >> + """ >> + Decode geneve options. Used for both tnl_push(header(geneve(options()))) >> + action and tunnel(geneve()) match. >> + >> + It has the following format: >> + >> + {class=0xffff,type=0x80,len=4,0xa} >> + >> + Args: >> + mask (bool): Whether masking is supported >> + value (str): The value to decode >> + """ >> + if mask: > > Create some consistency around the ending of doctext and the first line of code (in all patches). > Sometimes you have an empty line, like in decode_tnl_gre below, and sometimes not like here. > For some reason I was expecting my formatter to do that for me, but I guess it's not... Sure, I'll double check.
On 13 Jan 2022, at 12:33, Adrian Moreno wrote: > On 12/23/21 14:08, Eelco Chaudron wrote: >> Just some small comments below, and the request to fix up the comments. >> > > Thanks. > >> >> >> Trying to understand why you have a special ODPFlowFactory class to return an ODPFLow() object from a string? >> Can you not just add a static method to the ODPFlow class, so it would simply become a matter of calling: >> >> ODPFlow.from_string(string, id=id_data) >> >> Or probably more OOO is to override the __init__ function of ODPFlow() to take only the string and the id. >> Don’t think there is a use case to call ODPFlow() with sections and raw. >> > > I'll implement decoders caching via static class variables in the next version. > >>> +class ODPFlowFactory: >>> + """Datapath Flow""" >>> + >>> + def __init__(self): >>> + self.info_decoders = self._info_decoders() >>> + self.match_decoders = self._match_decoders() >>> + self.action_decoders = self._action_decoders() >>> + >>> + def from_string(self, odp_string, id=None): >>> + """Parse a odp flow string >>> + >>> + The string is expected to have the follwoing format: >>> + [ufid], [match] [flow data] actions:[actions] >>> + >>> + Args: >>> + odp_string (str): a datapath flow string >>> + >>> + Returns: >>> + an ODPFlow instance >>> + """ >>> + >>> + sections = [] >>> + >>> + # If UFID present, parse it and >>> + ufid_pos = odp_string.find("ufid:") >>> + if ufid_pos >= 0: >>> + ufid_string = odp_string[ >>> + ufid_pos : (odp_string[ufid_pos:].find(",") + 1) >>> + ] >>> + ufid_parser = KVParser(KVDecoders({"ufid": decode_default})) >>> + ufid_parser.parse(ufid_string) >>> + if len(ufid_parser.kv()) != 1: >>> + raise ValueError("malformed odp flow: %s" % odp_string) >>> + sections.append( >>> + Section("ufid", ufid_pos, ufid_string, ufid_parser.kv()) >>> + ) >>> + >>> + action_pos = odp_string.find("actions:") >>> + if action_pos < 0: >>> + raise ValueError("malformed odp flow: %s" % odp_string) >>> + >>> + # rest of the string is between ufid and actions >>> + rest = odp_string[ >>> + (ufid_pos + len(ufid_string) if ufid_pos >= 0 else 0) : action_pos >>> + ] >>> + >>> + action_pos += 8 # len("actions:") >>> + actions = odp_string[action_pos:] >>> + >>> + field_parts = rest.lstrip(" ").partition(" ") >>> + >>> + if len(field_parts) != 3: >>> + raise ValueError("malformed odp flow: %s" % odp_string) >>> + >>> + match = field_parts[0] >>> + info = field_parts[2] >>> + >>> + iparser = KVParser(KVDecoders(self.info_decoders)) >>> + iparser.parse(info) >> >> Looking at the two lines above, to me, it looks like the object might not have been defined right (but I’m not an OOO expert). >> I would have designed it as, KVParser(string, decoders=None), this way you would have a single object initialization, so: >> >> parser = KVParser(info, KVDecoders(self.info_decoders)) >> >> Also because the parse() method does not allow re-use of the object, meaning parse another string with the same instance. >> Or was there a specific reason to split this in two stages? >> > > It's a remainder of a previous design. Right now I agree it doesn't make sense. I'll change it in the next version. > >>> + isection = Section( >>> + name="info", >>> + pos=odp_string.find(info), >>> + string=info, >>> + data=iparser.kv(), >>> + ) >>> + sections.append(isection) >>> + >>> + mparser = KVParser(KVDecoders(self.match_decoders)) >>> + mparser.parse(match) >>> + msection = Section( >>> + name="match", >>> + pos=odp_string.find(match), >>> + string=match, >>> + data=mparser.kv(), >>> + ) >>> + sections.append(msection) >>> + >>> + aparser = KVParser( >>> + KVDecoders(self.action_decoders, default_free=decode_free_output) >>> + ) >>> + aparser.parse(actions) >>> + asection = Section( >>> + name="actions", >>> + pos=action_pos, >>> + string=actions, >>> + data=aparser.kv(), >>> + is_list=True, >>> + ) >>> + sections.append(asection) >>> + >>> + return ODPFlow(sections, odp_string, id) >>> + >>> + @classmethod >> >> Any reason why all of these are @classmethod, and not @staticmethod, as they do not seem to need a reference to self? >> Guess this is also true in some of the previous patches, but I did not notice ;) > > Right. > > [...] >>> + >>> + @classmethod >>> + def _field_decoders(cls): >>> + return { >>> + "skb_priority": Mask32, >>> + "skb_mark": Mask32, >>> + "recirc_id": decode_int, >>> + "dp_hash": Mask32, >>> + "ct_state": decode_default, # TODO: Parse flags >> >> Guess we might want to fix this before the merge? >> > > Well, I've left it initially to confirm whether there is a strong need for this. Decoding ct_state with a integer allows you to set any ct_state, only you have to do it using the hexadecimal representation. > > Do you think we'll want to use string representation of flags? Same goes for other flags like tcp_flags. Good question! Here is a dump with -m (ovs-appctl dpctl/dump-flows -m): ufid:97fc770e-2233-4b72-bfbc-7dbf3870ddf5, recirc_id(0x1),dp_hash(0/0),skb_priority(0/0),in_port(ovsp0),skb_mark(0/0),ct_state(0x1/0x1),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/00:00:00:00:00:00),eth_type(0x0800,ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0), packets:0, bytes:0, used:never, dp:ovs, actions:ct(commit,zone=1,mark=0x5/0x5,label=0x5/0x5),ct(commit,zone=2),ovs-p1 And one without: recirc_id(0x1),in_port(2),ct_state(+new),eth(),eth_type(0x0800),ipv4(proto=6,frag=no), packets:0, bytes:0, used:never, actions:ct(commit,zone=1,mark=0x5/0x5,label=0x5/0x5),ct(commit,zone=2),3 So which one do we need!? Guess we need to be able to decode both, depending on the input? Ping me offline if we need a quick brainstorming session. > [...] >>> +def decode_geneve(mask, value): >>> + """ >>> + Decode geneve options. Used for both tnl_push(header(geneve(options()))) >>> + action and tunnel(geneve()) match. >>> + >>> + It has the following format: >>> + >>> + {class=0xffff,type=0x80,len=4,0xa} >>> + >>> + Args: >>> + mask (bool): Whether masking is supported >>> + value (str): The value to decode >>> + """ >>> + if mask: >> >> Create some consistency around the ending of doctext and the first line of code (in all patches). >> Sometimes you have an empty line, like in decode_tnl_gre below, and sometimes not like here. >> > > For some reason I was expecting my formatter to do that for me, but I guess it's not... Sure, I'll double check. > > -- > Adrián Moreno
diff --git a/python/automake.mk b/python/automake.mk index d1464d7f6..8b0713cfc 100644 --- a/python/automake.mk +++ b/python/automake.mk @@ -48,7 +48,8 @@ ovs_pyfiles = \ python/ovs/flows/list.py \ python/ovs/flows/flow.py \ python/ovs/flows/ofp.py \ - python/ovs/flows/ofp_act.py + python/ovs/flows/ofp_act.py \ + python/ovs/flows/odp.py # These python files are used at build time but not runtime, # so they are not installed. diff --git a/python/ovs/flows/odp.py b/python/ovs/flows/odp.py new file mode 100644 index 000000000..8fe721f86 --- /dev/null +++ b/python/ovs/flows/odp.py @@ -0,0 +1,723 @@ +""" Defines an Openvswitch Datapath Flow +""" +import re +from functools import partial + +from ovs.flows.flow import Flow, Section + +from ovs.flows.kv import ( + KVParser, + KVDecoders, + nested_kv_decoder, + decode_nested_kv, +) +from ovs.flows.decoders import ( + decode_default, + decode_time, + decode_int, + decode_mask, + Mask8, + Mask16, + Mask32, + Mask64, + Mask128, + IPMask, + EthMask, + decode_free_output, + decode_flag, + decode_nat, +) + + +class ODPFlow(Flow): + def __init__(self, sections, raw="", id_=None): + """Constructor""" + super(ODPFlow, self).__init__(sections, raw, id_) + + def __str__(self): + if self._orig: + return self._orig + else: + return self.to_string() + + def to_string(self): + """Print a text representation of the flow""" + string = "Info: {}\n" + self.info + string += "Match : {}\n" + self.match + string += "Actions: {}\n " + self.actions + return string + + +class ODPFlowFactory: + """Datapath Flow""" + + def __init__(self): + self.info_decoders = self._info_decoders() + self.match_decoders = self._match_decoders() + self.action_decoders = self._action_decoders() + + def from_string(self, odp_string, id=None): + """Parse a odp flow string + + The string is expected to have the follwoing format: + [ufid], [match] [flow data] actions:[actions] + + Args: + odp_string (str): a datapath flow string + + Returns: + an ODPFlow instance + """ + + sections = [] + + # If UFID present, parse it and + ufid_pos = odp_string.find("ufid:") + if ufid_pos >= 0: + ufid_string = odp_string[ + ufid_pos : (odp_string[ufid_pos:].find(",") + 1) + ] + ufid_parser = KVParser(KVDecoders({"ufid": decode_default})) + ufid_parser.parse(ufid_string) + if len(ufid_parser.kv()) != 1: + raise ValueError("malformed odp flow: %s" % odp_string) + sections.append( + Section("ufid", ufid_pos, ufid_string, ufid_parser.kv()) + ) + + action_pos = odp_string.find("actions:") + if action_pos < 0: + raise ValueError("malformed odp flow: %s" % odp_string) + + # rest of the string is between ufid and actions + rest = odp_string[ + (ufid_pos + len(ufid_string) if ufid_pos >= 0 else 0) : action_pos + ] + + action_pos += 8 # len("actions:") + actions = odp_string[action_pos:] + + field_parts = rest.lstrip(" ").partition(" ") + + if len(field_parts) != 3: + raise ValueError("malformed odp flow: %s" % odp_string) + + match = field_parts[0] + info = field_parts[2] + + iparser = KVParser(KVDecoders(self.info_decoders)) + iparser.parse(info) + isection = Section( + name="info", + pos=odp_string.find(info), + string=info, + data=iparser.kv(), + ) + sections.append(isection) + + mparser = KVParser(KVDecoders(self.match_decoders)) + mparser.parse(match) + msection = Section( + name="match", + pos=odp_string.find(match), + string=match, + data=mparser.kv(), + ) + sections.append(msection) + + aparser = KVParser( + KVDecoders(self.action_decoders, default_free=decode_free_output) + ) + aparser.parse(actions) + asection = Section( + name="actions", + pos=action_pos, + string=actions, + data=aparser.kv(), + is_list=True, + ) + sections.append(asection) + + return ODPFlow(sections, odp_string, id) + + @classmethod + def _action_decoders(cls): + _decoders = { + "drop": decode_flag, + "lb_output": decode_int, + "trunc": decode_int, + "recirc": decode_int, + "userspace": nested_kv_decoder( + KVDecoders( + { + "pid": decode_int, + "sFlow": nested_kv_decoder( + KVDecoders( + { + "vid": decode_int, + "pcp": decode_int, + "output": decode_int, + } + ) + ), + "slow_path": decode_default, + "flow_sample": nested_kv_decoder( + KVDecoders( + { + "probability": decode_int, + "collector_sed_id": decode_int, + "obs_domain_id": decode_int, + "obs_point_id": decode_int, + "output_port": decode_default, + "ingress": decode_flag, + "egress": decode_flag, + } + ) + ), + "ipfix": nested_kv_decoder( + KVDecoders( + { + "output_port": decode_default, + } + ) + ), + "controller": nested_kv_decoder( + KVDecoders( + { + "reason": decode_int, + "dont_send": decode_int, + "continuation": decode_int, + "recirc_id": decode_int, + "rule_cookie": decode_int, + "controller_id": decode_int, + "max_len": decode_int, + } + ) + ), + "userdata": decode_default, + "actions": decode_flag, + "tunnel_out_port": decode_int, + "push_eth": nested_kv_decoder( + KVDecoders( + { + "src": EthMask, + "dst": EthMask, + "type": decode_int, + } + ) + ), + "pop_eth": decode_flag, + } + ) + ), + "set": nested_kv_decoder(KVDecoders(cls._field_decoders())), + "push_vlan": nested_kv_decoder( + KVDecoders( + { + "vid": decode_int, + "pcp": decode_int, + "cfi": decode_int, + "tpid": decode_int, + } + ) + ), + "pop_vlan": decode_flag, + "push_nsh": nested_kv_decoder( + KVDecoders( + { + "flags": decode_int, + "ttl": decode_int, + "mdtype": decode_int, + "np": decode_int, + "spi": decode_int, + "si": decode_int, + "c1": decode_int, + "c2": decode_int, + "c3": decode_int, + "c4": decode_int, + "md2": decode_int, + } + ) + ), + "pop_nsh": decode_flag, + "tnl_pop": decode_int, + "ct_clear": decode_flag, + "ct": nested_kv_decoder( + KVDecoders( + { + "commit": decode_flag, + "force_commit": decode_flag, + "zone": decode_int, + "mark": Mask32, + "label": Mask128, + "helper": decode_default, + "timeout": decode_default, + "nat": decode_nat, + } + ) + ), + **cls._tnl_action_decoder(), + } + + _decoders["clone"] = nested_kv_decoder( + KVDecoders(decoders=_decoders, default_free=decode_free_output) + ) + + return { + **_decoders, + "sample": nested_kv_decoder( + KVDecoders( + { + "sample": (lambda x: float(x.strip("%"))), + "actions": nested_kv_decoder( + KVDecoders( + decoders=_decoders, + default_free=decode_free_output, + ) + ), + } + ) + ), + "check_pkt_len": nested_kv_decoder( + KVDecoders( + { + "size": decode_int, + "gt": nested_kv_decoder( + KVDecoders( + decoders=_decoders, + default_free=decode_free_output, + ) + ), + "le": nested_kv_decoder( + KVDecoders( + decoders=_decoders, + default_free=decode_free_output, + ) + ), + } + ) + ), + } + + @classmethod + def _tnl_action_decoder(cls): + return { + "tnl_push": nested_kv_decoder( + KVDecoders( + { + "tnl_port": decode_int, + "header": nested_kv_decoder( + KVDecoders( + { + "size": decode_int, + "type": decode_int, + "eth": nested_kv_decoder( + KVDecoders( + { + "src": EthMask, + "dst": EthMask, + "dl_type": decode_int, + } + ) + ), + "ipv4": nested_kv_decoder( + KVDecoders( + { + "src": IPMask, + "dst": IPMask, + "proto": decode_int, + "tos": decode_int, + "ttl": decode_int, + "frag": decode_int, + } + ) + ), + "ipv6": nested_kv_decoder( + KVDecoders( + { + "src": IPMask, + "dst": IPMask, + "label": decode_int, + "proto": decode_int, + "tclass": decode_int, + "hlimit": decode_int, + } + ) + ), + "udp": nested_kv_decoder( + KVDecoders( + { + "src": decode_int, + "dst": decode_int, + "dsum": Mask16, + } + ) + ), + "vxlan": nested_kv_decoder( + KVDecoders( + { + "flags": decode_int, + "vni": decode_int, + } + ) + ), + "geneve": nested_kv_decoder( + KVDecoders( + { + "oam": decode_flag, + "crit": decode_flag, + "vni": decode_int, + "options": partial( + decode_geneve, False + ), + } + ) + ), + "gre": decode_tnl_gre, + "erspan": nested_kv_decoder( + KVDecoders( + { + "ver": decode_int, + "sid": decode_int, + "idx": decode_int, + "sid": decode_int, + "dir": decode_int, + "hwid": decode_int, + } + ) + ), + "gtpu": nested_kv_decoder( + KVDecoders( + { + "flags": decode_int, + "msgtype": decode_int, + "teid": decode_int, + } + ) + ), + } + ) + ), + "out_port": decode_int, + } + ) + ) + } + + @classmethod + def _info_decoders(cls): + return { + "packets": decode_int, + "bytes": decode_int, + "used": decode_time, + "flags": decode_default, + "dp": decode_default, + } + + @classmethod + def _match_decoders(cls): + return { + **cls._field_decoders(), + "encap": nested_kv_decoder(KVDecoders(cls._field_decoders())), + } + + @classmethod + def _field_decoders(cls): + return { + "skb_priority": Mask32, + "skb_mark": Mask32, + "recirc_id": decode_int, + "dp_hash": Mask32, + "ct_state": decode_default, # TODO: Parse flags + "ct_zone": Mask16, + "ct_mark": Mask32, + "ct_label": Mask128, + "ct_tuple4": nested_kv_decoder( + KVDecoders( + { + "src": IPMask, + "dst": IPMask, + "proto": Mask8, + "tcp_src": Mask16, + "tcp_dst": Mask16, + } + ) + ), + "ct_tuple6": nested_kv_decoder( + KVDecoders( + { + "src": IPMask, + "dst": IPMask, + "proto": Mask8, + "tcp_src": Mask16, + "tcp_dst": Mask16, + } + ) + ), + "tunnel": nested_kv_decoder( + KVDecoders( + { + "tun_id": Mask64, + "src": IPMask, + "dst": IPMask, + "ipv6_src": IPMask, + "ipv6_dst": IPMask, + "tos": Mask8, + "ttl": Mask8, + "tp_src": Mask16, + "tp_dst": Mask16, + "erspan": nested_kv_decoder( + KVDecoders( + { + "ver": Mask8, + "idx": Mask32, + "sid": decode_int, + "dir": Mask8, + "hwid": Mask8, + } + ) + ), + "vxlan": nested_kv_decoder( + KVDecoders( + { + "gbp": nested_kv_decoder( + KVDecoders( + { + "id": Mask16, + "flags": Mask8, + } + ) + ) + } + ) + ), + "geneve": partial(decode_geneve, True), + "gtpu": nested_kv_decoder( + KVDecoders( + { + "flags": Mask8, + "msgtype": Mask8, + } + ) + ), + "flags": decode_default, + } + ) + ), + "in_port": decode_default, + "eth": nested_kv_decoder( + KVDecoders( + { + "src": EthMask, + "dst": EthMask, + } + ) + ), + "vlan": nested_kv_decoder( + KVDecoders( + { + "vid": Mask16, + "pcp": Mask16, + "cfi": Mask16, + } + ) + ), + "eth_type": Mask16, + "mpls": nested_kv_decoder( + KVDecoders( + { + "label": Mask32, + "tc": Mask32, + "ttl": Mask32, + "bos": Mask32, + } + ) + ), + "ipv4": nested_kv_decoder( + KVDecoders( + { + "src": IPMask, + "dst": IPMask, + "proto": Mask8, + "tos": Mask8, + "ttl": Mask8, + "frag": decode_default, + } + ) + ), + "ipv6": nested_kv_decoder( + KVDecoders( + { + "src": IPMask, + "dst": IPMask, + "label": decode_mask(20), + "proto": Mask8, + "tclass": Mask8, + "hlimit": Mask8, + "frag": decode_default, + } + ) + ), + "tcp": nested_kv_decoder( + KVDecoders( + { + "src": Mask16, + "dst": Mask16, + } + ) + ), + "tcp_flags": decode_default, + "udp": nested_kv_decoder( + KVDecoders( + { + "src": Mask16, + "dst": Mask16, + } + ) + ), + "sctp": nested_kv_decoder( + KVDecoders( + { + "src": Mask16, + "dst": Mask16, + } + ) + ), + "icmp": nested_kv_decoder( + KVDecoders( + { + "type": Mask8, + "code": Mask8, + } + ) + ), + "icmpv6": nested_kv_decoder( + KVDecoders( + { + "type": Mask8, + "code": Mask8, + } + ) + ), + "arp": nested_kv_decoder( + KVDecoders( + { + "sip": IPMask, + "tip": IPMask, + "op": Mask16, + "sha": EthMask, + "tha": EthMask, + } + ) + ), + "nd": nested_kv_decoder( + KVDecoders( + { + "target": IPMask, + "sll": EthMask, + "tll": EthMask, + } + ) + ), + "nd_ext": nested_kv_decoder( + KVDecoders( + { + "nd_reserved": Mask32, + "nd_options_type": Mask8, + } + ) + ), + "packet_type": nested_kv_decoder( + KVDecoders( + { + "ns": Mask16, + "id": Mask16, + } + ) + ), + "nsh": nested_kv_decoder( + KVDecoders( + { + "flags": Mask8, + "mdtype": Mask8, + "np": Mask8, + "spi": Mask32, + "si": Mask8, + "c1": Mask32, + "c2": Mask32, + "c3": Mask32, + "c4": Mask32, + } + ) + ), + } + + +def decode_geneve(mask, value): + """ + Decode geneve options. Used for both tnl_push(header(geneve(options()))) + action and tunnel(geneve()) match. + + It has the following format: + + {class=0xffff,type=0x80,len=4,0xa} + + Args: + mask (bool): Whether masking is supported + value (str): The value to decode + """ + if mask: + decoders = { + "class": Mask16, + "type": Mask8, + "len": Mask8, + } + + def free_decoder(value): + return "data", Mask128(value) + + else: + decoders = { + "class": decode_int, + "type": decode_int, + "len": decode_int, + } + + def free_decoder(value): + return "data", decode_int(value) + + result = [] + for opts in re.findall(r"{.*?}", value): + result.append( + decode_nested_kv( + KVDecoders(decoders=decoders, default_free=free_decoder), + opts.strip("{}"), + ) + ) + return result + + +def decode_tnl_gre(value): + """ + Decode tnl_push(header(gre())) action + + It has the following format: + + gre((flags=0x2000,proto=0x6558),key=0x1e241)) + + Args: + value (str): The value to decode + """ + + return decode_nested_kv( + KVDecoders( + { + "flags": decode_int, + "proto": decode_int, + "key": decode_int, + "csum": decode_int, + "seq": decode_int, + } + ), + value.replace("(", "").replace(")", ""), + )
A ODPFlow is a Flow with the following sections: ufid info (e.g: bytes, packets, dp, etc) match actions Use a factory class ODPFlowFactory to cache the decoder objects. Only three datapath actions require special handling: gre: because it has double parenthesys geneve: because it supports many concatenated lists of options nat: we reuse the decoder used for openflow actions Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- python/automake.mk | 3 +- python/ovs/flows/odp.py | 723 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 725 insertions(+), 1 deletion(-) create mode 100644 python/ovs/flows/odp.py