[dm-devel] uid_attrs (was Re: [PATCH 04/11] multipathd: add need_do_map to indicate whether need calling domap() in ev_add_path())

[Martin Wilck]

On Wed, 2017-04-05 at 13:45 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 05, 2017 at 03:26:06PM +0200, Martin Wilck wrote: 

> > >From the end user point of view, "uid_attrs" seems to simply
> duplicate
> > the functionality of "uid_attribute", just in more awkward syntax,
> and
> > with higher priority. As a side effect, it enables uevent merging,
> > which is documented in the man page but unexpected from the name of
> the
> > option.
> 
> It should probably do a better job of masking "uid_attribute". The
> places in the code that currently use "uid_attribute" should all
> switch
> to using "uid_attrs" if it is set and applies to the device. 
> If they
> don't, we could be in a situation where "uid_attrs" and
> "uid_attribute"
> are set to give different wwids, and a path's wwid could change based
> on
> whether or not ID_SERIAL was filled in when multipath got

I'm not quite sure what you mean. The way it's currently implemented is
in select_getuid(), pp->uid_attribute will be set from uid_attrs if it
exists and matches the device name, and will henceforth be used
everywhere where uid_attribute for this path is referenced.
I don't see how this could lead to inconsistent settings.

Even for RBD devices, uid_attrs would take precedence if users could 
uid_attrs="rbd:SOME_VARIABLE". (Btw, why do we hardcode the WWID
generation for RBD rather than writing rbd udev rules and using the
udev attributes created by them)?

> > Anyway, I wonder if it would still be possible to change this at
> > this
> > point in time (i.e. after "uid_attrs" was merged).
> 
> It's always possible to change things.  As far as Red Hat goes, we
> haven't pulled in these changes yet, but I'm planning on rebasing
> Fedora
> to upstream shortly.

We're planning the same for openSUSE Factory. Currently we've left out
Tang's patch, but the plan is to pull in upstream cleanly.

>   So, it won't cause me any problems if uid_attrs
> goes away as long as it does so in the near future. I have no idea if
> other distributions have pulled this in. If so, then uid_attrs
> probably
> needs to remain accepted as a valid option by multipath.  But since
> all
> correct configurations have it give the same answers as
> uid_attribute,
> we could probably just make the option do nothing.
> 
> Of course, all this depends on your solution actually being enough
> better than Tang's to justify pulling his version out.

Point taken :-) I don't have patches at this time. However, let me make
a suggestion. Does "uid_attrs" really have to be user-configurable? IMO
configuring this is asked too much from 99% of users. I'd suggest to
hard-code the value of uid_attrs and use a boolean configuration option
in the defaults section ("auto_uid_attribute", say) which would control
whether WWID attributes should be configured "the uid_attrs way" or the
"traditional way" (traditional being the fallback if uid_attrs didn't
match, like now). Hard-coding this would have the advantage that we
wouldn't be bound to the "sd:..." syntax. Rather, we could explore more
intelligent ways to autodetect the uid_attribute depending on path
properties which are known early on.

Uevent merging could be controlled by a separate config option which
would depend on the "auto_uid_attribute" option. This would mainly be
done for clarity.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)