Skip to content

Commit

Permalink
Merge pull request ManageIQ#21117 from gtanzillo/fix-multi-tags-charg…
Browse files Browse the repository at this point in the history
…eback

Fixes bug where filtering by tag was returning all metric rollups regardless of assigned tags
  • Loading branch information
kbrock authored Mar 29, 2021
2 parents 0590341 + b28a7a4 commit d2b7104
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 6 deletions.
2 changes: 1 addition & 1 deletion app/models/chargeback_configured_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def self.where_clause(records, options, region)
scope = records.where(:resource_type => "ConfiguredSystem")
if options[:tag] && (@report_user.nil? || !@report_user.self_service?)
scope_with_current_tags = scope.where(:resource => ConfiguredSystem.find_tagged_with(:any => @options[:tag], :ns => '*'))
scope.for_tag_names(options[:tag].split("/")[2..-1]).or(scope_with_current_tags)
scope.for_tag_names(Array(options[:tag]).flatten.map { |t| t.split("/")[2..-1] }).or(scope_with_current_tags)
else
scope.where(:resource => configured_systems(region))
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/chargeback_vm.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def self.where_clause(records, options, region)
scope = records.where(:resource_type => "VmOrTemplate")
if options[:tag] && (@report_user.nil? || !@report_user.self_service?)
scope_with_current_tags = scope.where(:resource => Vm.find_tagged_with(:any => @options[:tag], :ns => '*'))
scope.for_tag_names(options[:tag].split("/")[2..-1]).or(scope_with_current_tags)
scope.for_tag_names(Array(options[:tag]).flatten.map { |t| t.split("/")[2..-1] }).or(scope_with_current_tags)
else
scope.where(:resource => vms(region))
end
Expand Down
4 changes: 2 additions & 2 deletions app/models/metric/common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ def nil_out_values_for_apply_time_profile
end

class_methods do
def for_tag_names(*args)
where("tag_names like ?", "%" + args.join("/") + "%")
def for_tag_names(args)
where(args.map { |t| "tag_names like ?" }.join(" OR "), *(args.map { |t| "%" + t.join("/") + "%" }))
end

def for_time_range(start_time, end_time)
Expand Down
2 changes: 1 addition & 1 deletion app/models/vim_performance_tag_value.rb
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def self.build_for_association(parent_perf, assoc, options = {})
def self.get_metrics(resources, timestamp, capture_interval_name, vim_performance_daily, category)
if vim_performance_daily
MetricRollup.with_interval_and_time_range("hourly", (timestamp)..(timestamp+1.day)).where(:resource => resources)
.for_tag_names(category, "") # append trailing slash
.for_tag_names([[category, ""]]) # append trailing slash
else
Metric::Helper.class_for_interval_name(capture_interval_name).where(:resource => resources)
.with_interval_and_time_range(capture_interval_name, timestamp)
Expand Down
26 changes: 26 additions & 0 deletions lib/before_session_middleware.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Add to config/application.rb:
#
# config.middleware.use 'BeforeSessionMiddleware'
#
class BeforeSessionMiddleware
def initialize(app)
@app = app
end

def call(env)
session_id = cookies(env)['_vmdb_session']
Rails.logger.info("Session ID: #{session_id.inspect}")

@app.call(env)
end

private

def cookies(env)
env["HTTP_COOKIE"].split(/\s*;\s*/).map do |keyval|
keyval.split('=')
end.to_h
rescue StandardError
{}
end
end
18 changes: 17 additions & 1 deletion spec/models/chargeback_vm_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,8 @@ def pluck_rollup(metric_rollup_records)

let(:development_vm) { FactoryBot.create(:vm_vmware, :created_on => month_beginning) }
let(:other_vm) { FactoryBot.create(:vm_vmware, :created_on => month_beginning) }
let(:another_vm) { FactoryBot.create(:vm_vmware, :created_on => month_beginning) }
let(:yet_another_vm) { FactoryBot.create(:vm_vmware, :created_on => month_beginning) }

before do
environment_category = Classification.find_by(:description => "Environment")
Expand All @@ -392,16 +394,20 @@ def pluck_rollup(metric_rollup_records)

metric_rollup_params.delete(:tag_names)
add_metric_rollups_for(other_vm, start_time...finish_time, 1.hour, metric_rollup_params)
metric_rollup_params[:tag_names] = ""
add_metric_rollups_for(another_vm, start_time...finish_time, 1.hour, metric_rollup_params)
metric_rollup_params[:tag_names] = "environment/dev"
add_metric_rollups_for(development_vm, start_time...finish_time, 1.hour, metric_rollup_params)
metric_rollup_params[:tag_names] = "department/accounting|department/engineering"
add_metric_rollups_for(yet_another_vm, start_time...finish_time, 1.hour, metric_rollup_params)
end

subject do
ChargebackVm.build_results_for_report_ChargebackVm(options).first.map { |x| x.entity.id }
end

it "doesn't filter resources without filter" do
expect(subject).to match_array([@vm1.id, development_vm.id, other_vm.id])
expect(subject).to match_array([@vm1.id, development_vm.id, other_vm.id, another_vm.id, yet_another_vm.id])
end

context "with filter" do
Expand All @@ -422,6 +428,16 @@ def pluck_rollup(metric_rollup_records)
expect(subject).to eq([@vm1.id, development_vm.id])
end
end

context "with multiple tags assigned to metric rollups, only" do
let(:options) do
base_filter_options.merge(:tag => ['/managed/department/accounting', '/managed/department/engineering'])
end

it "filters according to multiple tags assigned on metric rollups only" do
expect(subject).to eq([yet_another_vm.id])
end
end
end
end
end
Expand Down
25 changes: 25 additions & 0 deletions spec/models/vim_performance_tag_value_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
RSpec.describe VimPerformanceTagValue do
include Spec::Support::ChargebackHelper

context "#get_metrics" do
let(:ts) { Time.zone.now }
let(:category) { [] }
Expand All @@ -23,5 +25,28 @@
metrics = VimPerformanceTagValue.send('get_metrics', resources, ts, interval, vim_performance_daily, category)
expect(metrics).to be_empty
end

context "with metrics and a category" do
let(:development_vm) { FactoryBot.create(:vm_vmware, :created_on => report_run_time) }

let(:starting_date) { Time.parse('2012-09-01 23:59:59Z').utc }
let(:ts) { starting_date.in_time_zone(Metric::Helper.get_time_zone(:tz => 'UTC')) }
let(:report_run_time) { ts.end_of_month.utc }

let(:start_time) { report_run_time - 17.hours }
let(:finish_time) { report_run_time - 14.hours }

before do
metric_rollup_params = { :tag_names => "environment/dev" }
add_metric_rollups_for(development_vm, start_time...finish_time, 1.hour, metric_rollup_params)
end

it "finds metrics" do
interval = nil
vim_performance_daily = true
metrics = VimPerformanceTagValue.send('get_metrics', [development_vm], start_time, interval, vim_performance_daily, "environment")
expect(metrics).not_to be_empty
end
end
end
end

0 comments on commit d2b7104

Please sign in to comment.