[RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

This patch series adds/(updates) the following functionality
in the synchronization between master and backup daemons.

 - Ipv6
 - Persistence Engine
 - Firewall marks transfered
 - Timeouts transfered.
 - Flag field increased to 32 bits.

A new message format is also introduced, not understood by old backup daemons.
For compatibility reasons receiving the old version (version 0) is still possible.
Old (version 0) backups will just drop new (Version 1) messages.

Update scenario:
 Update the Machine with Bakup daemon first.

Message structure:
A new 32 bit word is added to the header,
where the old count_Conns is set to 0 so old Backup daemons just will drop the packet.
Added is version (1) and there is also a spare 16 bit field at the end.
This is a reworked version based upon Simon Hormans work.

 Version 1:
       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |      0        |    Syncid     |            Size               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |  Count Conns  |   Version     | Spare set to Zero             |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                                               |
      |                    IPVS Sync Connection (1)                   |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                            .                                  |
      ~                            .                                  ~
      |                            .                                  |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                                               |
      |                    IPVS Sync Connection (n)                   |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

 Version 0 Header
       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |  Count Conns  |    SyncID     |            Size               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                    IPVS Sync Connection (1)                   |


The Sync. Conn. message format is also changed to allow future enhancements
and versioning.

     Connection Message format

       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |    Type       |    Protocol   | Ver.  |        Size           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                         Flags                                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |            State              |         cport                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |            vport              |         dport                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             fwmark                            |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             timeout                           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                              ...                              |
      |                        IP-Addresses  (IPv4 or IPv6)           |
      |                              ...                              |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Options controlled be Flags
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             init_seq                          |
      |                in_seq       delta                             |
      | __________________________  prev_delta ______________________ |
      |                             init_seq                          |
      |               out_seq       delta                             |
      |                             prev_delta                        |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Optional dynamic length Persistence data, controled by type.
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | pe_len        | pe_name_len   |  pe_data                      |
      |                              ...                              |
      | pe_name ....                                                  |
      |            padded for 32 bit alignment                        |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

New fields that might need some explanation,

Type:  is one of this four
-IPv4
-IPv6
-IPv4 with PE data
-IPv6 with PE data

Ver.  Version of specified type right now it's 0

fwmark:  Firewall mark from skb.

timeout: from ip_vs_conn struct.

PATCH STATUS:
 - Persistence data is not tested.

QUESTIONS:
In ip_vs_nfct.c
ip_vs_conn_fill_param()  fwmark is set to 0,
Do we need to digg for it there ?
(It is a callback from ct ..)

THANKS
The Persistence part is based on Simon Hormans Backup RFC


SUMMARY
 include/net/ip_vs.h                     |    7 +-
 net/netfilter/ipvs/ip_vs_conn.c         |    9 +-
 net/netfilter/ipvs/ip_vs_core.c         |   19 +-
 net/netfilter/ipvs/ip_vs_ctl.c          |    4 +-
 net/netfilter/ipvs/ip_vs_ftp.c          |    8 +-
 net/netfilter/ipvs/ip_vs_nfct.c         |    2 +-
 net/netfilter/ipvs/ip_vs_proto_ah_esp.c |   13 +-
 net/netfilter/ipvs/ip_vs_sync.c         |  712 ++++++++++++++++++++++++-------
 8 files changed, 592 insertions(+), 182 deletions(-)

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Tue, 26 Oct 2010, Hans Schillstrom wrote:

> Type:  is one of this four
> -IPv4
> -IPv6
> -IPv4 with PE data
> -IPv6 with PE data

 	I'm thinking of different way to provide parameters. If before
now ip_vs_sync_conn_options uses cp->flags & IP_VS_CONN_F_SEQ_MASK
to check for present data now we can allow optional parameters
in such form:

- 1 octet length to next parameter: N
- 1 octet type: IPv4_Addresses, IPv6_Addresses, PE_Data, TCP_Seqs
- N-2 octets for data

 	The cost is 2 extra octets per parameter data and
that alignment is 2 octets, we must use memcpy for the
addresses.

 	By this way later we can add more parameters. If we
want master to specify that some parameter is optional
we can allocate 1 bit (bit 7) in 'type' for this. Most
of the parameters must be supported by backup server.
If backup does not recognize some type it will skip the
connection entry if bit 7 is not set.

 	While parsing the parameters we can set some bits
in local var to detect what params are received: only one of
IPv4_Addresses and IPv6_Addresses, etc.

 	FWMARK can be added in this way but may be it
is better to be in the mandatory structure.

 	The only problem is that it is difficult to
predict the entry size in ip_vs_sync_conn().

 	Also, we should add new define that will mask
all connection flags not supported by backup. Currently,
only IP_VS_CONN_F_HASHED is ignored:

#define IP_VS_CONN_F_BACKUP_MASK (IP_VS_CONN_F_FWD_MASK | \
 				IP_VS_CONN_F_NOOUTPUT | \
 				IP_VS_CONN_F_INACTIVE | \
 				IP_VS_CONN_F_SEQ_MASK | \
 				IP_VS_CONN_F_NO_CPORT | \
 				IP_VS_CONN_F_TEMPLATE | \
 				)

 	may be IP_VS_CONN_F_ONE_PACKET does not need to
