Add support for thin provisioning and setting poolmetadatasize #154

afalko · 2016-06-17T02:08:55Z

No description provided.

hunner · 2016-08-25T17:14:00Z

lib/puppet/provider/logical_volume/lvm.rb

@@ -110,7 +115,14 @@ def create
            args.push('--type', @resource[:type])
        end

-        args << @resource[:volume_group]
+	if @resource[:thin]


if @resource[:thin] is :false or 'false' then this will be truthy. The type should either munge or only accept literal true and false values.

This commit fixes the comments raised in PR/154 and adds testing and documentation; * Inherit the thin attribute from Puppet::Parameter::Boolean * Conditionally add -n flag to provider args rather than drop later * Fixed hard tab spaces in provider * Added documentation for new attributes * Added rspec tests

crayfishx · 2016-09-20T15:15:43Z

@afalko : I've addressed the point made by @hunner , made some other tweaks and added tests and documentation. I've raised this as a PR against your branch.... afalko#1

... yep, a PR to a PR, github is sweet.

@afalko Can you review and merge the above PR and then it should make it through to this one - we are keen to see this PR merged.

Thanks!

Additional fixes and testing/docs

crayfishx

This looks good to me.... @hunner ?

crayfishx · 2016-09-21T11:25:49Z

Before merging this, @afalko can you take a look at afalko#2 - I think we need this otherwise there is no way of configuring thinpools from hiera...etc.

vinzent

and I think for --thin you cant pass --extents or -L and you are required to pass --virtualsize

vinzent · 2016-09-21T11:31:32Z

lib/puppet/type/logical_volume.rb

+  end
+
+  newparam(:poolmetadatasize) do
+    desc "Change the size of logical volume pool metadata"


it is only used in the creat method. so it is not possible to change it.

Good point - lvresize does support this so it can be added easily enough

Summary from IRC discussion: This is better documented and changed to a property in the future.

vinzent · 2016-09-21T11:34:34Z

lib/puppet/provider/logical_volume/lvm.rb

-        args << @resource[:volume_group]
+        if @resource[:thin]
+            args.push('--thin')
+            args << @resource[:volume_group] + "/" + @resource[:name]


according lvcreate --help the syntax is lvceate ... --thin VolumeGroupName/PoolLogicalVolume the :name here is IMO the name of the lv to be created not the pool lv.

the -n (--name) is IMO still required and should be set to :name

I'm not sure about this..... I'll test it

as you quoted from the man page. --thin seems to have multiple outcomes. but there is --thinpool too. for me it seems like --thinpool was introduced later to be specific what happens.

vinzent · 2016-09-21T11:35:22Z

lib/puppet/type/logical_volume.rb

+    desc "Change the size of logical volume pool metadata"
+    validate do |value|
+      unless value =~ /^[0-9]+(\.[0-9]+)?[KMGTPE]/i
+        raise ArgumentError , "#{value} is not a valid logical volume size"


wrong error text

I'll change that, well spotted.

Changed in afalko#2

crayfishx · 2016-09-21T11:44:36Z

@vinzent your first point about --extents, you can use it, in fact that's how I tested it.... from the lvcreate manpage....

Specifying the optional argument --size or --extents will cause the creation of the thin pool logical volume. Specifying the optional argument --virtualsize will cause the creation of the thin logical volume from given thin pool volume. Specifying both arguments will cause the creation of both thin pool and thin volume using this pool.

vinzent · 2016-09-21T13:29:04Z

related: https://tickets.puppetlabs.com/browse/MODULES-2888

crayfishx · 2016-09-21T13:50:29Z

@afalko @vinzent FYI

@vinzent and I had a long discussion on this over IRC - I'll summarize here...

The terminology and documentation around thin pools and thin pool volumes (which are different things but both are logical volumes) is confusing. This PR enables the creation of thin pools. What it doesn't do is allow you to create thin pool volumes inside this thin pool. For that reason, and after a discussion about some future work that will be raised in separate tickets, we've changed the attribute thin to thinpool to imply the creation of a thin pool.

So currently, to create a thin pool you declare

