[*v4 PATCH 0/3] IPVS: Backup Adding Ipv6 and Persistence support

[*v4 PATCH 0/3] 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 transferred
 - Time-outs transferred.
 - Flag field increased to 32 bits.

Note:
It depends on Patch 1-9 in staging branch.
"IPVS: Backup, Prepare for transferring firewall marks (fwmark) to the backup daemon."
to ...
"IPVS: skb defrag in L7 helpers"

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.
It's also possible to send sync msg in version 0 format, by sysctl
#sysclt -w net.ipv4.vs.sync_version=0


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:
  Note, first byte should be Zero, so ver 0 receivers will drop the packet.

       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    |    Reserved, 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)                   |


Note: Msg. Size is the real block size include padding

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

     Sync Connection format (sync_conn)

       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  (in sec.)                |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                              ...                              |
      |                        IP-Addresses  (v4 or v6)               |
      |                              ...                              |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  Optional Parameters.
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | Param. Type    | Param. Length |   Param. data                |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                               |
      |                              ...                              |
      |                               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                               | Param Type    | Param. Length |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                           Param  data                         |
      |         Last Param data should be padded for 32 bit alignment |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

New fields that might need some explanation,

Type:
  Bit    7        6        . . .              1           0
    +--------------------------------------------------+-------+
    |                      Spare                       |  IPv6 |
    +--------------------------------------------------+-------+

Bit 0 = 1 This sync_conn contains IPv6 addresses.


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

Size;    not including ev. padding

fwmark:  Firewall mark from skb.

timeout: from ip_vs_conn struct, converted to sec.


Optional Parameters will be added at the end without any kind of alignment
each Param will have a header of two bytes

Param. header
 Bit    7        6        . . .    0    7                      0
   +----------+----------------------+---------------------------+
   | Optional |  Parameter Type      |  Parameter length         |
   +----------+----------------------+---------------------------+
   |  Data ...                                                   |
   |  ...                                                        |
   +----------+----------------------+---------------------------+
Bit 7 (msb) Optional Param, conn. entry could be keept, if not type
            is unknown.
Bit 6-0 Option Type defined below

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

Option Length, 1-255 bytes 0 is an illegal value.

PATCH STATUS:
 - Persistence data for IPv6 is not tested.


THANKS
To Julian and Simon for the review and alternative solutions.

*v4
 Corrections and enhancements acording to Julians review
 See individual patches.
 Change of principle, length for individual sync_conn
 does not contain padding.

*v3
 New: Ability to send ver. 0 sync messages.
 Optional data, renamed to Optional parameter
 Corrections, see comments in each patch,
 
*v2
 Simplified fwmark handling patch 1/4
 New handling of optional data patch 2/4
 Seconds in timeout
 Basically all changes is based on Julians and Simons comments on the RFC
 For details see individual patches.
--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [*v4 PATCH 0/3] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Mon, 15 Nov 2010, Hans Schillstrom wrote:

> This patch series adds/(updates) the following functionality
> in the synchronization between master and backup daemons.
>
> - IPv6
> - Persistence Engine
> - Firewall marks transferred
> - Time-outs transferred.
> - Flag field increased to 32 bits.

 	Great work! I don't see other fatal problems. May be you
can finally fix some grammatical and space problems and we can
wait next weeks for comments about the new message format from
other IPVS users. We also talked about checking if cp->pe
matches cp->dest->svc->pe in ip_vs_find_dest(). Or may be it
is already handled by the recent change that compares p->pe
with cp->pe before calling ct_match in ip_vs_ct_in_get?

v4 PATCH 1/3
 	- still the case for pe_data_len=0 and pe_name_len!=0 is
 	not handled properly as error because we now ignore
 	pe_name silently

v4 PATCH 2/3
 	ip_vs_sync_conn:
 		- I now see that we can calculate the padding
 		from previous message, so that we can skip
 		sending padding after the last connection:

 		pad = (4 - (int) curr_sb->head) & 3;

 		then we should clear the data before message:

 		p = curr_sb->head;
 		curr_sb->head += pad;
 		while (pad--)
 			*(p++) = 0;
 		s = (union ip_vs_sync_conn *) p;

 		- Some checks are not needed, may be
 		cp->pe_data_len is enough:

 		-if (cp->pe_data_len && cp->dest->svc && cp->pe && cp->pe->name)
 		+if (cp->pe_data_len)