be used in backup and master does not need to sync it.

> Ver.  Version of specified type right now it's 0
>
> fwmark:  Firewall mark from skb.

 	Is it really needed fwmark to be added in
struct ip_vs_conn_param? May be adding new argument just
to ip_vs_conn_new is enough?

> timeout: from ip_vs_conn struct.

 	May be timeout must be in seconds, in case both
boxes use different HZ.

> PATCH STATUS:
> - Persistence data is not tested.
>
> QUESTIONS:
> In ip_vs_nfct.c
> ip_vs_conn_fill_param()  fwmark is set to 0,
> Do we need to digg for it there ?
> (It is a callback from ct ..)

 	There is no ip_vs_conn_new in ip_vs_nfct.c, so it will
be simpler if fwmark is not added to struct ip_vs_conn_param

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

Hello

On Tuesday 26 October 2010 23:25:10 Julian Anastasov wrote:
>
>  	Hello,
>
> On Tue, 26 Oct 2010, Hans Schillstrom wrote:
>
> > Type:  is one of this four
> > -IPv4
> > -IPv6
> > -IPv4 with PE data
> > -IPv6 with PE data
>
>  	I'm thinking of different way to provide parameters. If before
> now ip_vs_sync_conn_options uses cp->flags & IP_VS_CONN_F_SEQ_MASK
> to check for present data now we can allow optional parameters
> in such form:
>
> - 1 octet length to next parameter: N
> - 1 octet type: IPv4_Addresses, IPv6_Addresses, PE_Data, TCP_Seqs
> - N-2 octets for data
>
>  	The cost is 2 extra octets per parameter data and
> that alignment is 2 octets, we must use memcpy for the
> addresses.
>
I have no problem with that,
So you mean something like this ?

       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |    Type       |    Protocol   | Ver.  |        Size           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                         Flags                                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |            State              |         cport                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |            vport              |         dport                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             fwmark                            |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             timeout                           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                              ...                              |
      |                        IP-Addresses  (v4 or v6)               |
      |                              ...                              |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      +- Optional: ip_vs_cync_conn_options -+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             init_seq                          |
      |                in_seq       delta                             |
      | __________________________  prev_delta ______________________ |
      |                             init_seq                          |
      |               out_seq       delta                             |
      |                             prev_delta                        |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Options header with alignment examples.
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | Option Type   | Option length |    Option data                |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                               +
      |                                                               |
      |               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |               | Opt padding   | Option Type   | Option length |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                 Opt data                                      |
      |                                                               |
      |               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | Last Opt data |  Last option Padded to 32 bit alignment       |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

/* Option types */
#define IPVS_OPT_SEQ_DATA    1
#define IPVS_OPT_PE_DATA     2
#define IPVS_OPT_PE_NAME     3


>  	By this way later we can add more parameters. If we
> want master to specify that some parameter is optional
> we can allocate 1 bit (bit 7) in 'type' for this. Most
> of the parameters must be supported by backup server.
> If backup does not recognize some type it will skip the
> connection entry if bit 7 is not set.
>
A New Spec of Type field:

Bit    7        6        . . .      2         1           0
  +----------+--------------------------+-------------+-------+
  | Opt.Data |          Spare           | Packed IPv6 |  IPv6 |
  +----------+--------------------------+-------------+-------+

>  	While parsing the parameters we can set some bits
> in local var to detect what params are received: only one of
> IPv4_Addresses and IPv6_Addresses, etc.
>
>  	FWMARK can be added in this way but may be it
> is better to be in the mandatory structure.
>

I think we should leave it in the struct as mandatory,
the code will be messy if everything is options :-)