# Create a thin pool
logical_volume { 'blah':
  volume_group => 'foo',
  thinpool => true,
}

The logic behind this is that we would (in a future PR) introduce the thin parameter to add functionality to create thin volumes within this pool, something like;

# Create a thin pool *volume*
logical_volume { 'mythinvol':
  thin => true,
  pool => 'blah',
}

I think I've summarized that properly!

The changes reflecting this are in a PR to this PR (afalko#2) - if you are happy with the above @afalko could you please merge that into this PR?

We'll create issues separate to this PR capturing:

Creation of thin pool volumes
Changing poolmetadatasize into a configurable property rather than a parameter

Additional changes...

afalko · 2016-09-21T17:28:03Z

@vinzent @crayfishx : I've resolved the merge conflicts.

crayfishx · 2016-09-22T07:01:14Z

Oops - minor mistake caught by travis - @afalko if you can update this PR with afalko#3 we should be good!

hunner · 2016-09-28T10:31:29Z

lib/puppet/type/logical_volume.rb

@@ -1,3 +1,5 @@
+require 'puppet/parameter/boolean'


This was added in puppet 3.3.0 so the version in metadata should be bumped.

hunner · 2016-09-28T10:31:41Z

README.md

 * readahead (Parameter) - The readahead count to use for the new logical volume.
 * region_size (Parameter) - A mirror is divided into regions of this size (in MB), the mirror log uses this granularity to track which regions are in sync. CAN NOT BE CHANGED on already mirrored volume. Take your mirror size in terabytes and round up that number to the next power of 2, using that number as the -R argument.
 * size (Property) - The size of the logical volume. Set to undef to use all available space
 * size_is_minsize (Parameter) Default value: `:false` - Set to true if the ‘size’ parameter specified, is just the minimum size you need (if the LV found is larger then the size requests this is just logged not causing a FAIL)
 * stripes (Parameter) - The number of stripes to allocate for the new logical volume.
 * stripesize (Parameter) - The stripesize to use for the new logical volume.
+* thinpool (Parameter) - Default value: `:false` - Set to true to create a thin pool


s/:false/false/

hunner · 2016-09-28T10:44:22Z

@crayfishx will fix in follow-up PR

unixsanet · 2017-01-04T20:09:08Z

Just updated my environment to use LVM-0.8.0 and it appears to be incompatible with Satellite 6.1 and using Smart Class Parameter. This code worked with 0.7.0:

oraclevg:
  physical_volumes:
  - /dev/sdb
  logical_volumes:
    lvd01:
      size: 24.99G
      mountpath: /d01

but fails with 0.8.0 with the following error:

Error: Could not retrieve catalog from remote server: Error 400 on SERVER: Invalid parameter thinpool on Logical_volume[lvd01] at /etc/puppet/environments/production/modules/lvm/manifests/logical_volume.pp:99

vinzent · 2017-01-04T20:23:39Z

@unixsanet you're probably facing the longstanding environment isolation problem with ruby types. See https://docs.puppet.com/puppet/latest/environment_isolation.html . If you have multiple environments with different versions of the same ruby types you'll can get into troubles like this.

walkamongus · 2017-01-04T23:07:34Z

@unixsanet I ran into the exact same issue and a restart of puppetserver on the affected master resolved it.

…tlabs#154) * Add support for thin provisioning and setting poolmetadatasize * Additional fixes and testing/docs This commit fixes the comments raised in PR/154 and adds testing and documentation; * Inherit the thin attribute from Puppet::Parameter::Boolean * Conditionally add -n flag to provider args rather than drop later * Fixed hard tab spaces in provider * Added documentation for new attributes * Added rspec tests * added thin and poolmetadatasize to lvm::volume_group defined type * Changed thin attribute to thinpool * changed error message for poolmetadatasize validation * Document that poolmetadatasize currently only sets on creation * changed $thin to $thinpool, ommitted after attribute rename (#3)

tenajsystems · 2017-12-28T16:57:12Z

I was following this PR and was not sure if the PR for creating thin pool volumes(thin-provisioned lv) like the example @crayfishx gave like below or #159. I do know that creating a thin pool was implemented in the PR but want to know if thin pool volumes was also implemented. Could someone please confirm this for me?

# Create a thin pool
logical_volume { 'blah':
volume_group => 'foo',
thinpool => true,
}

# Create a thin pool *volume*
logical_volume { 'mythinvol':
thin => true,
pool => 'blah',
}

Thanks.

tenajsystems · 2018-01-08T21:33:31Z

It appears the thinpool => true, doesn't work. I have setup the below and manually trying to create a thin LV using the command lvcreate -V 50GB -T vg-gluster/data -n Test and I get this message: Logical volume vg-gluster/data is not a thin pool

# Create a thin pool
logical_volume { 'blah':
volume_group => 'foo',
thinpool => true,
}

What I'm I doing wrong?

Add support for thin provisioning and setting poolmetadatasize

bc14321

hunner reviewed Aug 25, 2016
View reviewed changes

hunner added needs-tests needs-docs labels Aug 25, 2016

Merge pull request #1 from crayfishx/pr/154

b13e71c

Additional fixes and testing/docs

crayfishx approved these changes Sep 20, 2016

View reviewed changes

vinzent mentioned this pull request Sep 21, 2016

(MODULES-2888) Add thinpool support #159

Closed

added thin and poolmetadatasize to lvm::volume_group defined type

a5b4be8

vinzent reviewed Sep 21, 2016

View reviewed changes

crayfishx added 3 commits September 21, 2016 14:55

Changed thin attribute to thinpool

1ba7edb

changed error message for poolmetadatasize validation

edcf439

Document that poolmetadatasize currently only sets on creation

20fd060

afalko and others added 2 commits September 21, 2016 10:00

Merge pull request #2 from crayfishx/pr/154_definedtype

0c83b44

Additional changes...

Merge remote-tracking branch 'origin/master' into HEAD

95dd331

changed $thin to $thinpool, ommitted after attribute rename (#3)

595ac02

hunner suggested changes Sep 28, 2016

View reviewed changes

hunner merged commit 8bb29fd into puppetlabs:master Sep 28, 2016

jordanbreen28 added feature and removed needs-tests needs-docs labels May 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for thin provisioning and setting poolmetadatasize #154

Add support for thin provisioning and setting poolmetadatasize #154

afalko commented Jun 17, 2016

hunner Aug 25, 2016

crayfishx commented Sep 20, 2016

crayfishx left a comment

crayfishx commented Sep 21, 2016

vinzent left a comment

vinzent Sep 21, 2016

crayfishx Sep 21, 2016

crayfishx Sep 21, 2016

vinzent Sep 21, 2016

crayfishx Sep 21, 2016

vinzent Sep 21, 2016

vinzent Sep 21, 2016

crayfishx Sep 21, 2016

crayfishx Sep 21, 2016

crayfishx commented Sep 21, 2016

vinzent commented Sep 21, 2016

crayfishx commented Sep 21, 2016

afalko commented Sep 21, 2016

crayfishx commented Sep 22, 2016

hunner Sep 28, 2016

hunner Sep 28, 2016

hunner commented Sep 28, 2016

unixsanet commented Jan 4, 2017

vinzent commented Jan 4, 2017 •

edited

walkamongus commented Jan 4, 2017

tenajsystems commented Dec 28, 2017

tenajsystems commented Jan 8, 2018

Add support for thin provisioning and setting poolmetadatasize #154

Add support for thin provisioning and setting poolmetadatasize #154

Conversation

afalko commented Jun 17, 2016

Choose a reason for hiding this comment

crayfishx commented Sep 20, 2016

crayfishx left a comment

Choose a reason for hiding this comment

crayfishx commented Sep 21, 2016

vinzent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crayfishx commented Sep 21, 2016

vinzent commented Sep 21, 2016

crayfishx commented Sep 21, 2016

afalko commented Sep 21, 2016

crayfishx commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunner commented Sep 28, 2016

unixsanet commented Jan 4, 2017

vinzent commented Jan 4, 2017 • edited

walkamongus commented Jan 4, 2017

tenajsystems commented Dec 28, 2017

tenajsystems commented Jan 8, 2018

vinzent commented Jan 4, 2017 •

edited