Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough

[Jason Wang <jasowang@redhat.com>]

在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>
> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>
>>>> Since the real user scenario does not need COLO to monitor all 
>>>> traffic.
>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP 
>>>> commands.
>>>> All the fields of IPFlowSpec are optional.
>>>>
>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>> ---
>>> The QAPI schema looks good to me, but the interface documentation is
>>> still not quite clear enough.  To make progress, I'm going to make
>>> concrete suggestions wherever I can despite being quite clueless about
>>> the subject matter.  Risks me writing something that's clearer, but
>>> wrong.  Keep that in mind, please.
>>>
>>>>   net/net.c     | 10 +++++++
>>>>   qapi/net.json | 74 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 84 insertions(+)
>>>>
>>>> diff --git a/net/net.c b/net/net.c
>>>> index 76bbb7c31b..f913e97983 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error 
>>>> **errp)
>>>>       }
>>>>   }
>>>>   +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>> +{
>>>> +    /* TODO implement setup passthrough rule */
>>>> +}
>>>> +
>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>> +{
>>>> +    /* TODO implement delete passthrough rule */
>>>> +}
>>>> +
>>>>   static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>>>>   {
>>>>       char *str;
>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>> --- a/qapi/net.json
>>>> +++ b/qapi/net.json
>>>> @@ -7,6 +7,7 @@
>>>>   ##
>>>>     { 'include': 'common.json' }
>>>> +{ 'include': 'sockets.json' }
>>>>     ##
>>>>   # @set_link:
>>>> @@ -696,3 +697,76 @@
>>>>   ##
>>>>   { 'event': 'FAILOVER_NEGOTIATED',
>>>>     'data': {'device-id': 'str'} }
>>>> +
>>>> +##
>>>> +# @IPFlowSpec:
>>>> +#
>>>> +# IP flow specification.
>>>> +#
>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The 
>>>> protocol is the
>>>> +#            string instead of enum, because it can be passed to 
>>>> getprotobyname(3)
>>>> +#            and avoid duplication with /etc/protocols.
>>> The rationale is good, but it doesn't really belong into the interface
>>> documentation.  Suggest:
>>>
>>>     # @protocol: Transport layer protocol like TCP/UDP, etc. This 
>>> will be
>>>     #            passed to getprotobyname(3).
>>>
>>>
>>>> +#
>>>> +# @object-name: The @object-name means packet handler in Qemu.


I think we need clarify "packet handler" here. It does not look like a 
Qemu terminology.


>>>> Because not
>>>> +#               all the network packet must pass the colo-compare 
>>>> module,
>>>> +#               the net-filters are same situation. There modules 
>>>> attach to
>>>> +#               netdev or chardev to work, VM can run multiple 
>>>> modules
>>>> +#               at the same time. 


This sentence needs some tweak since I fail to understand it's meaning.


>>>> So it needs the object-name to set
>>>> +#               the effective module.
>>> I still don't understand this, and I'm too ignorant of COLO and
>>> networking to suggest improvements.
>>>
>>> Jason or David, perhaps?
>> I'll leave Jason to check on the object behaviour (and I see the rest of
>> the thread); but at a high level, COLO is deciding whether to do VM
>> synchronisation on whether the network behaviour of the two VMs get out
>> of sync (e.g. due to randomness in the flow of requests); if you don't
>> sync then when you fail-over, you'll get TCP errors/inconsistencies in
>> the stream view from the secondary; but this patch series is saying
>> you don't care if some TCP connections fail like that, you only care
>> about maybe the main sockets the application is providing.
>
> Yes, you are right.


I wonder if it's the best to introduce colo specific command here. 
Instead, can we introduce commands to set and get netfilter properties?

Thanks


>
>
> Thanks
>
> Chen
>
>
>> Dave
>>
>>>> +#
>>>> +# @source: Source address and port.
>>>> +#
>>>> +# @destination: Destination address and port.
>>>> +#
>>>> +# Since: 6.1
>>>> +##
>>>> +{ 'struct': 'IPFlowSpec',
>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>> +    '*source': 'InetSocketAddressBase',
>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>> +
>>>> +##
>>>> +# @colo-passthrough-add:
>>>> +#
>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>> +# The protocol and source/destination IP/ports are optional. if 
>>>> the user
>>>> +# only inputs part of the information, this will match all traffic.
>>> Actually, all arguments are optional.
>>>
>>> Suggest:
>>>
>>>     # Add an entry to the COLO network passthrough list.
>>>     # Absent protocol, host addresses and ports match anything.
>>>
>>> If there is more than one such list, then "to a COLO network 
>>> passthrough
>>> list" instead.
>>>
>>> Still missing then: meaning of absent @object-name.  Does it select the
>>> COLO network passthrough list, perhaps?
>>>
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since: 6.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "colo-passthrough-add",
>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +##
>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>> +     'data': 'IPFlowSpec' }
>>>> +
>>>> +##
>>>> +# @colo-passthrough-del:
>>>> +#
>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>> +# The protocol and source/destination IP/ports are optional. if 
>>>> the user
>>>> +# only inputs part of the information, this will match all traffic.
>>> I suspect this command doesn't actually match traffic, it matches
>>> entries added with colo-passthrough-add.
>>>
>>> Can it delete more than one such entry?
>>>
>>> Suggest:
>>>
>>>     # Delete an entry from the COLO network passthrough list.
>>>
>>> and then explain how the command arguments select entries.
>>>
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since: 6.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "colo-passthrough-del",
>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +##
>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>> +     'data': 'IPFlowSpec' }
>