On 05/24/12 13:44, Ethan Quach wrote:
On 05/24/12 01:28, Matt Keenan wrote:
Ethan,
Comments below.
On 05/23/12 20:59, Ethan Quach wrote:
On 05/23/12 01:55, Matt Keenan wrote:
Ethan,
Thanks for the review.
On 05/22/12 21:17, Ethan Quach wrote:
Hi Matt,
172 - Question: Is it indeed required that /var/share live in the
root pool?
As var share is specified to be "in_be", and a BE can only reside
on the root pool then yes it has to be on the root pool.
I think you misstated that, the /var/share filesystem (i.e.
VARSHARE) is not in_be.
Let me restate the question, would this be a valid configuration?
rpool/ROOT/solaris /
rpool/ROOT/soalris/var /var
....
foopool/export /export
foopool/export/home /export/home
foopool/VARSHARE /var/share
Or is the /var/share filesystem actually required to be in the same
pool as the root pool for some reason? (I just want to make sure
we're not enforcing something that isn't required.)
Sorry read your original comment a little fast. 27 C(80 F) heat in
the office must be effecting my vision.
To be honest I don't know, I'm going to get clarification from ZFS
team and if it's not a requirement I'll ensure we don't enforce it.
Got the answer from Dave's response.
266 - With this line being moved here, are we missing the setting
for canmount for /var now?
Setting of "canmount" on /var was never required and is not being
done anywhere. It's only set on VARSHARE.
That can't be. Generically, any child dataset underneath a BE's
root dataset must have canmount=noauto. Otherwise, the call to "zfs
mount -a" from the filesystem/local service method ends up trying to
mount that filesystem from all BEs causing failure of that service.
It must be getting set by default on creation as our code never set
canmount for /var we have only ever enforced it's setting on VARSHARE
which is what fs-minimal does. The BE specific datasets are created
via be_init, maybe it sets them. I just checked a fresh non-global
zone instal and rpool/ROOT/solaris/var has canmount=noauto
Look in your webrev, see line 218 of the old code. The comment on
line 217 says it's setting canmount on VARSHARE, but the code at line
218 actually sets it on VAR_DATASET_NAME.
That looked like it was actually unintentional, but nevertheless it
did the needed thing, 'var' definitely needs to have canmount=noauto,
otherwise what I describe above happens.
I just looked at libbe, during be_init() we do actually enforce that all
non-shared datasets get created with their canmount property set with
noauto.
So it seems you're actually fixing a bug here in the old varshare code
... it just wasn't obvious to me when looking at the old line 218. I
think its fine now.
thanks,
-ethan
Requiring all children datasets that live hierarchically under a BE's
root dataset have its canmount property equal 'noauto' is part of the
design of zfs boot environments, see PSARC/2006/370. The 'canmount'
property was actually integrated early, see PSARC/2008/168, so that
beadm could use it when we initially implemented beadm(1M) and libbe
during OpenSolaris.
-ethan
cheers
Matt
For cases where VARHSARE is in the manifest, a user must also
explicitly specify compression=<something>, for our code to not
glean that property from /var, is that correct?
If VARSHARE is specified in the manifest and it does NOT have a
specifiic compression value set, then our code
will only attempt to set compression on VARSHARE :
- if Specified on /var
- if Specifiied on BE
Thanks for the clarification.
Specification on /var Filesystem element takes precedence over BE
element.
Nit -- The code paths starting at 211 and 266 are now getting a
bit unwieldy, and they seem to try to handle the same thing --
setting the Options for the filesystem at hand. Is there any way
we can consolidate these paths?
We are attempting to set the Options in two slightly different
scenarios e.g. VARSHARE exists or VARSHARE does not exist. I've
tried adding utility functions as best I can, and I feel what's
there at the moment is sufficient. As pointed out to be before it's
actually more efficient in Python to not have too many utility
methods.
I wasn't necessarily thinking adding a function, but its ok, this
was just a nit, I'll drop this.
thanks,
-ethan
cheers
Matt
thanks,
-ethan
On 05/22/12 06:46, Matt Keenan wrote:
Hi,
Can I get two pairs of eyes to look over this fix please:
Bug
http://monaco.us.oracle.com/detail.jsf?cr=7165978
7165978 AI should create VARSHARE with compression as per /var
Webrev:
https://cr.opensolaris.org/action/browse/caiman/mattman/7165978
This bug was uncovered via zones testing. The default manifest
for zones installation specifies a BE option to turn on
compression for all datasets created within that zone. However as
VARSHARE is not within the BE it needs to inherit the compression
property from var dataset that's created within the BE.
The changes I've made allow for a user to still specify
compression for var and VARSHARE to be different via manually
specification in an AI manifest. A warning message is shown if
they manually set it, but installation will complete.
This CR also fixes a scenario for canmount not being set if
VARSHARE was specified in a manifest.
Testing:
- Build ISOS for AI, TI and GUI and tested full installs and
verified
on first boot compression property set to the same value for
var and
VARSHARE datasets.
- For AI and zones installs, I tried out a number of different
scenarios where compression is set with within BE or manually via
Filesystem elements for var and VARSHARE. Always ensuring that var
and VARSHARE are the same unless VARSHARE manually set.
- Ran full set of slim_source unit tests and no regressions found.
cheers
Matt
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
