Skip to content

Commit

Permalink
fix: avoid unnecessary filters on system_platform.rh_account_id
Browse files Browse the repository at this point in the history
it results in bad query plan for some accounts

RHINENG-14433
  • Loading branch information
jdobes committed Dec 4, 2024
1 parent cb15e7f commit bb87df8
Show file tree
Hide file tree
Showing 10 changed files with 37 additions and 39 deletions.
6 changes: 3 additions & 3 deletions common/peewee_conditions.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

# pylint: disable=singleton-comparison,superfluous-parens
from peewee import Expression
from peewee import Value
from peewee import fn
from psycopg2.extras import Json

Expand All @@ -13,14 +14,13 @@
from .peewee_model import SystemVulnerabilities


def system_is_active(opt_out=False, stale=False, deleted=False, edge=False, rh_account_id=None):
def system_is_active(opt_out=False, stale=False, deleted=False, edge=False):
"""
Filter out invalid systems from system_platform table.
Filter by rh_account_id if present.
Expects table: system_platform
"""

cond = SystemPlatform.rh_account_id == rh_account_id
cond = Value(True)

if opt_out is not None:
cond &= SystemPlatform.opt_out == opt_out
Expand Down
6 changes: 3 additions & 3 deletions manager/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ def get_subquery(cves: List[int], rh_account_id):

subquery = (SystemVulnerabilities.select(SystemVulnerabilities.id, SystemVulnerabilities.rule_id, SystemVulnerabilities.cve_id)
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(rh_account_id=rh_account_id, edge=None)))
system_is_active(edge=None)))
.where(SystemVulnerabilities.cve_id.in_(cves))
.where(SystemVulnerabilities.mitigation_reason.is_null(True))
.where((SystemVulnerabilities.rh_account_id == rh_account_id)))
Expand Down Expand Up @@ -641,7 +641,7 @@ def get_system_count(rh_account, include_cyndi=True, filters=None, filters_args=
query = SystemPlatform.select(fn.COUNT(SystemPlatform.id).alias("count"))\
.where((SystemPlatform.rh_account_id == rh_account)
& ((SystemPlatform.last_evaluation.is_null(False)) | (SystemPlatform.advisor_evaluated.is_null(False)))
& system_is_active(rh_account_id=rh_account, edge=None))
& system_is_active(edge=None))

if include_cyndi:
query = cyndi_join(query)
Expand All @@ -662,7 +662,7 @@ def get_system_count_by_type(rh_account_id) -> Dict[str, int]:
query = (SystemPlatform.select(*selectables)
.where((SystemPlatform.rh_account_id == rh_account_id) &
((SystemPlatform.last_evaluation.is_null(False)) | (SystemPlatform.advisor_evaluated.is_null(False))) &
system_is_active(rh_account_id=rh_account_id, edge=None))
system_is_active(edge=None))
.dicts())
query = cyndi_join(query)
return query.first()
Expand Down
14 changes: 7 additions & 7 deletions manager/cve_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ def _full_query(rh_account_id, synopsis, parsed_args, filters, remediation_filte
.join(InsightsRule, JOIN.LEFT_OUTER, on=(InsightsRule.id == SystemVulnerabilities.rule_id))
.where(CveMetadata.cve == synopsis)
.where(SystemVulnerabilities.rh_account_id == rh_account_id)
.where(system_is_active(rh_account_id=rh_account_id, edge=None))
.where(system_is_active(edge=None))
.where(system_is_vulnerable(rule_subselect=False)))
if remediation_filter:
subq = subq.where(SystemVulnerabilities.remediation_type_id << remediation_filter)
Expand Down Expand Up @@ -319,7 +319,7 @@ def _unpatched_full_query(rh_account_id, synopsis, parsed_args, filters):
.join(CveAccountData, JOIN.LEFT_OUTER, on=((CveAccountData.rh_account_id == rh_account_id) &
(CveMetadata.id == CveAccountData.cve_id)))
.where(CveMetadata.cve == synopsis)
.where(system_is_active(rh_account_id=rh_account_id, edge=None))
.where(system_is_active(edge=None))
.where(SystemVulnerablePackage.rh_account_id == rh_account_id))
unfixed_subq = cyndi_join(unfixed_subq)
unfixed_subq = apply_filters(unfixed_subq, parsed_args, filters, {"unfixed": [True]})
Expand Down Expand Up @@ -365,7 +365,7 @@ def _id_query(rh_account_id, synopsis, parsed_args, filters, remediation_filter=
.join(InsightsRule, JOIN.LEFT_OUTER, on=(InsightsRule.id == SystemVulnerabilities.rule_id))
.where(CveMetadata.cve == synopsis)
.where(SystemVulnerabilities.rh_account_id == rh_account_id)
.where(system_is_active(rh_account_id=rh_account_id, edge=None))
.where(system_is_active(edge=None))
.where(system_is_vulnerable(rule_subselect=False)))
if remediation_filter:
subq = subq.where(SystemVulnerabilities.remediation_type_id << remediation_filter)
Expand Down Expand Up @@ -413,7 +413,7 @@ def _unpatched_id_query(rh_account_id, synopsis, parsed_args, filters):
(CveMetadata.id == CveAccountData.cve_id)))
.where(CveMetadata.cve == synopsis)
.where(SystemVulnerablePackage.rh_account_id == rh_account_id)
.where(system_is_active(rh_account_id=rh_account_id, edge=None)))
.where(system_is_active(edge=None)))

