Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #5197 Preallocated disk support for ovirt #1573

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@leon-strong
Copy link
Contributor

commented Jul 8, 2014

Please excuse my github newbieness, this should replace PR #1568

@mmoll

This comment has been minimized.

Copy link
Member

commented Jul 8, 2014

You can modify existing pull requests by adding more commits to your branch. Once the review is done, you will normally be asked to "squash" your commits. This means, you use git rebase -i to melt all your commits into one and then force push your new branch. For now, however, run the [test]

@lzap

This comment has been minimized.

Copy link
Member

commented Jul 9, 2014

Ok that looks better.

Screenshot

I tested this and it works fine. Maybe instead Thick Provision the disk when creating I'd go for Uncheck for Thin Provision option

I tested this and it works for both Preallocated and Thin Provision against RHEV 3.3. Please squash into one commit.

@@ -9,6 +9,9 @@
:help_inline=> remove_child_link("X", f, { :method => :'_delete', :title => _('remove volume'), :class => 'label label-danger' }) %>
<%= f.hidden_field :storage_domain if disabled %>
<%= f.hidden_field :id %>

<%= checkbox_f f, :preallocate, { :checked => true, :help_inline => _("Thick Provision the disk when creating"), :label => _('Preallocate Disk') } %>

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 9, 2014

Contributor

Please also change the first letter all "Provision" and "Disk" to lower case, only the first should be upper case.

@leon-strong

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2014

I kinda like "Uses thin provisioning if unchecked" as suggested on IRC, any other suggestions?

@lzap

This comment has been minimized.

Copy link
Member

commented Jul 9, 2014

Hmmm we use this format for the commit messages:

fixes #5197 - Preallocated disk support for ovirt

We are good to go, Dominic will likely do it in the next merge timeframe :-) Thanks for your contribution.

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2014

[test] If you're happy, you can merge it @lzap. I still disagree somewhat with the default due to the inconsistency with other CRs, but otherwise fine.

@lzap

This comment has been minimized.

Copy link
Member

commented Jul 9, 2014

[test] If you're happy, you can merge it @lzap. I still disagree somewhat with the default due to the inconsistency with other CRs, but otherwise fine.

While thin provisioning is not a bad concept at all, it allows
over-provisioning which can (when incorrectly approached by an
administrator) lead to data loss.

Having said that, modern VM managers warn quite early to prevent from
that situation, including RHEV. I am fine with having thin option as the
default one.

Others have opinions about that?

Later,

Lukas "lzap" Zapletal
irc: lzap #theforeman

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2014

I've no opinion about thin provisioning itself, btw, but my point is purely one of consistency between Foreman's CRs. I happen to use thin provisioning with libvirt, but if it were disabled by default then I'd enable it via a profile.

@leon-strong

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2014

I've been doing some hard thinking last night, and I think dom's right for a few reasons, firstly because we're now changing an existing default behaviour, not just that, we can't be sure that the additional provisioning time that'd be introduced because of that default change won't adversely affect existing installations. (causing build timeouts etc is a real possibility).

I think the prudent thing here is to simply add the support, keep existing default behaviour, and be comfortable in the fact if users wish to change their default settings they can do it via profiles/compute templates, and we can be sure that this change won't adversely affect existing users

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2014

Thanks for revisiting that @icesmurf, much appreciated! [test]

@lzap

This comment has been minimized.

Copy link
Member

commented Jul 10, 2014

I am fine with that. Nice contribution, thanks a lot!

Later,

Lukas "lzap" Zapletal

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2014

Merged as 2bf991a for Foreman 1.6.0. Thanks for your contribution @icesmurf!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.