>  	The only problem is that it is difficult to
> predict the entry size in ip_vs_sync_conn().
>
>  	Also, we should add new define that will mask
> all connection flags not supported by backup. Currently,
> only IP_VS_CONN_F_HASHED is ignored:
>
> #define IP_VS_CONN_F_BACKUP_MASK (IP_VS_CONN_F_FWD_MASK | \
>  				IP_VS_CONN_F_NOOUTPUT | \
>  				IP_VS_CONN_F_INACTIVE | \
>  				IP_VS_CONN_F_SEQ_MASK | \
>  				IP_VS_CONN_F_NO_CPORT | \
>  				IP_VS_CONN_F_TEMPLATE | \
>  				)
>
>  	may be IP_VS_CONN_F_ONE_PACKET does not need to
> be used in backup and master does not need to sync it.
>

I do agree

> > Ver.  Version of specified type right now it's 0
> >
> > fwmark:  Firewall mark from skb.
>
>  	Is it really needed fwmark to be added in
> struct ip_vs_conn_param? May be adding new argument just
> to ip_vs_conn_new is enough?
>

OK

> > timeout: from ip_vs_conn struct.
>
>  	May be timeout must be in seconds, in case both
> boxes use different HZ.

Yeah, you are right.

>
> > PATCH STATUS:
> > - Persistence data is not tested.
> >
> > QUESTIONS:
> > In ip_vs_nfct.c
> > ip_vs_conn_fill_param()  fwmark is set to 0,
> > Do we need to digg for it there ?
> > (It is a callback from ct ..)
>
>  	There is no ip_vs_conn_new in ip_vs_nfct.c, so it will
> be simpler if fwmark is not added to struct ip_vs_conn_param

Sure, I'll do that-

>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

Hello
Some small changes, ...

On Wednesday 27 October 2010 11:19:20 Hans Schillstrom wrote:
> Hello
>
> On Tuesday 26 October 2010 23:25:10 Julian Anastasov wrote:
> >
> >  	Hello,
[snip]
>
> /* Option types */
> #define IPVS_OPT_SEQ_DATA    1
> #define IPVS_OPT_PE_DATA     2
> #define IPVS_OPT_PE_NAME     3
>
>
> >  	By this way later we can add more parameters. If we
> > want master to specify that some parameter is optional
> > we can allocate 1 bit (bit 7) in 'type' for this. Most
> > of the parameters must be supported by backup server.
> > If backup does not recognize some type it will skip the
> > connection entry if bit 7 is not set.
> >
> A New Spec of Type field:
>
> Bit    7        6        . . .      2         1           0
>   +----------+--------------------------+-------------+-------+
>   | Opt.Data |          Spare           | Packed IPv6 |  IPv6 |
>   +----------+--------------------------+-------------+-------+

I can see a better usage of it in Option Type so Type will look like this
    +-------------------------------------+-------------+-------+
    |                     Spare           | Packed IPv6 |  IPv6 |
    +-------------------------------------+-------------+-------+

And "Option Type" in option field would look like this

 Bit    7        6        . . .    0    7                      0
   +----------+----------------------+---------------------------+
   | Optional |      Option type     |    Option length          |
   +----------+----------------------+---------------------------+

We can have a better fine tuning of options in this way.

I.e. as Julian wrote but transferd to Option Type:

" If backup does not recognize some type it will skip the
  connection entry if bit 7 is not set."

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Wed, 27 Oct 2010, Hans Schillstrom wrote:

>> A New Spec of Type field:
>>
>> Bit    7        6        . . .      2         1           0
>>   +----------+--------------------------+-------------+-------+
>>   | Opt.Data |          Spare           | Packed IPv6 |  IPv6 |
>>   +----------+--------------------------+-------------+-------+
>
> I can see a better usage of it in Option Type so Type will look like this
>    +-------------------------------------+-------------+-------+
>    |                     Spare           | Packed IPv6 |  IPv6 |
>    +-------------------------------------+-------------+-------+
>
> And "Option Type" in option field would look like this
>
> Bit    7        6        . . .    0    7                      0
>   +----------+----------------------+---------------------------+
>   | Optional |      Option type     |    Option length          |
>   +----------+----------------------+---------------------------+
>
> We can have a better fine tuning of options in this way.

 	Yes, that is exactly my idea. I more like the name
"Parameter" instead of "Option", i.e. we have additional
parameters that can be mandatory (usually) but also can be
optional. For now I don't have idea for any optional
parameters but allocating 1 bit for this does not look
fatal.

