Puppet users - Feb 2008 - Re: [PATCH 2/5] basic xen domU creation and destruction

Hi all!

Having asked anarcat to send the patches to the list, I''m obviously in 
favor of having such patches out in the open, where they can be 
discussed by the public, instead of buried in my mailbox, where they may 
"rot in peace".

I have to admit though, that this is all work in progress, so any and 
all input is very welcome. Currently there are one or two sets of 
patches every month, so I hope it won''t be too much of a burden on the 
list and IMHO only if this traffic increased significantly, a seperate 
mailing list should be created.


Back to the meat:

anarcat schrieb:
> only works with LVM for now
> ---
>  files/xen-tools.conf |  184
++++++++++++++++++++++++++++++++++++++++++++++++++
>  manifests/xen.pp     |   58 +++++++++++++++-
>  2 files changed, 241 insertions(+), 1 deletions(-)
>  create mode 100644 files/xen-tools.conf
> 
> diff --git a/files/xen-tools.conf b/files/xen-tools.conf
> new file mode 100644
> index 0000000..e766551
> --- /dev/null
> +++ b/files/xen-tools.conf
> @@ -0,0 +1,184 @@
> diff --git a/manifests/xen.pp b/manifests/xen.pp
> index 613c15a..935adf2 100644
> --- a/manifests/xen.pp
> +++ b/manifests/xen.pp
> @@ -49,8 +49,64 @@ class xen::domain::xen0 inherits xen::domain {
>  		[
>  	        "xen-hypervisor-3.0.3-1-$architecture",
>  		"linux-image-xen-$architecture",
> -		''libsysfs2''
> +		''libsysfs2'',
> +		''xen-tools''
>  		]:
>  		ensure => present
>  	}
> +	file { "/etc/xen-tools/xen-tools.conf":
> +		source => "puppet://$servername/virtual/xen-tools.conf",
> +		mode => 0644, owner => anarcat, group => root,
"owner => anarcat" is no good. Also, please use config_file for
this, it
would have to right defaults then.
> +		require => Package[''xen-tools''];
> +	}
> +}
> +
> +# XXX: we have a logic issue here: if xen::domain is a class applied on a
node,
> +# then a xen::domain::xenU would also be applied on a node, but then it
> +# couldn''t be used to create the node in the first place since
the node
> +# wouldn''t exist...
> +# chicken and egg. we therefore don''t define xenU as a class but
as a define
> +# that is applied on the parent xen0
Yeah, I had the same problem with vservers and solved it the same: There 
is a "vserver" define which is applied in the host to create the
guests.

> +# ensure: present
> +# TODO: running, stopped, absent... (in that order?)
You already have implemented "absent". Does xen-create-image take care
about starting the image too? IIRC the domU config has to be manually 
placed into /etc/xen/auto/ or something to do this.

