Nirmal,
target.dtd
201-203: NIT - align indents with previous lines
test_zpool_vdevs.py
480 - can you change the name of the function to be a little bit more
descriptive? It helps when looking at the test reports to know what the
test was (if it ever fails) from just the name.
519 - same comment
Otherwise this looks good! Thanks for doing this!
-Drew
On 2/17/12 9:58 AM, Nirmal Agarwal wrote:
Hi Karen
Thanks for the review. Please find the update webrev.
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7070697-2/webrev/
Approach :
--> size is optional only in case of "swap" zvol's.
--> since we can have more than 1 swap zvol in a pool, I have
restricted that we can have only 1 zvol without size defined. In my
opinion, having multiple zvol for swap on the same pool is not of much
use. It can be changed if you feel the need of removing this
restriction.
Changes :
usr/src/lib/install_target/instantiation.py
I have changed the order and created swap zvol(s) in the last.
usr/src/lib/install_target/logical.py
changes are mainly to make size subelement optional.
usr/src/lib/install_target/shadow/logical.py
introduced check for multiple zvol without size in the same pool.
usr/src/lib/install_target/test/test_shadow_list.py
corresponding test case
usr/src/lib/install_target/test/test_zpool_vdevs.py
test case for zvol with optional size
I have introduced comments in target.dtd as suggested. Though I am not
sure of changes for ai_manifest. Do I need to file doc/man page bug
for the same ?
Testing :
I have done following tests with custom image :
1) 2 Zvol without size subelement in the same pool. It fails.
2) Zvol without "use" as swap . It fails.
3) 2 Zvol without size subelement in different pools. It succeeds.
Source is pep8 clean .
Run the slim_test. Please find the output of test at below location :
/net/indiana-build.us.oracle.com//export/home/na210770/ai/7070697/slim_test
Regards
Nirmal
On 02/16/12 04:36, Karen Tung wrote:
Hi Nirmal,
Please see my comments inline.
On 02/15/12 10:12 AM, Nirmal Agarwal wrote:
Hi Karen
I tested the scenario's you suggested. Below are my findings and
suggested solution(s) :
On 2/15/2012 12:35 AM, Karen Tung wrote:
Hi Nirmal,
Dave's question below prompted me to think of a couple of
questions on the proposed bug fix:
1) Since size is now optional, I can have more than 1 zvol
where I don't specify the size. Based on your fix, each of those
zvols that does not have a size will get 90% of the zpool size. That
obviously
won't work. How would this situation be handled?
As per the current implementation, it creates 2nd zvol as 90 % of
remaining pool size left after creation of 1st zvol (which is
effectively 9 % of zpool).
We can handle this situation in following manner :
--> allow only 1 zvol with undefined size.
--> allow undefined size zvol for "swap" use only.
--> create the swap in last to allocate 90 % of available space in
pool.
Among the 3 choices above, I like the choice to allow undefined zvol
size for "swap" use only
the best. I don't like the first choice, because it seems kinda odd to
allow only 1 zvol with
undefined size in general. I don't like the 3rd choice because we still
can have more than 1 zvols
with undefined size, and anything besides the first undefined size zvol
is pretty much useless.
2) A related question, if I specify a zpool that have both ZFS
filesystems
and a zvol where I didn't specify a size, is it necessary for us to
warn the
user that the ZFS filesystem that will be created will only be 10%
size of the pool.
I will introduce a "warn" level debug message whenever we create zvol
with 90 % of the pool size.
A "warning" level debug message during execution is certainly useful. In
addition,
I think we can improve the user experience as follows:
1) Since you are changing the target.dtd, put some comment in the
section
where the zvol size is optional, and discuss what will happen if the
user
did not specify a size. That way, people won't be surprised when they do
the
actual install.
2) If we provide any sample AI manifests for users to use as starting
points,
also put some comment there talking about the behavior.
Thanks,
--Karen
Please let me know your comments on the approach.
Thanks
Nirmal
Thanks,
--Karen
On 02/14/12 08:28, Nirmal Agarwal wrote:
Hi Dave
I did test the full installation and it works.
Let me know if you want to run any other tests.
Thanks
Nirmal
On 02/14/12 03:22, Dave Miner wrote:
I haven't reviewed the code here, but can you elaborate on the
testing?
Does AI create a correctly usable swap volume, and will Solaris boot
with that swap volume once you've done so? It's not entirely clear
to me
that this will work.
Dave
On 02/13/12 00:12, Nirmal Agarwal wrote:
Hi all
Could I please get a code review for the following CR:
7070697 size element for zvols should be optional
Webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7070697/webrev/
slim_test result
----------------
/net/indiana-build.us.oracle.com//export/home/na210770/ai/7070697/slim_test
Manual Tests :
-- created a custom image and tested the fix with the manifest not
describing the size of zvol.
Thanks
Nirmal
_______________________________________________
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
