Draft: Add performance tests for sync license
What does this MR do and why?
This MR adds performance tests for sync license feature.
Related to #433407 (closed)
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %16.7
added backend groupsecurity policies labels
assigned to @mc_rocha
added devopsgovern sectionsec labels
- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@mc_rocha - please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
mentioned in issue gitlab-com/gl-infra/production#17171 (closed)
Setting label(s) Category:Security Policy Management based on groupsecurity policies.
added Category:Security Policy Management label
added typemaintenance label
added maintenancetest-gap label
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend @hustewart
(UTC-5, same timezone as author)
@sdungarwal
(UTC+5.5, 10.5 hours ahead of author)
Please check reviewer's status!
Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger116 worker.perform(configuration.project_id, configuration.id) 117 end.to issue_same_number_of_queries_as(control) 118 end 119 end 120 121 context 'when the policy did not changed but a user is added to the project' do 122 it 'does not increase the query count', :use_sql_query_cache do 123 control = ActiveRecord::QueryRecorder.new(skip_cached: false) do 124 worker.perform(configuration.project_id, configuration.id) 125 end 126 127 configuration.project.add_developer(create(:user)) 128 129 expect do 130 worker.perform(configuration.project_id, configuration.id) 131 end.not_to exceed_query_limit(control) 37 37 38 38 subject 39 39 end 40 41 context 'with multiple projects in the namespace' do 42 let_it_be(:worker) { Security::ProcessScanResultPolicyWorker } 43 44 it 'does trigger SyncScanResultPoliciesProjectService for each project in group' do 45 create_list(:project, 10, namespace: namespace) 46 47 expect(worker).to receive(:perform_async).and_call_original.exactly(11).times This spec captures one of the root causes of the incident mentioned here:
Huge number of invocation of
Security::ProcessScanResultPolicyWorker
If we follow the suggestion of using a batch of projects, this will reduce the number of invocations of
Security::ProcessScanResultPolicyWorker
.@mc_rocha Once we introduce the suggested ID batching, this test will fail, despite it being a performance improvement. It seems to test this loop. Which sorts of performance issues could it detect that we might introduce?
Edited by Dominic BauerWhich sorts of performance issues could it detect that we might introduce?
Great question @bauerdominic!
Maybe it won't detect new performance issues, but I had it for 2 reasons:
-
I believe this is a spec we were missing. If we had it before, it could have helped us identify the potential performance risk.
-
If this spec is merged before the ID batching update, it will be explicit that we are no longer enqueuing a job for each project.
-
@mc_rocha Agreed that the spec is useful! I don't think we disagree here. My concern regarding #433409 (closed) is that is aimed at detecting performance regressions by counting queries, especially regarding the improvements from &9971.
I think the improvements &9971 will bring can be tested by counting queries, since we will be able to express the assertions in relative terms: If only one policy is changed, we should need less queries than if two policies were changed.
But since today's implementation deletes and recreates all
scan_result_policy_read
s and their associations, a change to one or two policies take us the same amount of queries, because we also rebuild all other policies. So we would end up with an already failing test case, and I don't think these are useful (!138401 (comment 1681530094)).I believe this is a spec we were missing. If we had it before, it could have helped us identify the potential performance risk.
I am not sure about this, since the worker behaved as specified in the test case. What happened was that the worker got repeatedly invoked for a group with a large number of contained projects. So we could also have written the test like:
create_list(:project, 12345, namespace: namespace) expect(worker).to receive(:perform_async).and_call_original.exactly(12346).times
We correctly get 12346 invocations of
ProcessScanResultPolicyWorker
, so this test wouldn't have saved us.WDYT?
Thanks for the detailed response @bauerdominic!
I believe it is a missing spec because the expected behavior was not explicit in the tests. No spec indicated that we are invoking a job for each project in a group.
I thought that making this behavior explicit in a spec could have helped us because this incident and this another incident was caused by a large number of invocations of a worker.
In one incident we opted to inline some of the workers invocations. And for the most recent one we are discussing about batching to reduce the number of workers.
I believe that worker invocation is a point for attention during our reviews, and specs like this one could help anticipate future performance incidents.
Once we introduce the suggested ID batching, this test will fail, despite it being a performance improvement.
I believe this is expected since we would be changing the service's behavior. Fixing this test in the future will demonstrate the performance improvement.
102 enabled: true, 103 rules: [ 104 { type: 'scan_finding', branches: %w[main], vulnerabilities_allowed: 0, 105 severity_levels: %w[critical], scanners: %w[container_scanning], 106 vulnerability_states: %w[detected] } 107 ], 108 actions: [ 109 { type: 'require_approval', approvals_required: 1, user_approvers: %w[admin] } 110 ] 111 } 112 113 policies[:scan_result_policy].push(policy) 114 115 expect do 116 worker.perform(configuration.project_id, configuration.id) 117 end.to issue_same_number_of_queries_as(control) The failure in this spec indicates that the number of queries increases when a new policy is added.
@mc_rocha But isn't that expected for the current implementation?
Would you set the test to pending until &9971 is done?
Edited by Dominic BauerBut isn't that expected for the current implementation?
I believe so @bauerdominic. I was trying to capture this in a spec, but I'm unsure which expectation we should check here.
Setting it to pending sounds like a good idea.
Edited by Marcos RochaSetting it to pending sounds like a good idea.
@mc_rocha I would disagree, since an already failing test cannot fail and so we lose the benefit of getting alerted when it tips over. We'd have to check the test job output manually which adds toil, and what such a test could tell us is that performance has improved, not that it has worsened. I'd say test suites should test the current implementation, not a potential future one. WDYT?
Edited by Dominic BauerThat makes sense @bauerdominic.
Maybe this spec should be moved to the MR responsible for the improvement.
mentioned in issue #433409 (closed)
mentioned in issue #433407 (closed)
The relevant spec that could be added, was added by MR !139409 (merged).