v4 PATCH 3/3
 	OK

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [*v4 PATCH 0/3] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Mon, Nov 15, 2010 at 07:31:26PM +0100, Hans Schillstrom wrote:
> This patch series adds/(updates) the following functionality
> in the synchronization between master and backup daemons.
> 
>  - IPv6
>  - Persistence Engine
>  - Firewall marks transferred
>  - Time-outs transferred.
>  - Flag field increased to 32 bits.
> 
> Note:
> It depends on Patch 1-9 in staging branch.
> "IPVS: Backup, Prepare for transferring firewall marks (fwmark) to the backup daemon."
> to ...
> "IPVS: skb defrag in L7 helpers"

Hi Hans,

unfortuantely I am a bit confused about which tree your patches apply to.

I have sent a pull request to Patrick just now and as part of that
I have updated the master, for-patrick, and stable branches such
that they are currently all the same thing. This means stable has
been purged and then re-added. In the process I dropped the following
patches:

* IPVS: ip_vs_pe.c, use strncmp to be safe.

  Julian didn't seem very happy with this change.
  He suggested using pe->name_len to be safe.

* IPVS: Prepare for transferring firewall marks (fwmark) to the backup daemon.

  I believe you have revised this idea

* IPVS: Backup, Adding structs for new sync format

  I believe this one has changed too.

I notice that Julian made some comments too.  When you are ready could you
please post a fresh patch series, that includes all required patches and
applies cleanly on top of the master branch of lvs-next-2.6?

Re: [*v4 PATCH 0/3] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Tuesday 16 November 2010 07:44:39 Simon Horman wrote:
> On Mon, Nov 15, 2010 at 07:31:26PM +0100, Hans Schillstrom wrote:
> > This patch series adds/(updates) the following functionality
> > in the synchronization between master and backup daemons.
> > 
> >  - IPv6
> >  - Persistence Engine
> >  - Firewall marks transferred
> >  - Time-outs transferred.
> >  - Flag field increased to 32 bits.
> > 
> > Note:
> > It depends on Patch 1-9 in staging branch.
> > "IPVS: Backup, Prepare for transferring firewall marks (fwmark) to the backup daemon."
> > to ...
> > "IPVS: skb defrag in L7 helpers"
> 
> Hi Hans,
> 
> unfortuantely I am a bit confused about which tree your patches apply to.

I was your staging branch, as you suggested.

> 
> I have sent a pull request to Patrick just now and as part of that
> I have updated the master, for-patrick, and stable branches such
> that they are currently all the same thing. This means stable has
> been purged and then re-added. In the process I dropped the following
> patches:
> 
> * IPVS: ip_vs_pe.c, use strncmp to be safe.
> 
>   Julian didn't seem very happy with this change.
>   He suggested using pe->name_len to be safe.
> 
> * IPVS: Prepare for transferring firewall marks (fwmark) to the backup daemon.
> 
>   I believe you have revised this idea
> 
> * IPVS: Backup, Adding structs for new sync format
> 
>   I believe this one has changed too.
> 
> I notice that Julian made some comments too.  When you are ready could you
> please post a fresh patch series, that includes all required patches and
> applies cleanly on top of the master branch of lvs-next-2.6?

I will send new fresh ones today for all that I need

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

Re: [*v4 PATCH 0/3] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Tue, Nov 16, 2010 at 08:22:35AM +0100, Hans Schillstrom wrote:
> On Tuesday 16 November 2010 07:44:39 Simon Horman wrote:
> > On Mon, Nov 15, 2010 at 07:31:26PM +0100, Hans Schillstrom wrote:
> > > This patch series adds/(updates) the following functionality
> > > in the synchronization between master and backup daemons.
> > > 
> > >  - IPv6
> > >  - Persistence Engine
> > >  - Firewall marks transferred
> > >  - Time-outs transferred.
> > >  - Flag field increased to 32 bits.
> > > 
> > > Note:
> > > It depends on Patch 1-9 in staging branch.
> > > "IPVS: Backup, Prepare for transferring firewall marks (fwmark) to the backup daemon."
> > > to ...
> > > "IPVS: skb defrag in L7 helpers"
> > 
> > Hi Hans,
> > 
> > unfortuantely I am a bit confused about which tree your patches apply to.
> 
> I was your staging branch, as you suggested.

Ok, perhaps I messed something up on my end.
Sorry about that.

