[reSIProcate] upcoming changes to DUM's RegistrationPersistenceManager and ServerRegistration

[Adam Roach]

Overall, the proposal looks sound to me. A few minor comments inline.

Also, I know that David Bryan has been doing some investigation into how 
to modify these data structures so that they have hooks to support 
storage of P2P information in registration records. You probably want to 
talk with him to determine if there are some things you can do in the 
refactoring to make such changes easier and/or possible.

Rohan Mahy wrote:

>
> class BaseContactRecord {
>    NameAddr contact;    // can contain callee caps
>    time_t regExpires;
>    time_t lastUpdated;
>    Tuple receivedFrom;  // source transport, IP address, and port
>    UriList sipPath;     // Value of SIP Path header from the Request
> };


Does the NameAddr contain the GRUU information also? That is, is this 
going to be an exact copy of what comes off the wire, or do we have to 
tear it apart and resynthesize it? If it's the latter, then I think I 
like the other approach -- of storing the actual capabilities as a 
separate item -- better. (In fact, it might even be better in the former 
case). I'm thinking of how I would go about implementing a search for 
the correct contact based on caller prefs; it would be much faster if 
the callee caps were already broken down and pre-digested in an 
appropriate structure (e.g. a hash of capabilities).

> class Aor {
>    Uri aor;
>    SimpleContactList contacts;
>    InstanceList instances;
>    UriList aliases;
> };


I suppose that the "aliases" field here is used to remove aliases when 
deregistration occurs, and nothing else, right?


>
> 2. ServerRegistration::dispatch first processes a registration by 
> first locking then adding, updating, or deleting records in the 
> "database", then if reject is called, we roll back the changes.  
> accept() and reject() unlock the database.  This is problematic for a 
> number of reasons. This is a DoS bait if I ever saw it. Sending a lot 
> of illegitimate REGISTERs can lock the database and prevent the 
> LocationServer from accessing the locked records.


When I did this, I intended to eventually overhaul it to be more like a 
two-phase commit, where the database could confirm the update before 
effecting it, but the application would have the opportunity to enforce 
policy before readers had access. It was a Known Ugly Hack just to get 
things up and running. :)

> - We don't want Registrations to impact the actual database unless 
> they are actually accepted
> - We should lock the database as briefly as possible. It would be 
> great if we only needed write locks (we could do this if all the 
> operations on the database were atomic)*
>

It's completely unclear to me how you can make reads through STL 
structures atomic. I think you'll probably still need AOR-level locking 
here. Read/Write locks make much more sense than mutexes.

> I propose refactoring ServerRegistration so we pass a list of 
> operations (memory owned by DUM) in the ServerRegistrationHandlers.  
> The application can inspect the operations and/or the original 
> request.  accept() would cause the list of operations to be committed 
> to the database (quick lock/unlock), return the current list of 
> registered contacts (with and without instances), and free the list of 
> operations.  reject() would free the memory.


That sounds like a good design to me.

> We can use the same operations for updates from other cluster members 
> when we add high availability.


Yep. I like that.

/a