unfixed_subq = cyndi_join(unfixed_subq)
unfixed_subq = apply_filters(unfixed_subq, parsed_args, filters, {"unfixed": [True]})
Expand Down Expand Up @@ -500,7 +500,7 @@ def _cve_details(cls, synopsis, advisory_available):
.join(InsightsRule, JOIN.LEFT_OUTER, on=(SystemVulnerabilities.rule_id == InsightsRule.id))
.where((SystemVulnerabilities.rh_account_id == rh_account_id))
.where(CveMetadata.cve == synopsis)
.where(system_is_active(rh_account_id=rh_account_id, edge=None)))
.where(system_is_active(edge=None)))
base_cnt_query = cyndi_join(base_cnt_query)

abnv_query = base_cnt_query.where(system_is_abnv())
Expand Down Expand Up @@ -528,7 +528,7 @@ def _cve_details(cls, synopsis, advisory_available):
.join(InsightsRule, JOIN.LEFT_OUTER, on=(InsightsRule.id == SystemVulnerabilities.rule_id))
.where(CveMetadata.cve == synopsis)
.where(SystemVulnerabilities.rh_account_id == rh_account_id)
.where(system_is_active(rh_account_id=rh_account_id, edge=None))
.where(system_is_active(edge=None))
.where(system_is_vulnerable())
.group_by(fn.COALESCE(SystemCveData.status_id,
fn.COALESCE(CveAccountData.status_id, 0)))
Expand All @@ -550,7 +550,7 @@ def _cve_details(cls, synopsis, advisory_available):
(CveMetadata.id == CveAccountData.cve_id)))
.where(CveMetadata.cve == synopsis)
.where(SystemVulnerablePackage.rh_account_id == rh_account_id)
.where(system_is_active(rh_account_id=rh_account_id, edge=None))
.where(system_is_active(edge=None))
.group_by(fn.COALESCE(SystemCveData.status_id,
fn.COALESCE(CveAccountData.status_id, 0)))
.dicts())
Expand Down
2 changes: 1 addition & 1 deletion manager/dashbar_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def handle_get(cls, **kwargs):
.join(CveRuleMapping, JOIN.LEFT_OUTER, on=((SystemVulnerabilities.cve_id == CveRuleMapping.cve_id)))
.join(InsightsRule, JOIN.LEFT_OUTER, on=(CveRuleMapping.rule_id == InsightsRule.id))
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(rh_account_id=account_data.id, edge=None)))
system_is_active(edge=None)))
.where(SystemVulnerabilities.rh_account_id == account_data.id)
.where(system_is_vulnerable())
.where(SystemVulnerabilities.remediation_type_id << DEFAULT_REMEDIATION_FILTER))
Expand Down
4 changes: 2 additions & 2 deletions manager/dashboard_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def handle_get(cls, **kwargs):
active_cves_subquery = (SystemVulnerabilities
.select(fn.Distinct(SystemVulnerabilities.cve_id).alias("cve_id_"))
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(rh_account_id=account_data.id, edge=None)))
system_is_active(edge=None)))
.where(SystemVulnerabilities.rh_account_id == account_data.id)
.where(system_is_vulnerable())
.where(SystemVulnerabilities.remediation_type_id << DEFAULT_REMEDIATION_FILTER))
Expand Down Expand Up @@ -198,7 +198,7 @@ def handle_get(cls, **kwargs):
.select(SystemVulnerabilities.rule_id.alias("rule_id_"),
fn.Count(fn.Distinct(SystemVulnerabilities.system_id)).alias("systems_affected_"))
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(rh_account_id=account_data.id, edge=None)))
system_is_active(edge=None)))
.where(SystemVulnerabilities.rh_account_id == account_data.id)
.where(system_has_rule_hit())
.group_by(SystemVulnerabilities.rule_id)
Expand Down
6 changes: 3 additions & 3 deletions manager/report_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def handle_get(cls, **kwargs):
.select(SystemVulnerabilities.cve_id.alias("cve_id_"),
fn.Count(SystemVulnerabilities.id).alias("systems_affected_"))
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(rh_account_id=account_data.id, edge=None)))
system_is_active(edge=None)))
.where(SystemVulnerabilities.rh_account_id == account_data.id)
.where(system_is_vulnerable())
.where(SystemVulnerabilities.remediation_type_id << DEFAULT_REMEDIATION_FILTER)
Expand Down Expand Up @@ -199,7 +199,7 @@ def handle_get(cls, **kwargs):
fn.COUNT(fn.Distinct(SystemVulnerabilities.system_id)).alias("systems_affected"))
.join(InsightsRule, on=(SystemVulnerabilities.rule_id == InsightsRule.id))
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(rh_account_id=account_data.id, edge=None)))
system_is_active(edge=None)))
.where(SystemVulnerabilities.rh_account_id == account_data.id)
.where(system_has_rule_hit(rule_subselect=False))
.where(SystemVulnerabilities.remediation_type_id << DEFAULT_REMEDIATION_FILTER)
Expand Down Expand Up @@ -236,7 +236,7 @@ def handle_get(cls, **kwargs):
.join(CveRuleMapping, on=(InsightsRule.id == CveRuleMapping.rule_id))
.join(CveMetadata, on=(CveRuleMapping.cve_id == CveMetadata.id))
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(rh_account_id=account_data.id, edge=None)))
system_is_active(edge=None)))
.where(SystemVulnerabilities.rh_account_id == account_data.id)
.where(system_has_rule_hit())
.where(SystemVulnerabilities.remediation_type_id << DEFAULT_REMEDIATION_FILTER)
Expand Down
11 changes: 5 additions & 6 deletions manager/status_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,9 @@ def _prepare_data(data: Dict[str, any]) -> Tuple[Optional[List[str]], List[str],

@staticmethod
def _apply_system_list_filter(query: Query,
rh_account_id: int,
in_inventory_id_list: Optional[List[str]]) -> Query:
query = cyndi_join(query)
query = query.where((SystemPlatform.rh_account_id == rh_account_id) &
(SystemPlatform.when_deleted.is_null(True)))
query = query.where(SystemPlatform.when_deleted.is_null(True))
if in_inventory_id_list is not None:
query = query.where(SystemPlatform.inventory_id << in_inventory_id_list)
return query
Expand All @@ -104,9 +102,10 @@ def _get_current_status(cls,
SystemCveData.status_id, SystemCveData.status_text)
.join(CveMetadata, on=(SystemCveData.cve_id == CveMetadata.id))
.join(SystemPlatform, on=(SystemCveData.system_id == SystemPlatform.id))
.where(SystemPlatform.rh_account_id == rh_account_id)
.where(CveMetadata.cve << in_cve_list)
.dicts())
system_cve_details = cls._apply_system_list_filter(system_cve_details, rh_account_id, in_inventory_id_list)
system_cve_details = cls._apply_system_list_filter(system_cve_details, in_inventory_id_list)
current_status = {}
for system_cve_detail in system_cve_details:
current_status.setdefault(system_cve_detail["cve"], {})[system_cve_detail["inventory_id"]] = \
Expand Down Expand Up @@ -140,7 +139,7 @@ def _get_affected_pairs(cls,
(SystemVulnerabilities.rule_id << (InsightsRule.select(InsightsRule.id)
.where((InsightsRule.active == True) & (InsightsRule.rule_only == False)))))))
.dicts())
fixable_pairs = cls._apply_system_list_filter(fixable_pairs, rh_account_id, in_inventory_id_list)
fixable_pairs = cls._apply_system_list_filter(fixable_pairs, in_inventory_id_list)
for pair in fixable_pairs:
affected_pairs.add(SystemCvePair(pair["inventory_id"], pair["cve"]))