> I.e. as Julian wrote but transferd to Option Type:
>
> " If backup does not recognize some type it will skip the
>  connection entry if bit 7 is not set."
>
> --
> Regards
> Hans Schillstrom <hans.schillstrom@ericsson.com>

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Wed, Oct 27, 2010 at 10:58:31PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Wed, 27 Oct 2010, Hans Schillstrom wrote:
> 
> >>A New Spec of Type field:
> >>
> >>Bit    7        6        . . .      2         1           0
> >>  +----------+--------------------------+-------------+-------+
> >>  | Opt.Data |          Spare           | Packed IPv6 |  IPv6 |
> >>  +----------+--------------------------+-------------+-------+
> >
> >I can see a better usage of it in Option Type so Type will look like this
> >   +-------------------------------------+-------------+-------+
> >   |                     Spare           | Packed IPv6 |  IPv6 |
> >   +-------------------------------------+-------------+-------+
> >
> >And "Option Type" in option field would look like this
> >
> >Bit    7        6        . . .    0    7                      0
> >  +----------+----------------------+---------------------------+
> >  | Optional |      Option type     |    Option length          |
> >  +----------+----------------------+---------------------------+

As it stands a little more than 256 bytes may be needed for
pe_data (+ pe_name_length + pe_name). This could be resolved by
shortening the maximum pe_data length. Or perhaps we could use 16 bytes for
Option length, which should ensure its never too small.

The 256 byte limit that I made for pe_data was arbitrarily chosen.

> >We can have a better fine tuning of options in this way.
> 
> 	Yes, that is exactly my idea. I more like the name
> "Parameter" instead of "Option", i.e. we have additional
> parameters that can be mandatory (usually) but also can be
> optional. For now I don't have idea for any optional
> parameters but allocating 1 bit for this does not look
> fatal.

I'm not sure I understand the motivation for optional parameters.
I think its important to allow for backwards compatibility. But
I don't see that there will be multiple independent implementations
of the synchronisation daemon in the near future. So the use-case
isn't clear to me.

That said, I agree that allocating 1 bit isn't a show-stopper.

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

Hello

On Thursday 28 October 2010 13:35:12 Simon Horman wrote:
> On Wed, Oct 27, 2010 at 10:58:31PM +0300, Julian Anastasov wrote:
> >
> > 	Hello,
> >
> > On Wed, 27 Oct 2010, Hans Schillstrom wrote:
> >
> > >>A New Spec of Type field:
> > >>
> > >>Bit    7        6        . . .      2         1           0
> > >>  +----------+--------------------------+-------------+-------+
> > >>  | Opt.Data |          Spare           | Packed IPv6 |  IPv6 |
> > >>  +----------+--------------------------+-------------+-------+
> > >
> > >I can see a better usage of it in Option Type so Type will look like this
> > >   +-------------------------------------+-------------+-------+
> > >   |                     Spare           | Packed IPv6 |  IPv6 |
> > >   +-------------------------------------+-------------+-------+
> > >
> > >And "Option Type" in option field would look like this
> > >
> > >Bit    7        6        . . .    0    7                      0
> > >  +----------+----------------------+---------------------------+
> > >  | Optional |      Option type     |    Option length          |
> > >  +----------+----------------------+---------------------------+
>
> As it stands a little more than 256 bytes may be needed for
> pe_data (+ pe_name_length + pe_name). This could be resolved by
> shortening the maximum pe_data length. Or perhaps we could use 16 bytes for
> Option length, which should ensure its never too small.
>
> The 256 byte limit that I made for pe_data was arbitrarily chosen.
>

I have PE_NAME and PE_DATA ass different options
so the limit is actually 255 bytes.

#define IPVS_OPT_SEQ_DATA 	1
#define IPVS_OPT_PE_DATA	2
#define IPVS_OPT_PE_NAME	3

How ever they are not independant of each other.
 - PE_NAME never goes alone, only if there is PE_DATA.
 - In the receiving path, PE_NAME must have PE_DATA to be valid.

> > >We can have a better fine tuning of options in this way.
> >
> > 	Yes, that is exactly my idea. I more like the name
> > "Parameter" instead of "Option", i.e. we have additional
> > parameters that can be mandatory (usually) but also can be
> > optional. For now I don't have idea for any optional
> > parameters but allocating 1 bit for this does not look
> > fatal.
>
> I'm not sure I understand the motivation for optional parameters.
> I think its important to allow for backwards compatibility. But
> I don't see that there will be multiple independent implementations
> of the synchronisation daemon in the near future. So the use-case
> isn't clear to me.

