On 10/16/2012 04:52 PM, Iustin Pop wrote:
On Wed, Sep 26, 2012 at 05:38:19PM +0300, Constantinos Venetsanopoulos wrote:
Add support for passing parameters to the ext template (ext-params).
Take advantage of disk-params, that don't seem to make much sense in
this template (ExtStorage Providers are not predefined and we don't
know their needs) and use them to pass the ext-params dynamically to
the template.
ext-params are correlated with gnt-os-interface's os-params.
All ext-params are exported to the ExtStorage Provider through it's
environment, with variables prefixed with 'EXTP_' (similarly to the
OS interface's 'OSP_' params).
ext-params are passed through the --disk option. If the disk template
is of type `ext' during instance add, then any additional options that
are not in IDISK_PARAMS given to --disk are considered ext-params
e.g.:
gnt-instance add -t ext --disk=0:size=2G,param1=value1,param2=value2
Finally, we introduce a new IDISK_PARAM called IDISK_PROVIDER, that is
mandatory for template `ext' and is used to select the desired
ExtStorage Provider. This parameter is not valid for other template
types.
The IDISK_PROVIDER parameter becomes the first element of the
disk's unique_id tuple e.g.:
unique_id = ('sample_provider1', 'UUID.ext.diskX')
Example selecting different ExtStorage Providers for each disk and
passing different ext-params to them:
-t ext --disk=0:size=2G,provider=sample_provider1,param1=value1
--disk=1:size=3G,provider=sample_provider2,param2=value2
I know understand better patch 4/7.
While we might end up with the interface you currently propose based on
practicality of CLI input, I would like to raise some concerns on it.
The problem is that we're mixing here two things which we shouldn't.
Basically the structure is:
{ size: Int,
mode: ReadWrite,
extp: { }
}
But at cli level (and, I believe, at opcode level too) we're flattening
this structure. This leads to the complications in 4/7 w.r.t. the
validation of disk options, and to the impossibility, for example, that
we have an extp with the name 'size' (ignoring case for the moment).
So I'm wondering if we couldn't separate, both at CLI and opcode level,
these two. Maybe having --disk=0:size=2G,params=<some other format> or
--disk 0:size=2G --dparams 0:p1=v1,p2=v2 or anything else that clearly
separates them.
Just thinking out loud…
I had thought about these points (!) a lot of times before writing the
code and these were the reasons that got me thinking about Storage Pools
initially. It's great to see we are aligned in such a way.
As you suggest, I had also thought of the exact two solutions you propose.
However, the complication comes from the current design at opcode
and lu level, rather than the CLI. Specifically:
Currently we have the following IDISK_PARAMS:
size, mode, adopt, vg, metavg
1. 'size', 'mode' and 'adopt' are indeed separate "disk parameters"
2. 'vg' and 'metavg' are actually "template parameters"
3. 'metavg' is a "template parameter" and is already included in DRBD's
"disk-params"
4. 'vg' is not included in DRBD's "disk-params", although it probably should
(as discussed in the Storage Pools thread)
The above is a bit confusing design-wise imho, and flattening already
happens at cli level; that's why I kept the ext-params there as well
(for practicality).
I've given these issues quite some thought [that's why I delayed my
reply] to find the best way to handle them, also having in mind your and
Guido's comments on Storage Pools. This is what I've come up with so far:
* Some "template-params" need to stay the same at nodegroup level
(e.g.: DRBD_NET_CUSTOM), and some need to be changed at disk/instance
level (e.g.: DRBD_DEFAULT_METAVG and 'vg')
* Also ext-params for the 'ext' template need to be set at disk level
I would propose the following:
For now:
- Keep the current ext-params interface for practicality
As a next step to pair with the Storage Pools implementation:
- Rename current "disk-params" to "template-params" to reflect their exact use.
- Make 'vg' a "template-param" for LVM and DRBD and remove it from cluster init.
(this also solves the problem that setting the vg_name during cluster init
doesn't also set the metavg correctly)
- Introduce a new variable for each template (e.g. for drbd:
"DRBD_DYNAMIC_PARAMS")
modeled as a frozenset containing the template's "template-params" that can
be modifiable at disk level
(for drbd these would be the 'DRBD_DEFAULT_METAVG' = 'metavg' and
'DRBD_DEFAULT_VG(?)' = 'vg',
for rbd that could be 'RBD_POOL' inside 'RBD_DYNAMIC_PARAMS').
- Keep "template-params" inheritance intact at cluster and nodegroup level.
- Introduce Storage Pools
- After the above, IDISK_PARAMS will end up containing only the following:
size, mode, adopt, pool (which are all disk level specific)
- Allow setting ONLY the modifiable *_DYNAMIC_PARAMS at Storage Pool level
which will override the corresponding nodegroup level "template-param".
- Just connect Storage Pools to nodegroups without modifying parameters during
connect (as initially proposed by Guido).
Then we will be able to even make ext template's 'provider' a modifiable
"template-param", with the constraint that in an 'ext' Storage Pool this
parameter should ALWAYS be set.
Finally, we can rethink how to display ext-params at CLI, after we give them
more thought at the Storage Pool level, and decide exactly how they will find
their way down to bdev. Maybe then, they can get their own CLI option (e.g.,
--dparams), but I think we should re-discuss that after
the design/implementation of Storage Pools.
What do you think? Looking forward to your comments,
Constantinos
