[merged] Let lvm create metadata volume automatically #171
Conversation
| VG_SIZE=$(vgs --noheadings --nosuffix --units s -o vg_size $VG) | ||
| META_SIZE=$(( $VG_SIZE / 1000 + 1 )) | ||
| if [ -z "$META_SIZE" ];then | ||
| Fatal "Failed to calculate size of metadata volume" |
There was a problem hiding this comment.
we don't need to do any of the above. let LVM do this for us as it will automatically create the metadata LV for us.
If we want to allow the user to specify META_SIZE in the future, then we can by simply exposing the --poolmetadatasize argument to them.
There was a problem hiding this comment.
I am doing this so that I can give very clear error message to user if we screw up calculating the size of metadata volume. I don't know what error message will lvm emit if META_SIZE="". I can try though.
There was a problem hiding this comment.
I get following from lvm if META_SIZE="".
Invalid argument for --poolmetadatasize: s
I guess not bad. Will get rid of above check.
There was a problem hiding this comment.
no, what I am saying is that we don't need any of the lines. We don't need to calculate META_SIZE at all and we don't need to pass that information in to the lvcreate command.
There was a problem hiding this comment.
@dustymabe I don't want to change existing behavior of the script until and unless we have a good reason to do so. As of now script calculates the metadata size and there is no reason to change it?
There was a problem hiding this comment.
@dustymabe We can easily change it if our calculations become a problem. We had a long conversation in the past about how big a metadata lvm should be and we did not have a good answer. So we decided on .1%. Given it seems to be working reasonably well, I don't see an incentive to change it.
Even if we have to do it, it should be done in a separate PR and this PR should not be blocked because of that.
There was a problem hiding this comment.
We had a long conversation in the past about how big a metadata lvm should be and we did not have a good answer.
Even more of a reason to let the tool decide.
Even if we have to do it, it should be done in a separate PR and this PR should not be blocked because of that.
I can accept that. if we leave it like this then we should probably add the 'if statement' back
There was a problem hiding this comment.
I am not sure why do you think that lvm will necessarily do a better job of figuring out metadata volume size better. So I don't agree with Even more of a reason to let the tool decide. You are assuming that lvm always makes good decisions on this.
Ok, I can add back META_SIZE being empty check.
There was a problem hiding this comment.
Offhand I think I agree with this:
Even if we have to do it, it should be done in a separate PR and this PR should not be blocked because of that.
There was a problem hiding this comment.
@cgwalters, yes, if we do something we should do it in a separate PR. The question is, should we do it (remove the calculation and specification of meta data lv size) or not?
rhvgoyal
force-pushed
the
data-size-100
branch
from
0d5a8ac to
0d24123
Compare
|
Got rid of META_SIZE check. Also started using |
rhvgoyal
force-pushed
the
data-size-100
branch
from
0d24123 to
7dfeba5
Compare
Right now we create lvm metadata volume first and then data volume and then pass it to lvm to convert it into a thin pool (lvconvert). This means lvm internally creates an spare logical volume (used during repair) and needs free space in volume group for that spare volume. But lvm does not tell how big an spare volume will be so caller does not know how much free space to leave in volume group. And that means if user passes in DATA_SIZE=100%FREE by the time we call lvconvert, there is no free space left and lvconvert fails. And we leave the system in uncleaned state (metadata and data volumes behind). Dusty came up with idea that why not let lvm create metadata, data and spare volume. That way docker-storage-setup does not have to deal with how much free space to leave. Also use option --poolmetadatasize option to specify size of metadata, so that we retain control on how big metadata volume should be. This should allow users to pass in DATA_SIZE as 100%FREE. Though lvm developers say it is not a good idea to fill up volume group completely and leave some space free so that later either metadata, data or spare volume can grow as need be. This patch changes behavior of MIN_DATA_SIZE. Previously we will make sure free space in VG is more than MIN_DATA_SIZE before creating data volume (and after crating metadata volume). Now we do the same check before creating metadata volume. Given metadata volume is very small (.1% of data size). This can be considered sort of round off error and should not be a real problem. Reported-by: Dusty Mabe <[email protected]> Signed-off-by: Vivek Goyal <[email protected]>
rhvgoyal
force-pushed
the
data-size-100
branch
from
7dfeba5 to
c93c2e3
Compare
|
@dustymabe Added META_SIZE check back. PTAL. |
|
@rhatdan @cgwalters PTAL |
|
☀️ Test successful - status-atomicjenkins |
rh-atomic-bot
changed the title
|
This is obviously a lot nicer! |
Fixes #169
Right now we create lvm metadata volume first and then data volume and then
pass it to lvm to convert it into a thin pool (lvconvert). This means lvm
internally creates an spare logical volume (used during repair) and needs
free space in volume group for that spare volume. But lvm does not tell
how big an spare volume will be so caller does not know how much free space
to leave in volume group. And that means if user passes in DATA_SIZE=100%FREE
by the time we call lvconvert, there is no free space left and lvconvert
fails. And we leave the system in uncleaned state (metadata and data volumes
behind).
Dusty came up with idea that why not let lvm create metadata, data and spare
volume. That way docker-storage-setup does not have to deal with how much
free space to leave. Also use option --poolmetadatasize option to specify
size of metadata, so that we retain control on how big metadata volume
should be.
This should allow users to pass in DATA_SIZE as 100%FREE. Though lvm
developers say it is not a good idea to fill up volume group completely
and leave some space free so that later either metadata, data or spare
volume can grow as need be.
This patch changes behavior of MIN_DATA_SIZE. Previously we will make sure
free space in VG is more than MIN_DATA_SIZE before creating data volume
(and after crating metadata volume). Now we do the same check before
creating metadata volume. Given metadata volume is very small (.1% of data
size). This can be considered sort of round off error and should not
be a real problem.
Reported-by: Dusty Mabe [email protected]
Signed-off-by: Vivek Goyal [email protected]