Backward compatibility is "one way only"
a new backup daemon can listen to an old one
not the other way around.

>
> That said, I agree that allocating 1 bit isn't a show-stopper.
>
>

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Thu, Oct 28, 2010 at 01:52:15PM +0200, Hans Schillstrom wrote:
> Hello
> 
> On Thursday 28 October 2010 13:35:12 Simon Horman wrote:
> > On Wed, Oct 27, 2010 at 10:58:31PM +0300, Julian Anastasov wrote:
> > >
> > > 	Hello,
> > >
> > > On Wed, 27 Oct 2010, Hans Schillstrom wrote:
> > >
> > > >>A New Spec of Type field:
> > > >>
> > > >>Bit    7        6        . . .      2         1           0
> > > >>  +----------+--------------------------+-------------+-------+
> > > >>  | Opt.Data |          Spare           | Packed IPv6 |  IPv6 |
> > > >>  +----------+--------------------------+-------------+-------+
> > > >
> > > >I can see a better usage of it in Option Type so Type will look like this
> > > >   +-------------------------------------+-------------+-------+
> > > >   |                     Spare           | Packed IPv6 |  IPv6 |
> > > >   +-------------------------------------+-------------+-------+
> > > >
> > > >And "Option Type" in option field would look like this
> > > >
> > > >Bit    7        6        . . .    0    7                      0
> > > >  +----------+----------------------+---------------------------+
> > > >  | Optional |      Option type     |    Option length          |
> > > >  +----------+----------------------+---------------------------+
> >
> > As it stands a little more than 256 bytes may be needed for
> > pe_data (+ pe_name_length + pe_name). This could be resolved by
> > shortening the maximum pe_data length. Or perhaps we could use 16 bytes for
> > Option length, which should ensure its never too small.
> >
> > The 256 byte limit that I made for pe_data was arbitrarily chosen.
> >
> 
> I have PE_NAME and PE_DATA ass different options
> so the limit is actually 255 bytes.
> 
> #define IPVS_OPT_SEQ_DATA 	1
> #define IPVS_OPT_PE_DATA	2
> #define IPVS_OPT_PE_NAME	3
> 
> How ever they are not independant of each other.
>  - PE_NAME never goes alone, only if there is PE_DATA.
>  - In the receiving path, PE_NAME must have PE_DATA to be valid.

Ok, thanks, I haven't reviewed the patches that closely yet :-(

But still, 8 buts for option length does limit us to never ever
having an option longer than 255 bytes. But I guess we can
make sync protocol v3 if we need to cross that bridge.

> > > >We can have a better fine tuning of options in this way.
> > >
> > > 	Yes, that is exactly my idea. I more like the name
> > > "Parameter" instead of "Option", i.e. we have additional
> > > parameters that can be mandatory (usually) but also can be
> > > optional. For now I don't have idea for any optional
> > > parameters but allocating 1 bit for this does not look
> > > fatal.
> >
> > I'm not sure I understand the motivation for optional parameters.
> > I think its important to allow for backwards compatibility. But
> > I don't see that there will be multiple independent implementations
> > of the synchronisation daemon in the near future. So the use-case
> > isn't clear to me.
> 
> Backward compatibility is "one way only"
> a new backup daemon can listen to an old one
> not the other way around.

Yes. I thought that was my point :-)

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Thu, 28 Oct 2010, Simon Horman wrote:

> As it stands a little more than 256 bytes may be needed for
> pe_data (+ pe_name_length + pe_name). This could be resolved by
> shortening the maximum pe_data length. Or perhaps we could use 16 bytes for
> Option length, which should ensure its never too small.
>
> The 256 byte limit that I made for pe_data was arbitrarily chosen.

 	Yes, this is a problem.

>>> We can have a better fine tuning of options in this way.
>>
>> 	Yes, that is exactly my idea. I more like the name
>> "Parameter" instead of "Option", i.e. we have additional
>> parameters that can be mandatory (usually) but also can be
>> optional. For now I don't have idea for any optional
>> parameters but allocating 1 bit for this does not look
>> fatal.
>
> I'm not sure I understand the motivation for optional parameters.
> I think its important to allow for backwards compatibility. But
> I don't see that there will be multiple independent implementations
> of the synchronisation daemon in the near future. So the use-case
> isn't clear to me.

 	If we want to support optional parameters they
