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

[Jason Wang <jasowang@redhat.com>]

在 2021/6/22 下午3:38, chen.zhang@intel.com 写道:
>
> On 6/22/21 3:04 PM, Jason Wang wrote:
>> 在 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.
>
>
> OK.
>
> The @object-name means a qemu object with network packet processing 
> function, for example colo-compare, filtr-mirror, etc.
>
> I will add it in the next version.


So if two net-filters want to use exact the same set of flowspec, we 
need to specify them twice?


>
>
>>
>>
>>>>>> 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.
>>
>
> OK. Change to:
>
> VM can running with multi network packet processing function objects.
>
> They can control different network data paths from netdev or chardev.
>
> So it needs the object-name to set the effective module.
>
>
>>>>>> 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?
>
> This series has not added pass-through support for other netfilters,
>
> Can we change the qmp command to passthrough-filter-add/del as 
> markus's comments in this series,


It looks more general, so I think it should work.

Thanks


>
> And enable filters pass-through capability and add properties in next 
> series. Step by step.
>
>
> Thanks
>
> Chen
>
>
>>
>> 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' }
>>
>