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 #2314 - fine grain for RAM selectors #3774

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions app/assets/javascripts/application.js
Expand Up @@ -2,6 +2,7 @@
//= require turbolinks
//= require i18n
//= require jquery.ui.autocomplete
//= require jquery.ui.spinner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use jquery.ui.spinner instead of patternfly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patternfly spinner didn't allow me to customize the output and parsing - note that gb/mb is part of the input so user can type "5gb" and it will get parsed correctly

//= require scoped_search
//= require bootstrap
//= require multi-select
Expand Down
1 change: 1 addition & 0 deletions app/assets/javascripts/host_edit.js
Expand Up @@ -76,6 +76,7 @@ function computeResourceSelected(item){
activate_select2('#compute_resource');
if ($('#compute_resource').find('.alert-danger').length > 0) $('#compute_resource_tab a').addClass('tab-error');
update_capabilities($('#capabilities').val());
tfm.initByteSpinner();
}
})
}
Expand Down
2 changes: 2 additions & 0 deletions app/assets/stylesheets/application.scss
Expand Up @@ -3,6 +3,8 @@
@import "jquery.ui.menu";
//we need to import all the above because we are using jquery-ui-rails and it uses sprockets require instaed of sass import.
@import "jquery.ui.autocomplete";
@import "jquery.ui.spinner";
@import "jquery.ui.spinner_custom";
@import "../../../vendor/assets/stylesheets/vendor";
@import "scoped_search";
@import "multi-select";
Expand Down
21 changes: 21 additions & 0 deletions app/assets/stylesheets/jquery.ui.spinner_custom.scss
@@ -0,0 +1,21 @@
span.ui-spinner {
width: 100%;
border: none;
border-bottom-right-radius: 0;
border-bottom-left-radius: 0;
border-top-right-radius: 0;
border-top-left-radius: 0;
}

input.ui-spinner-input {
margin: 0;
}

.ui-spinner a.ui-spinner-button {
border-radius: 0;
border: 1px solid #bababa;
background-color: #eee;
&.ui-spinner-up {
border-bottom: none;
}
}
5 changes: 0 additions & 5 deletions app/helpers/compute_resources_helper.rb
Expand Up @@ -34,11 +34,6 @@ def vm_pause_action(vm, authorizer = nil)
display_link_if_authorized pause_action, opts, html.merge(:method => :put)
end

def memory_options(max_memory)
opts = [0.25, 0.5, 0.75, 1, 2, 3, 4, 6, 8, 12, 16, 24, 32]
opts.map{|n| [number_to_human_size(n*Foreman::SIZE[:giga]), (n*Foreman::SIZE[:giga]).to_i] unless n > (max_memory / Foreman::SIZE[:giga])}.compact
end

def password_placeholder(obj, attr = nil)
pass = obj.read_attribute(attr).present? || obj.read_attribute(:password_hash).present?
pass ? "********" : ''
Expand Down
12 changes: 12 additions & 0 deletions app/helpers/form_helper.rb
Expand Up @@ -192,6 +192,18 @@ def autocomplete_f(f, attr, options = {})
end
end

def byte_size_f(f, attr, options = {})
options[:class] = options[:class].to_s + ' byte_spinner'
options[:help_inline] ||= popover('', _("When specifying custom value, add 'MB' or 'GB' at the end. Field is not case sensitive and MB is default if unspecified."))
options[:help_block] ||= content_tag(:span, :class => 'maximum-limit hidden') do
content_tag(:i, '', :class => 'pficon-warning-triangle-o') +
content_tag(:span, ' ' + _('Specified value is higher than recommended maximum'), :class => 'error-message')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be validated by the server?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, the thing is that that many CR hardcode some maximum which should not limit the users if they know what they're doing. Later I'd like to remove artificial limits but that needs more CR code refactoring first. So now we can just display a warning.

end
options[:help_block] += f.hidden_field(attr, :class => "real-hidden-value", :id => nil)

text_f(f, attr, options)
end

def form_to_submit_id(f)
object = f.object.respond_to?(:to_model) ? f.object.to_model : f.object
key = if object.present?
Expand Down
9 changes: 5 additions & 4 deletions app/models/compute_resources/foreman/model/libvirt.rb
Expand Up @@ -58,12 +58,13 @@ def max_cpu_count
hypervisor.cpus
end