must have some known length field, 16 bits if needed.
But if we don't want such optional parameters then
we can just use single octet for parameter type
which can imply the length of the following data. 
Types that have data with variable length should provide
1 or 2 octets after the parameter type for their length.
So, the parsing should be per-type. If one day
we need optional parameters we can just add 1 octet for
type and 2 for length before data.

 	For example, ip_vs_sync_conn_options if implemented
as parameter can be coded with 1 octet for type and the
following data structure. OTOH, PE params can use 1 octet
for type, 2 for length (read with get_unaligned_be16) and
then data.

> That said, I agree that allocating 1 bit isn't a show-stopper.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

Hello

On Thursday, October 28, 2010 23:07:30 Julian Anastasov wrote:
> 
>  	Hello,
> 
> On Thu, 28 Oct 2010, Simon Horman wrote:
> 
> > As it stands a little more than 256 bytes may be needed for
> > pe_data (+ pe_name_length + pe_name). This could be resolved by
> > shortening the maximum pe_data length. Or perhaps we could use 16 bytes for
> > Option length, which should ensure its never too small.
> >
> > The 256 byte limit that I made for pe_data was arbitrarily chosen.
> 
>  	Yes, this is a problem.
> 
> >>> We can have a better fine tuning of options in this way.
> >>
> >> 	Yes, that is exactly my idea. I more like the name
> >> "Parameter" instead of "Option", i.e. we have additional
> >> parameters that can be mandatory (usually) but also can be
> >> optional. For now I don't have idea for any optional
> >> parameters but allocating 1 bit for this does not look
> >> fatal.
> >
> > I'm not sure I understand the motivation for optional parameters.
> > I think its important to allow for backwards compatibility. But
> > I don't see that there will be multiple independent implementations
> > of the synchronisation daemon in the near future. So the use-case
> > isn't clear to me.
> 
>  	If we want to support optional parameters they
> must have some known length field, 16 bits if needed.
> But if we don't want such optional parameters then
> we can just use single octet for parameter type
> which can imply the length of the following data. 
> Types that have data with variable length should provide
> 1 or 2 octets after the parameter type for their length.
> So, the parsing should be per-type. If one day
> we need optional parameters we can just add 1 octet for
> type and 2 for length before data.
> 
>  	For example, ip_vs_sync_conn_options if implemented
> as parameter can be coded with 1 octet for type and the
> following data structure. OTOH, PE params can use 1 octet
> for type, 2 for length (read with get_unaligned_be16) and
> then data.

OK, a minor change in length check only.
For todays known types one byte is ok, since pe data willbe splitted.

> 
> > That said, I agree that allocating 1 bit isn't a show-stopper.
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

Regars 
Hans Schillstrom <hans@schillstrom.com>

Re: [RFC PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Fri, Oct 29, 2010 at 12:07:30AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Thu, 28 Oct 2010, Simon Horman wrote:
> 
> >As it stands a little more than 256 bytes may be needed for
> >pe_data (+ pe_name_length + pe_name). This could be resolved by
> >shortening the maximum pe_data length. Or perhaps we could use 16 bytes for
> >Option length, which should ensure its never too small.
> >
> >The 256 byte limit that I made for pe_data was arbitrarily chosen.
> 
> 	Yes, this is a problem.
> 
> >>>We can have a better fine tuning of options in this way.
> >>
> >>	Yes, that is exactly my idea. I more like the name
> >>"Parameter" instead of "Option", i.e. we have additional
> >>parameters that can be mandatory (usually) but also can be
> >>optional. For now I don't have idea for any optional
> >>parameters but allocating 1 bit for this does not look
> >>fatal.
> >
> >I'm not sure I understand the motivation for optional parameters.
> >I think its important to allow for backwards compatibility. But
> >I don't see that there will be multiple independent implementations
> >of the synchronisation daemon in the near future. So the use-case
> >isn't clear to me.
> 
> 	If we want to support optional parameters they
> must have some known length field, 16 bits if needed.

At this point I see no strong reason to support optional parameters.

> But if we don't want such optional parameters then
> we can just use single octet for parameter type
> which can imply the length of the following data. Types that have
> data with variable length should provide
> 1 or 2 octets after the parameter type for their length.
>
> So, the parsing should be per-type. If one day
> we need optional parameters we can just add 1 octet for
> type and 2 for length before data.
> 
> 	For example, ip_vs_sync_conn_options if implemented
> as parameter can be coded with 1 octet for type and the
> following data structure. OTOH, PE params can use 1 octet
> for type, 2 for length (read with get_unaligned_be16) and
> then data.

Agreed.

With such a scheme it would be possible to have pe_data and pe_name
as a single type - but two types is also fine by me.