Expand All @@ -154,7 +153,7 @@ def _get_affected_pairs(cls,
(CveMetadata.select(CveMetadata.id).where(
CveMetadata.cve << in_cve_list))))
.dicts())
unfixable_pairs = cls._apply_system_list_filter(unfixable_pairs, rh_account_id, in_inventory_id_list)
unfixable_pairs = cls._apply_system_list_filter(unfixable_pairs, in_inventory_id_list)
for pair in unfixable_pairs:
affected_pairs.add(SystemCvePair(pair["inventory_id"], pair["cve"]))
return affected_pairs
Expand Down
14 changes: 8 additions & 6 deletions manager/system_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ def _full_query(rh_account_id, query_args, parsed_args, filters, remediation_fil
fn.COALESCE(SystemVulnerabilities.remediation_type_id, remediation.PLAYBOOK.value).alias("remediation_type_id"),
)
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(edge=None, stale=None, rh_account_id=rh_account_id)))
system_is_active(edge=None, stale=None)))
.join(CveMetadata, on=(SystemVulnerabilities.cve_id == CveMetadata.id))
.join(SystemCveData, JOIN.LEFT_OUTER, on=((SystemPlatform.id == SystemCveData.system_id)
& (CveMetadata.id == SystemCveData.cve_id)))
Expand Down Expand Up @@ -268,7 +268,7 @@ def _unpatched_full_query(rh_account_id, query_args, parsed_args, filters):
)
.distinct()
.join(SystemPlatform, on=(SystemVulnerablePackage.system_id == SystemPlatform.id) &
system_is_active(edge=None, stale=None, rh_account_id=rh_account_id))
system_is_active(edge=None, stale=None))
.join(VulnerablePackageCVE, on=(SystemVulnerablePackage.vulnerable_package_id == VulnerablePackageCVE.vulnerable_package_id))
.join(CveMetadata, on=(VulnerablePackageCVE.cve_id == CveMetadata.id))
.join(SystemCveData, JOIN.LEFT_OUTER, on=((SystemPlatform.id == SystemCveData.system_id)
Expand Down Expand Up @@ -316,7 +316,7 @@ def _id_query(rh_account_id, query_args, parsed_args, filters, remediation_filte
fn.COALESCE(SystemVulnerabilities.remediation_type_id, remediation.PLAYBOOK.value).alias("remediation_type_id"),
)
.join(SystemPlatform, on=((SystemVulnerabilities.system_id == SystemPlatform.id) &
system_is_active(edge=None, stale=None, rh_account_id=rh_account_id)))
system_is_active(edge=None, stale=None)))
.join(CveMetadata, on=(SystemVulnerabilities.cve_id == CveMetadata.id))
.join(SystemCveData, JOIN.LEFT_OUTER, on=((SystemPlatform.id == SystemCveData.system_id)
& (CveMetadata.id == SystemCveData.cve_id)))
Expand Down Expand Up @@ -367,7 +367,7 @@ def _unpatched_id_query(rh_account_id, query_args, parsed_args, filters):
)
.distinct()
.join(SystemPlatform, on=(SystemVulnerablePackage.system_id == SystemPlatform.id) &
system_is_active(edge=None, stale=None, rh_account_id=rh_account_id))
system_is_active(edge=None, stale=None))
.join(VulnerablePackageCVE, on=(SystemVulnerablePackage.vulnerable_package_id == VulnerablePackageCVE.vulnerable_package_id))
.join(CveMetadata, on=(VulnerablePackageCVE.cve_id == CveMetadata.id))
.join(SystemCveData, JOIN.LEFT_OUTER, on=((SystemPlatform.id == SystemCveData.system_id)
Expand Down Expand Up @@ -458,7 +458,8 @@ def _full_query(rh_account_id):

return (SystemPlatform
.select(*selectables)
.where(system_is_active(rh_account_id=rh_account_id, deleted=False, edge=False, opt_out=None, stale=None))
.where(SystemPlatform.rh_account_id == rh_account_id)
.where(system_is_active(deleted=False, edge=False, opt_out=None, stale=None))
.where(SystemPlatform.last_evaluation.is_null(False) | SystemPlatform.advisor_evaluated.is_null(False))
.dicts())

Expand All @@ -474,7 +475,8 @@ def _id_query(rh_account_id, list_args):

query = (SystemPlatform
.select(*selectables)
.where(system_is_active(rh_account_id=rh_account_id, deleted=False, edge=False, opt_out=None, stale=None))
.where(SystemPlatform.rh_account_id == rh_account_id)
.where(system_is_active(deleted=False, edge=False, opt_out=None, stale=None))
.where(SystemPlatform.last_evaluation.is_null(False) | SystemPlatform.advisor_evaluated.is_null(False))
.dicts())

Expand Down
Loading

0 comments on commit bb87df8

Please sign in to comment.