# libvirt reports in KB
# returns available memory for VM in bytes
def max_memory
hypervisor.memory * Foreman::SIZE[:kilo]
# libvirt reports in KB
hypervisor.memory.kilobyte
rescue => e
logger.debug "unable to figure out free memory, guessing instead due to:#{e}"
16*Foreman::SIZE[:giga]
16.gigabytes
end

def test_connection(options = {})
Expand Down Expand Up @@ -207,7 +208,7 @@ def disconnect

def vm_instance_defaults
super.merge(
:memory => 768*Foreman::SIZE[:mega],
:memory => 768.megabytes,
:nics => [new_nic],
:volumes => [new_volume],
:display => { :type => display_type,
Expand Down
2 changes: 1 addition & 1 deletion app/models/compute_resources/foreman/model/ovirt.rb
Expand Up @@ -99,7 +99,7 @@ def max_cpu_count
end

def max_memory
16*Foreman::SIZE[:giga]
16.gigabytes
end

def ovirt_quota=(ovirt_quota_id)
Expand Down
2 changes: 1 addition & 1 deletion app/models/compute_resources/foreman/model/vmware.rb
Expand Up @@ -45,7 +45,7 @@ def max_cpu_count(cluster = nil)
end

def max_memory
16*Foreman::SIZE[:giga]
16.gigabytes
end

def datacenters
Expand Down
4 changes: 2 additions & 2 deletions app/models/concerns/fog_extensions/libvirt/server.rb
Expand Up @@ -17,11 +17,11 @@ def volumes_attributes=(attrs); end

# Libvirt expect units in KB, while we use bytes
def memory
attributes[:memory_size].to_i * Foreman::SIZE[:kilo]
attributes[:memory_size].to_i.kilobytes
end

def memory=(mem)
attributes[:memory_size] = mem.to_i / Foreman::SIZE[:kilo] if mem
attributes[:memory_size] = mem.to_i / 1.kilobyte if mem
end

def reset
Expand Down
2 changes: 1 addition & 1 deletion app/services/fog_extensions/vsphere/mini_server.rb
Expand Up @@ -10,7 +10,7 @@ def initialize(raw, path = nil, uuid = nil)
@identity = uuid
@cpus = hardware.numCPU
@corespersocket = hardware.numCoresPerSocket
@memory = hardware.memoryMB * Foreman::SIZE[:mega]
@memory = hardware.memoryMB.megabytes
@state = raw.runtime.powerState
@path = path
end
Expand Down
4 changes: 3 additions & 1 deletion app/views/compute_resources_vms/form/libvirt/_base.html.erb
@@ -1,8 +1,10 @@
<%= javascript_tag("$(document).on('ContentLoad', tfm.initByteSpinner)"); %>

<%= text_f f, :name, :label => _('Name'), :label_size => "col-md-2", :disabled => !new_host if show_vm_name? %>

<%= selectable_f f, :cpus, 1..compute_resource.max_cpu_count, { }, :disabled => !new_host, :label => _('CPUs'), :label_size => "col-md-2" %>
<%= selectable_f f, :memory, memory_options(compute_resource.max_memory), { }, :disabled => !new_host, :label => _('Memory'), :label_size => "col-md-2" %>

<%= byte_size_f f, :memory, :disabled => !new_host, :label => _('Memory'), :label_size => "col-md-2", :'data-soft-max' => compute_resource.max_memory %>

<!--TODO # Move to a helper-->
<% checked = params[:host] && params[:host][:compute_attributes] && params[:host][:compute_attributes][:start] || '1' %>
Expand Down
3 changes: 2 additions & 1 deletion app/views/compute_resources_vms/form/ovirt/_base.html.erb
@@ -1,3 +1,4 @@
<%= javascript_tag("$(document).on('ContentLoad', tfm.initByteSpinner)"); %>
<% javascript 'compute_resource' %>
<%= text_f f, :name, :label => _('Name'), :label_size => "col-md-3" if show_vm_name? %>
<% clusters = compute_resource.clusters %>
Expand All @@ -18,7 +19,7 @@
<% selected_cluster ||= params[:host] && params[:host][:compute_attributes] && params[:host][:compute_attributes][:cluster] %>

<%= selectable_f f, :cores, 1..compute_resource.max_cpu_count, { }, :class => "col-md-2", :label => _('Cores'), :label_size => "col-md-2" %>
<%= selectable_f f, :memory, memory_options(compute_resource.max_memory), { }, :class => "col-md-2", :label => _('Memory'), :label_size => "col-md-2" %>
<%= byte_size_f f, :memory, :disabled => !new_host, :label => _('Memory'), :label_size => "col-md-2", :'data-soft-max' => compute_resource.max_memory %>

<% checked = params[:host] && params[:host][:compute_attributes] && params[:host][:compute_attributes][:start] || '1' %>
<%= checkbox_f f, :start, { :checked => (checked == '1'), :help_inline => _("Power ON this machine"), :label => _('Start'), :label_size => "col-md-2" } if new_host && controller_name != "compute_attributes" %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/compute_resources_vms/index/_libvirt.html.erb
Expand Up @@ -13,7 +13,7 @@
<td><%= link_to_if_authorized vm.name, hash_for_compute_resource_vm_path(:compute_resource_id => @compute_resource, :id => vm.uuid).merge(:auth_object => @compute_resource, :auth_action => 'view', :authorizer => authorizer) %></td>
<td><%= vm.cpus %></td>
<td>
<%= number_to_human_size vm.memory_size*Foreman::SIZE[:kilo] %>
<%= number_to_human_size vm.memory_size.kilobytes %>
</td>
<td>
<span <%= vm_power_class(vm.ready?) %>>
Expand Down
4 changes: 2 additions & 2 deletions app/views/compute_resources_vms/show/_libvirt.html.erb
Expand Up @@ -26,8 +26,8 @@
</tr>
<tr>
<td><%= _('Memory') %></td>
<td><%= number_to_human_size @vm.max_memory_size*Foreman::SIZE[:kilo] %>
<%= "(" + _("Allocated") + ": #{number_to_human_size @vm.memory_size*Foreman::SIZE[:kilo]})" %></td>
<td><%= number_to_human_size @vm.max_memory_size.kilobytes %>
<%= "(" + _("Allocated") + ": #{number_to_human_size @vm.memory_size.kilobytes})" %></td>
</tr>
<tr>
<td><%= _('Display') %></td>
Expand Down
6 changes: 3 additions & 3 deletions lib/core_extensions.rb
Expand Up @@ -144,10 +144,10 @@ def to_gb
value, _, unit=self.match(/(\d+(\.\d+)?) ?(([KMGT]i?B?|B))$/i)[1..3]
case unit.to_sym
when nil, :B, :byte then (value.to_f / Foreman::SIZE[:giga])
when :TB, :TiB, :T, :terabyte then (value.to_f * Foreman::SIZE[:kilo])
when :TB, :TiB, :T, :terabyte then (value.to_f * 1.kilobyte)
when :GB, :GiB, :G, :gigabyte then value.to_f
when :MB, :MiB, :M, :megabyte then (value.to_f / Foreman::SIZE[:kilo])
when :KB, :KiB, :K, :kilobyte, :kB then (value.to_f / Foreman::SIZE[:mega])
when :MB, :MiB, :M, :megabyte then (value.to_f / 1.kilobyte)
when :KB, :KiB, :K, :kilobyte, :kB then (value.to_f / 1.megabyte)
else raise "Unknown unit: #{unit.inspect}!"
end
rescue
Expand Down
2 changes: 0 additions & 2 deletions lib/foreman.rb
Expand Up @@ -17,6 +17,4 @@ def self.in_rake?(rake_task = nil)
rake_task.nil? || running_rake_task.start_with?(rake_task)
end
end

SIZE = {:kilo => 1_024, :mega => 1_048_576, :giga => 1_073_741_824, :tera => 1_099_511_627_776}
end
4 changes: 2 additions & 2 deletions test/functional/compute_resources_vms_controller_test.rb
Expand Up @@ -63,7 +63,7 @@ def setup_user(operation, type = 'compute_resources_vms')
test "should not create compute resource when not permitted" do
setup_user "view"
assert_difference('@compute_resource.vms.count', 0) do
attrs = {:name => 'name123', :memory => 128*Foreman::SIZE[:mega], :arch => "i686"}
attrs = {:name => 'name123', :memory => 128.megabytes, :arch => "i686"}
post :create, {:vm => attrs, :compute_resource_id => @compute_resource.to_param}, set_session_user
end
assert_response :forbidden
Expand All @@ -76,7 +76,7 @@ def test_should_create_vm(name = "new_test")
user.roles.last.add_permissions! :view_compute_resources
end
assert_difference('@compute_resource.vms.count', +1) do
attrs = {:name => name, :memory => 128*Foreman::SIZE[:mega], :domain_type => "test", :arch => "i686"}
attrs = {:name => name, :memory => 128.megabytes, :domain_type => "test", :arch => "i686"}
post :create, {:vm => attrs, :compute_resource_id => @compute_resource.to_param}, set_session_user
end
assert_redirected_to compute_resource_vms_path
Expand Down
3 changes: 2 additions & 1 deletion webpack/assets/javascripts/bundle.js
Expand Up @@ -7,5 +7,6 @@ require('./bundle_flot');
require('./bundle_select2');
require('./bundle_datatables');
window.tfm = {
tools: require('./foreman_tools')
tools: require('./foreman_tools'),
initByteSpinner: require('./jquery.ui.custom_spinners').initByteSpinner
};
114 changes: 114 additions & 0 deletions webpack/assets/javascripts/jquery.ui.custom_spinners.js
@@ -0,0 +1,114 @@
const megabyte = 1024 * 1024;
const gigabyte = 1024 * megabyte;

$(function () {
$.widget('ui.limitedSpinner', $.ui.spinner, {
options: {
softMaximum: 0,
errorTarget: null
},
validate: function () {
return this._validate();
},
_validate: function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to trigger on manual changes of the field (ie not using the spinner)

if (this.options.softMaximum !== 0) {
this.options.errorTarget.toggle(this.value() > this.options.softMaximum);
}
},
_spin: function (step, event) {
let result = this._super(step, event);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected blank line after variable declarations newline-after-var

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to save this in a variable instead of just return this._super(step, event); after running the validation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep the interface of method I'm overriding


this._validate();
return result;
}
});

$.widget('ui.byteSpinner', $.ui.limitedSpinner, {
options: {
step: 1,
min: 1,
incremental: false,
valueTarget: null
},
updateValueTarget: function () {
this.options.valueTarget.val(this.value());
},
_gigabyteSpin: function (step) {
if (step > 0) {
if (step % gigabyte === 0) {
step = gigabyte;
} else {
step = gigabyte - (this.value() % gigabyte);
}
} else if (step < 0) {
if (this.value() % gigabyte === 0) {
step = -1 * gigabyte;
} else {
step = -1 * (this.value() % gigabyte);
}
}
return step;
},
_megabyteSpin: function (step) {
const megabyteStep = step * 256 * megabyte;

if (this.value() + megabyteStep > gigabyte) {
step = gigabyte - this.value();
} else if (this.value() + megabyteStep < megabyte) {
step = step * (this.value() - megabyte);
} else if (this.value() === megabyte && step > 0) {
step = 255 * megabyte;
} else {
step = megabyteStep;
}
return step;
},
_spin: function (step, event) {
let result = null;

if ((this.value() > gigabyte && step < 0) ||
(this.value() >= gigabyte && step > 0)) {
step = this._gigabyteSpin(step);
} else {
step = this._megabyteSpin(step);
}

result = this._super(step, event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, any reason to save this rather then directly returning it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's one more call after this but I want to keep the same interface for this function so returning whatever _super returns

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if i'm wrong, but the other call doesn't modify this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not but _spin must be called before we update the hidden which holds the real value, so we need to call super and then the update. But I want to keep method interface therefore returning the result of super call.

this.updateValueTarget();

return result;
},
_parse: function (value) {
if (typeof value === 'string') {
if (value.match(/gb$/i)) {
return parseFloat(value) * gigabyte;
} else if (value.match(/mb$/i) || parseInt(value, 10) < megabyte) {
return parseInt(value, 10) * megabyte;
}
}
return value;
},
// prints value with unit, if it's multiple of gigabytes use GB, otherwise format in MB
_format: value =>
(value % gigabyte === 0) ? (value / gigabyte) + ' GB' : (value / megabyte) + ' MB'
});
});

export function initByteSpinner() {
$('input.byte_spinner').each(function () {
let field = $(this);
let errorMessage = field.closest('.form-group').find('.maximum-limit');
let valueTarget = field.closest('.form-group').find('.real-hidden-value');

field.byteSpinner({
valueTarget: valueTarget,
softMaximum: field.data('softMax'),
errorTarget: errorMessage
});

field.change(function () {
field.byteSpinner('updateValueTarget');
field.byteSpinner('validate');
});
});
}