Anyways, I would recommend following the Service model and split it into 
"enable => true/false" - whether the domU should be started on boot
-
and "ensure => running/stopped"  - whether puppet should take care
of
starting the domU if it is down.
> +# TODO: resize LVM when $size changes?
lvresize would allow you that easily with "--size $size", but I would 
either advise against it ("Be careful when reducing a logical
volume''s
size,  because  data  in  the  reduced  part  is lost!!!" -- 
lvresize(1)) or at least do a very thorough check, that the new size is 
bigger than the old.
> +define xen::domain::xenU($ensure, $memory, $size, $swapsize = 0, $role =
"minimal", $vg = "vg00",
> +			 $ip = "", $netmask = "255.255.255.0", $gateway =
"", $mac = "") {
> +    case $name { '''': { fail ( "Cannot create Xen
domain with empty name" ) } }
> +    case $swapsize {
> +        0: {
> +	    $swap = "--noswap"
> +	}
> +	default: {
> +	    $swap = "--swap $swapsize"
> +	}
> +    }
> +    case $gatway { "": { $gateway_flag = "" } default:
{ $gateway_flag = "--gateway $gateway" } }
> +    case $mac { "": { $mac_flag = "" } default: {
$mac_flag = "--mac $mac" } }
> +    case $ip {
> +    	"": {
> +	    $network = "--dhcp $mac_flag"
> +	}
> +	default: {
> +	    $network = "--ip $ip --netmask $netmask $gateway_flag
$mac_flag"
> +	}
> +    }
Please take a look at the Style Guide [1] regarding the formatting of 
case statements. Personally I would set $..._real or _flag variables 
with the ?: operator instead of using case statements, where applicable.

> +    case $ensure {
> +    	"present": {
> +	    exec { "/usr/bin/xen-create-image --lvm $vg --memory $memory
--size $size $swap --hostname $name $network --debootstrap --role $role":
> +	    	creates => "/dev/mapper/$vg-$name--disk",
> +		alias => "xen_create_$name",
> +		require => [ Package[''xen-tools''],
File[''/etc/xen-tools/xen-tools.conf'']],
> +	    }
> +	}
> +	"absent": {
> +	    exec { "/usr/bin/xen-delete-image --lvm $vg $name":
> +		onlyif => "/usr/bin/test -e /dev/mapper/$vg-$name--disk",
> +	    	alias => "xen_delete_$name",
> +		require => [ Package[''xen-tools''],
File[''/etc/xen-tools/xen-tools.conf'']],
> +	    }
> +	}
> +	default: { err("${fqdn}: xen domain $name: unknown ensure
''${ensure}''") }
> +    }
>  }

Regards, DavidS

[1] http://reductivelabs.com/trac/puppet/wiki/StyleGuide

On Feb 16, 2008, at 3:06 AM, David Schmitt wrote:
> Hi all!
>
> Having asked anarcat to send the patches to the list, I''m
obviously in
> favor of having such patches out in the open, where they can be
> discussed by the public, instead of buried in my mailbox, where they  
> may
> "rot in peace".
>
> I have to admit though, that this is all work in progress, so any and
> all input is very welcome. Currently there are one or two sets of
> patches every month, so I hope it won''t be too much of a burden on
the
> list and IMHO only if this traffic increased significantly, a seperate
> mailing list should be created.

Okay, we''ll start with this and see how it goes.

-- 
Men never do evil so completely and cheerfully as when they do it from a
religious conviction. --Blaise Pascal
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

Wouldn''t it be better to send patches and such to Puppet-dev rather
than
Puppet-users?

Thanks
Brian

On 2/16/08, Luke Kanies <luke@madstop.com> wrote:
>
> On Feb 16, 2008, at 3:06 AM, David Schmitt wrote:
>
> > Hi all!
> >
> > Having asked anarcat to send the patches to the list, I''m
obviously in
> > favor of having such patches out in the open, where they can be
> > discussed by the public, instead of buried in my mailbox, where they
> > may
> > "rot in peace".
> >
> > I have to admit though, that this is all work in progress, so any and
> > all input is very welcome. Currently there are one or two sets of
> > patches every month, so I hope it won''t be too much of a
burden on the
> > list and IMHO only if this traffic increased significantly, a seperate
> > mailing list should be created.
>
>
> Okay, we''ll start with this and see how it goes.
>
> --
> Men never do evil so completely and cheerfully as when they do it from a
> religious conviction. --Blaise Pascal
> ---------------------------------------------------------------------
> Luke Kanies | http://reductivelabs.com | http://madstop.com
>
> _______________________________________________
> Puppet-users mailing list
> Puppet-users@madstop.com
> https://mail.madstop.com/mailman/listinfo/puppet-users
>

_______________________________________________
Puppet-users mailing list
Puppet-users@madstop.com
https://mail.madstop.com/mailman/listinfo/puppet-users

Never mind, just realized these are patches against a puppet repo from best
practices and not against the puppet code base.

Thanks
Brian

On 2/16/08, Brian Finney <bri@nfinney.com> wrote:
>
> Wouldn''t it be better to send patches and such to Puppet-dev
rather than
> Puppet-users?
>
> Thanks
> Brian
>
> On 2/16/08, Luke Kanies <luke@madstop.com> wrote:
> >
> > On Feb 16, 2008, at 3:06 AM, David Schmitt wrote:
> >
> > > Hi all!
> > >
> > > Having asked anarcat to send the patches to the list,
I''m obviously in
> > > favor of having such patches out in the open, where they can be
> > > discussed by the public, instead of buried in my mailbox, where
they
> > > may
> > > "rot in peace".
> > >
> > > I have to admit though, that this is all work in progress, so any
and
> > > all input is very welcome. Currently there are one or two sets of
> > > patches every month, so I hope it won''t be too much of a
burden on the
> > > list and IMHO only if this traffic increased significantly, a
seperate
> > > mailing list should be created.
> >
> >
> > Okay, we''ll start with this and see how it goes.
> >
> > --
> > Men never do evil so completely and cheerfully as when they do it from
a
> > religious conviction. --Blaise Pascal
> > ---------------------------------------------------------------------
> > Luke Kanies | http://reductivelabs.com | http://madstop.com
> >
> > _______________________________________________
> > Puppet-users mailing list
> > Puppet-users@madstop.com
> > https://mail.madstop.com/mailman/listinfo/puppet-users
> >
>
>

_______________________________________________
Puppet-users mailing list
Puppet-users@madstop.com
https://mail.madstop.com/mailman/listinfo/puppet-users

On Sat, Feb 16, 2008 at 10:06:46AM +0100, David Schmitt wrote:
[...]
> Back to the meat:
> 
> anarcat schrieb:
> > only works with LVM for now
> > ---
> >  files/xen-tools.conf |  184
++++++++++++++++++++++++++++++++++++++++++++++++++
> >  manifests/xen.pp     |   58 +++++++++++++++-
> >  2 files changed, 241 insertions(+), 1 deletions(-)
> >  create mode 100644 files/xen-tools.conf
> > 
> > diff --git a/files/xen-tools.conf b/files/xen-tools.conf
> > new file mode 100644
> > index 0000000..e766551
> > --- /dev/null
> > +++ b/files/xen-tools.conf
> > @@ -0,0 +1,184 @@
> 
> > diff --git a/manifests/xen.pp b/manifests/xen.pp
> > index 613c15a..935adf2 100644
> > --- a/manifests/xen.pp
> > +++ b/manifests/xen.pp
> > @@ -49,8 +49,64 @@ class xen::domain::xen0 inherits xen::domain {
> >  		[
> >  	        "xen-hypervisor-3.0.3-1-$architecture",
> >  		"linux-image-xen-$architecture",
> > -		''libsysfs2''
> > +		''libsysfs2'',
> > +		''xen-tools''
> >  		]:
> >  		ensure => present
> >  	}
> > +	file { "/etc/xen-tools/xen-tools.conf":
> > +		source =>
"puppet://$servername/virtual/xen-tools.conf",
> > +		mode => 0644, owner => anarcat, group => root,
> 
> "owner => anarcat" is no good. Also, please use config_file
for this, it
> would have to right defaults then.
Wow. Bad case of pattern replacement... I was having problems with git
and ran a s/root/anarcat/ on the patch to change the From... :( That was
a long day, this should of course read as "owner => root" and does
so
here...

As for config_file, I try to keep my code independant from the
"common"
module as much as possible, but I guess it''s a bit stupid here since
the
rest of the "virtual" module does use "common"...
> > +		require => Package[''xen-tools''];
> > +	}
> > +}
> > +
> > +# XXX: we have a logic issue here: if xen::domain is a class applied
on a node,
> > +# then a xen::domain::xenU would also be applied on a node, but then
it
> > +# couldn''t be used to create the node in the first place
since the node
> > +# wouldn''t exist...
> > +# chicken and egg. we therefore don''t define xenU as a class
but as a define
> > +# that is applied on the parent xen0
> 
> Yeah, I had the same problem with vservers and solved it the same: There 
> is a "vserver" define which is applied in the host to create the
guests.
Okay.
> > +# ensure: present
> > +# TODO: running, stopped, absent... (in that order?)
> 
> You already have implemented "absent".
Yes, indeed, that comment needs to be fixed. I even made a crude
"running" implementation.
> Does xen-create-image take care about starting the image too?
No, for that you need "xm create <foo>".
> IIRC the domU config has to be manually placed into /etc/xen/auto/ or
> something to do this.
You''re right, I forgot about that.
> Anyways, I would recommend following the Service model and split it into 
> "enable => true/false" - whether the domU should be started on
boot -
> and "ensure => running/stopped"  - whether puppet should take
care of
> starting the domU if it is down.
Indeed, that''s a good idea.
> > +# TODO: resize LVM when $size changes?
> 
> lvresize would allow you that easily with "--size $size", but I
would
> either advise against it ("Be careful when reducing a logical
volume''s
> size,  because  data  in  the  reduced  part  is lost!!!" -- 
> lvresize(1)) or at least do a very thorough check, that the new size is 
> bigger than the old.
I am aware of that limitation... I just put that todo there to keep that
in mind. I guess resizing would be done to grow the size, not to
shrink... Anyways, it''s a really sensitive operation, so I''m
not sure
I''ll really work on that feature.
> > +define xen::domain::xenU($ensure, $memory, $size, $swapsize = 0,
$role = "minimal", $vg = "vg00",
> > +			 $ip = "", $netmask = "255.255.255.0",
$gateway = "", $mac = "") {
> > +    case $name { '''': { fail ( "Cannot create
Xen domain with empty name" ) } }
> > +    case $swapsize {
> > +        0: {
> > +	    $swap = "--noswap"
> > +	}
> > +	default: {
> > +	    $swap = "--swap $swapsize"
> > +	}
> > +    }
> > +    case $gatway { "": { $gateway_flag = "" }
default: { $gateway_flag = "--gateway $gateway" } }
> > +    case $mac { "": { $mac_flag = "" } default: {
$mac_flag = "--mac $mac" } }
> > +    case $ip {
> > +    	"": {
> > +	    $network = "--dhcp $mac_flag"
> > +	}
> > +	default: {
> > +	    $network = "--ip $ip --netmask $netmask $gateway_flag
$mac_flag"
> > +	}
> > +    }
> 
> Please take a look at the Style Guide [1] regarding the formatting of 
> case statements. Personally I would set $..._real or _flag variables 
> with the ?: operator instead of using case statements, where applicable.
 
Right, okay.

So I''ve got a lot of work to do to fix those patch... Should I just
--amend them? It seems unlikely since i would have to "pop" 3 patches
from the "stack" to amend this one (for example). Can I just send you
new patches?

Or maybe I should just get it over with and publish my own git repo.. :(
But I feel that this would go against the wish to centralise development
in a single location (ie. your repo for now).

A.

-- 
Man really attains the state of complete humanity when he produces,
without being forced by physical need to sell himself as a commodity.
			- Ernesto "Che" Guevara


_______________________________________________
Puppet-users mailing list
Puppet-users@madstop.com
https://mail.madstop.com/mailman/listinfo/puppet-users

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tuesday 19 February 2008, anarcat wrote:
> As for config_file, I try to keep my code independant from the
"common"
> module as much as possible, but I guess it''s a bit stupid here
since the
> rest of the "virtual" module does use "common"...
Exactly. I probably would change this locally before publishing if the patch 
wouldn''t have it, just for consistency, but I''d prefer it if
the patch had it
already. Not the least, because it gives the user a "well-defined"
point to
modify standard behaviour.
> > > +# TODO: resize LVM when $size changes?
> >
> > lvresize would allow you that easily with "--size $size",
but I would
> > either advise against it ("Be careful when reducing a logical
volume''s
> > size,  because  data  in  the  reduced  part  is lost!!!" --
> > lvresize(1)) or at least do a very thorough check, that the new size
is
> > bigger than the old.
>
> I am aware of that limitation... I just put that todo there to keep that
> in mind. I guess resizing would be done to grow the size, not to
> shrink... Anyways, it''s a really sensitive operation, so
I''m not sure
> I''ll really work on that feature.
Since this would also need growing the filesystem I''d say to skip it
and let
this be a manual step. To make this clear, you should think about renaming 
$size to $initial_size or something.
> So I''ve got a lot of work to do to fix those patch... Should I
just
> --amend them? It seems unlikely since i would have to "pop" 3
patches
> from the "stack" to amend this one (for example). Can I just send
you
> new patches?
I haven''t applied any of them yet, so you basically may do anything
with them
as you wish. Of course for a clean history (and better possibility to review 
them it''d be great to have all changes for a given topic in one commit.

A simple way to do so would be to re-clone from my HEAD and manually apply one 
of the original patches, add your changes and then commit again. This would 
lead to a clean tree with all the "dirty" development history erased.
> Or maybe I should just get it over with and publish my own git repo.. :(
> But I feel that this would go against the wish to centralise development
> in a single location (ie. your repo for now).
No worries about that... the (my) wish is to centralise publication, not 
development. And as we all know, in the end this should be 
centralised/published on modules.reductivelabs.com, not on my site.




Regards, DavidS


- -- 
The primary freedom of open source is not the freedom from cost, but the free-
dom to shape software to do what you want. This freedom is /never/ exercised
without cost, but is available /at all/ only by accepting the very different
costs associated with open source, costs not in money, but in time and effort.
- -- http://www.schierer.org/~luke/log/20070710-1129/on-forks-and-forking
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFHu/QM/Pp1N6Uzh0URAqoBAKCb7i5MR27Rz/JrcogwSCPj/fncPQCgmoIF
hHn++iLefg/we8OvB7ebLcM=3xuV
-----END PGP SIGNATURE-----