Re: [*v4 PATCH 0/3] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Monday 15 November 2010 23:29:11 Julian Anastasov wrote:
> 
>  	Hello,
> 
> On Mon, 15 Nov 2010, Hans Schillstrom wrote:
> 
> > This patch series adds/(updates) the following functionality
> > in the synchronization between master and backup daemons.
> >
> > - IPv6
> > - Persistence Engine
> > - Firewall marks transferred
> > - Time-outs transferred.
> > - Flag field increased to 32 bits.
> 
>  	Great work! I don't see other fatal problems. May be you
> can finally fix some grammatical and space problems and we can
> wait next weeks for comments about the new message format from
> other IPVS users. We also talked about checking if cp->pe
> matches cp->dest->svc->pe in ip_vs_find_dest(). Or may be it
> is already handled by the recent change that compares p->pe
> with cp->pe before calling ct_match in ip_vs_ct_in_get?

I do think it's handled by patch
 "Only match pe_data created by the same pe"

> 
> v4 PATCH 1/3
>  	- still the case for pe_data_len=0 and pe_name_len!=0 is
>  	not handled properly as error because we now ignore
>  	pe_name silently

Do you mean that IP_VS_DBG should be replaced ?

	if (pe_data_len) {
		if (pe_name_len) {
...
		} else {
			IP_VS_DBG(3, "BACKUP, Invalid PE parameters\n");
			return 1;
		}
to this ?
	...
			IP_VS_ERR_RL("BACKUP, Invalid PE parameters\n");
    ...
> 
> v4 PATCH 2/3
>  	ip_vs_sync_conn:
>  		- I now see that we can calculate the padding
>  		from previous message, so that we can skip
>  		sending padding after the last connection:

Done.

> 
>  		pad = (4 - (int) curr_sb->head) & 3;
> 
>  		then we should clear the data before message:
> 
>  		p = curr_sb->head;
>  		curr_sb->head += pad;
>  		while (pad--)
>  			*(p++) = 0;
>  		s = (union ip_vs_sync_conn *) p;
> 
>  		- Some checks are not needed, may be
>  		cp->pe_data_len is enough:
> 
>  		-if (cp->pe_data_len && cp->dest->svc && cp->pe && cp->pe->name)
>  		+if (cp->pe_data_len)

OK, I'll do that
i.e. we should accept the result of: "pe or name is null" as a BUG...

> 
> v4 PATCH 3/3
>  	OK
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
> 

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

Re: [*v4 PATCH 0/3] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Tue, 16 Nov 2010, Hans Schillstrom wrote:

>> v4 PATCH 1/3
>>  	- still the case for pe_data_len=0 and pe_name_len!=0 is
>>  	not handled properly as error because we now ignore
>>  	pe_name silently
>
> Do you mean that IP_VS_DBG should be replaced ?
>
> 	if (pe_data_len) {
> 		if (pe_name_len) {
> ...
> 		} else {
> 			IP_VS_DBG(3, "BACKUP, Invalid PE parameters\n");
> 			return 1;
> 		}
> to this ?
> 	...
> 			IP_VS_ERR_RL("BACKUP, Invalid PE parameters\n");
>    ...

 	No, I'm talking about one missing case:

 	/* Handle pe data */
 	if (pe_data_len) {
 		if (pe_name_len) {
 		} else {
 			IP_VS_DBG(3, "BACKUP, Invalid PE parameters\n");
 			return 1;
 		}
 		... kmalloc ...

 	THIS CASE IS MISSING:

+	} else if (pe_name_len) {
+		IP_VS_DBG(3, "BACKUP, Invalid PE parameters\n");
+		return 1;
 	}

 	That is why my first example was a sort of XOR operation:

 	/* Handle pe data */
 	if ((pe_data_len != 0) != (pe_name_len != 0)) {
 		IP_VS_DBG(3, "BACKUP, Invalid PE parameters\n");
 		return 1;
 	}
 	if (!pe_name_len)
 		return 0;

 	it means: PE DATA and PE NAME must come together.

 	because we have 4 possible cases:

1. pe_name_len==0 && pe_data_len==0 => VALID, return 0
2. pe_name_len!=0 && pe_data_len==0 => INVALID, return 1
3. pe_name_len==0 && pe_data_len!=0 => INVALID, return 1
4. pe_name_len!=0 && pe_data_len!=0 => VALID, kmalloc, return 0

Regards

--
Julian Anastasov <ja@ssi.bg>