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

Use current tags for filtering resources in chargeback for VMs #17470

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/chargeback/consumption_with_rollups.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def hash_features_affecting_rate

def tag_names
@tag_names ||= @rollups.inject([]) do |memo, rollup|
memo |= rollup.tag_names.split('|') if rollup.tag_names.present?
memo |= rollup.all_tag_names
memo
end
end
Expand Down
3 changes: 2 additions & 1 deletion app/models/chargeback_vm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ def self.build_results_for_report_ChargebackVm(options)
def self.where_clause(records, options, region)
scope = records.where(:resource_type => "VmOrTemplate")
if options[:tag] && (@report_user.nil? || !@report_user.self_service?)
scope.for_tag_names(options[:tag].split("/")[2..-1])
scope_with_current_tags = scope.where(:resource => Vm.find_tagged_with(:any => @options[:tag], :ns => '*'))
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd that we hardcode VmOrTemplate on line 84 and Vm on line 86. This class should have knowledge what classes it's handling at a high level, such as a class method. I'm not sure why it's VmOrTemplate above and Vm below. Should it be VmOrTemplate.find_tagged_with ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is -- but we are charging only VMs and not VmOrTemplate. So to have VmOrTemplate here is basically useless.

But reason VmOrTemplate on line 84 is that we are not doing metric rollups for only type VM but for VmOrTemplate.

but if you think that we have to keep it more consistent I can change it to VmOrTemplate.find_tagged_with but it is not needed.

Copy link
Member

@jrafanie jrafanie May 24, 2018

Choose a reason for hiding this comment

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

@lpichler we can address these hardcoded class names in a followup. It feels like we need a method call to replace these class names:

class ChargebackVm < Chargeback
  ...
  def self.chargeable_resource
    Vm
  end
end

Then, all of these hardcoded places can call chargeable_resource or chargeable_resource.to_s or chargeable_resource.tableize depending if they need Vm, "Vm", or "vms"

Copy link
Member

Choose a reason for hiding this comment

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

Again, we don't have to do this now, it's just something I'm noticing while looking at these change...

scope.for_tag_names(options[:tag].split("/")[2..-1]).or(scope_with_current_tags)
else
scope.where(:resource => vms(region))
end
Expand Down
14 changes: 13 additions & 1 deletion app/models/metric/chargeback_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def tag_list_with_prefix
end
end

"#{image_tag_name}#{tag_names}".split("|").reject(&:empty?).map { |x| "#{tag_prefix}#{x}" } + (labels || [])
"#{image_tag_name}#{all_tag_names.join("|")}".split("|").reject(&:empty?).map { |x| "#{tag_prefix}#{x}" } + (labels || [])
end

def resource_parents
Expand All @@ -47,4 +47,16 @@ def parents_determining_rate
[parent_ems].compact
end
end

def resource_current_tag_names
resource ? resource.tags.collect(&:name).map { |x| x.gsub("/managed/", "") } : []
end

def resource_tag_names
tag_names ? tag_names.split("|") : []
end

def all_tag_names
resource_current_tag_names | resource_tag_names
end
end
24 changes: 22 additions & 2 deletions spec/models/chargeback_vm_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,21 @@ def result_row_for_vm(vm)

subject { ChargebackVm.build_results_for_report_ChargebackVm(options).first.first }

context "when MetricRollup#tag_names are not considered" do
before do
# report filter is set to different tag
@vm1.metric_rollups.each { |mr| mr.update(:tag_names => 'registered/no|folder_path_yellow/datacenters') }
end

it "cpu" do
expect(subject.cpu_allocated_metric).to eq(cpu_count)
used_metric = used_average_for(:cpu_usagemhz_rate_average, hours_in_month, @vm1)
expect(subject.cpu_used_metric).to be_within(0.01).of(used_metric)
expect(subject.cpu_used_cost).to be_within(0.01).of(used_metric * hourly_rate * hours_in_month)
expect(subject.cpu_allocated_cost).to be_within(0.01).of(cpu_count * count_hourly_rate * hours_in_month)
end
end

context "chargeback rate contains rate unrelated to chargeback vm" do
let!(:chargeback_rate) do
FactoryGirl.create(:chargeback_rate, :detail_params => detail_params.merge(:chargeback_rate_detail_cpu_cores_allocated => {:tiers => [count_hourly_variable_tier_rate]}))
Expand Down Expand Up @@ -905,19 +920,24 @@ def result_row_for_vm(vm)
FactoryGirl.create(:metric_rollup_vm_hr, :timestamp => report_run_time - 1.day - 17.hours,
:parent_host_id => @host1.id, :parent_ems_cluster_id => @ems_cluster.id,
:parent_ems_id => ems.id, :parent_storage_id => @storage.id,
:resource => @vm1)
:resource => @vm)
end
let(:consumption) { Chargeback::ConsumptionWithRollups.new([metric_rollup], nil, nil) }

before do
@storage.tag_with([classification_1.tag.name, classification_2.tag.name], :ns => '*')
ChargebackRate.set_assignments(:storage, [rate_assignment_options_1, rate_assignment_options_2])
@vm = FactoryGirl.create(:vm_vmware, :name => "test_vm_1", :evm_owner => admin, :ems_ref => "ems_ref", :created_on => month_beginning)
end

it "return only one chargeback rate according to tag name of Vm" do
skip('this feature needs to be added to new chargeback') if Settings.new_chargeback

[rate_assignment_options_1, rate_assignment_options_2].each do |rate_assignment|
metric_rollup.tag_names = rate_assignment[:tag].first.tag.send(:name_path)
uniq_rates = chargeback_vm.get(consumption)
@vm.tag_with(["/managed/#{metric_rollup.tag_names}"], :ns => '*')
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a test where a rollup is tagged with one set of tags and the resource/vm is tagged with an additional tag and we confirm that the additional tag is used for running the chargeback report?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point 👍 because we don't and I found bug during writing this test.

@vm.reload
uniq_rates = Chargeback::RatesCache.new.get(consumption)
consumption.instance_variable_set(:@tag_names, nil)
consumption.instance_variable_set(:@hash_features_affecting_rate, nil)
expect([rate_assignment[:cb_rate]]).to match_array(uniq_rates)
Expand Down