Re: [Nbd] nbd-server: about adding new exports dynamically

[Tuomas Jorma Juhani Räsänen <tuomasjjrasanen@...1258...>]

On Sun, Dec 23, 2012 at 10:49:59PM +0100, Wouter Verhelst wrote:
> On Sat, Dec 22, 2012 at 09:34:21PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> 
> > It does not alter any of the existing exports, it only adds new ones.
> > In our environment, this is to make it safe (non-destructive) to use.
> > In our environment, it is non-destructive operation, because we make
> > sure that we only add new configuration fragments to
> > /etc/nbd-server/conf.d and make no other configuration changes.
> > However, in its current form, it is not non-destructive in general,
> > because it calls parse_cfile() which in turn modifies global
> > variables. In my opinion, parse_cfile() should be refactored to make
> > it side-effect free.
> 
> Yes, I agree. In fact, I removed almost all global variables from
> nbd-server a few years back (originally everything was stored in a
> global variable, and you couldn't even serve more than one export from a
> single nbd-server process), but apparently I forgot about that since and
> put some back. That's a bit silly, of course.
> 

Well, I think I could try to look at that. parse_cfile() is quite big
currently. Refactoring it would be the first step towards generic
reconfiguration solution.

> > Do you think the SIGHUP-based reconfiguration method is viable?
> 
> It is exactly what has been on my low-priority TODO list to support. I
> agree that not being able to reload configuration is a bug, but it's not
> something I've had time to work on so far. However, if there were a
> patch, that'd change my priorities ;-)

As I said, I do not have a generic solution yet. I have only a quickly
tested and specially crafted patch, which assumes non-destructive
configuration changes and new-style configuration:
https://github.com/tuomasjjrasanen/nbd/commit/2ed07f8771617a883e107db7fa3697b5da54bd88

It works, but obviously isn't ready for the upstream.

> 
> As said, I've been thinking about doing this myself off and on, but it
> just hasn't happened yet. The way I see it, such a reconfiguration would
> need to do something like the following:
> 
> - Read the config file, make sure it is error free (if not, produce an
>   error message, but continue serving as usual). This should also
>   involve things that can't be changed anymore by a simple SIGHUP (e.g.,
>   changing the value of the "user" or "group" global options)

Yes, agreed. This would be the simplest possible SIGHUP-handler. It
would only read configuration files and log all inconsistencies found.

> - Verify all currently-existing exports, and remove the ones that are no
>   longer present.

Yes. I think there is need for two different kind of behaviors for
both update and delete actions: soft and hard.And the choice between
soft and hard could be parameterized via the main configuration file,
for example with following options:

hard_update = true|false # defaults to false
hard_delete = true|false # defaults to false

The "soft" deletion behavior would be to delete exports that are no
longer present in the new configuration and that are not served (not
having clients).

The "hard" behavior would be to just delete exports that are no
longer present in the new configuration, no matter if there are
clients or not.

> - Check all currently-existing exports, and update the settings for
>   those what's in the config file isn't the same anymore compared to
>   what's currently being served

> - Kill any client connections which are using now-illegal options (e.g.,
>   clients which are writing to an export that has just been turned into
>   a read-only connection), or which are using exports that are no longer
>   being exported

Yes, this would be "hard" update. 

> - Alternatively, when a client is using an export, we can refuse
>   (verbosely, i.e., log something) to update the configuration for that
>   export (which would probably be somewhat easier to implement).

And this would be "soft" update.

> 
> However, if it's somewhat more limited than that, I suppose I'll accept
> it too, provided it's properly documented what the limitations are.
> 

Well yes, currently it is more limited. As I said, it is tailored
pretty much to our case, but I am ready to make it more general.

I think that next steps would be something like:

PATCH 1: make configuration parsing side-effect free

PATCH 2: read configuration on SIGHUP

PATCH 3: add new exports on SIGHUP

PATCH 4: soft-update current exports on SIGHUP

PATCH 5: hard-update current exports on SIGHUP

PATCH 6: soft-delete current exports on SIGHUP

PATCH 7: hard-delete current exports on SIGHUP

I'll begin with PATCH 1, that is something which should be done
anyway.

-- 
Tuomas