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

refact(RHINENG-15309): Refactor filtering logic related to custom staleness #2183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
56 changes: 45 additions & 11 deletions api/filtering/db_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
from api.filtering.db_custom_filters import build_system_profile_filter
from api.filtering.db_custom_filters import get_host_types_from_filter
from api.staleness_query import get_staleness_obj
from app.auth.identity import IdentityType
from app.config import ALL_STALENESS_STATES
from app.config import HOST_TYPES
from app.culling import staleness_to_conditions
from app.culling import Conditions
from app.exceptions import ValidationException
from app.logging import get_logger
from app.models import OLD_TO_NEW_REPORTER_MAP
Expand All @@ -27,9 +29,16 @@
from app.models import db
from app.serialization import serialize_staleness_to_dict
from app.utils import Tag
from lib.host_repository import ALL_STALENESS_STATES

__all__ = ("canonical_fact_filter", "query_filters", "host_id_list_filter", "rbac_permissions_filter")
__all__ = (
"canonical_fact_filter",
"query_filters",
"host_id_list_filter",
"rbac_permissions_filter",
"stale_timestamp_filter",
"staleness_to_conditions",
"update_query_for_owner_id",
)

logger = get_logger(__name__)
DEFAULT_STALENESS_VALUES = ["not_culled"]
Expand Down Expand Up @@ -79,7 +88,7 @@ def _group_ids_filter(group_id_list: list) -> list:
return _query_filter


def _get_host_modified_on_time_filter(gt=None, lte=None):
def stale_timestamp_filter(gt=None, lte=None):
filters = []
if gt:
filters.append(Host.modified_on > gt)
Expand All @@ -89,18 +98,18 @@ def _get_host_modified_on_time_filter(gt=None, lte=None):
return and_(*filters)


def _stale_timestamp_filter(gt=None, lte=None, host_type=None):
return _get_host_modified_on_time_filter(gt=gt, lte=lte)
def _host_type_filter(host_type: str | None):
return Host.system_profile_facts["host_type"].as_string() == host_type


def _stale_timestamp_per_reporter_filter(gt=None, lte=None, reporter=None, host_type=None):
def _stale_timestamp_per_reporter_filter(gt=None, lte=None, reporter=None):
non_negative_reporter = reporter.replace("!", "")
reporter_list = [non_negative_reporter]
if non_negative_reporter in OLD_TO_NEW_REPORTER_MAP.keys():
reporter_list.extend(OLD_TO_NEW_REPORTER_MAP[non_negative_reporter])

if reporter.startswith("!"):
time_filter_ = _get_host_modified_on_time_filter(gt=gt, lte=lte)
time_filter_ = stale_timestamp_filter(gt=gt, lte=lte)
return and_(
and_(
not_(Host.per_reporter_staleness.has_key(rep)),
Expand Down Expand Up @@ -135,12 +144,13 @@ def per_reporter_staleness_filter(staleness, reporter, host_type_filter):
staleness,
host_type,
partial(_stale_timestamp_per_reporter_filter, reporter=reporter),
True,
)
)
if len(host_type_filter) > 1:
staleness_conditions.append(
and_(
Host.system_profile_facts["host_type"].astext == host_type,
_host_type_filter(host_type),
conditions,
)
)
Expand All @@ -156,11 +166,11 @@ def _staleness_filter(
staleness_obj = serialize_staleness_to_dict(get_staleness_obj(identity))
staleness_conditions = []
for host_type in host_type_filter:
conditions = or_(*staleness_to_conditions(staleness_obj, staleness, host_type, _stale_timestamp_filter))
conditions = or_(*staleness_to_conditions(staleness_obj, staleness, host_type, stale_timestamp_filter, True))
thearifismail marked this conversation as resolved.
Show resolved Hide resolved
if len(host_type_filter) > 1:
staleness_conditions.append(
and_(
Host.system_profile_facts["host_type"].astext == host_type,
_host_type_filter(host_type),
conditions,
)
)
Expand All @@ -170,6 +180,21 @@ def _staleness_filter(
return [or_(*staleness_conditions)]


def staleness_to_conditions(
staleness, staleness_states, host_type, timestamp_filter_func, omit_host_type_filter: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

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

should stick with one name staleness_states or staleness_type? The caller calls it type but the argument uses states; unnecessary change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is retaining the same signature as what it had when it was in culling.py.
Also, I don't see staleness_type used anywhere; can you please point me to that?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh it was staleness_types on line 206, 207, and 228 in lib/host_repository.py

):
def _timestamp_and_host_type_filter(condition, state):
filter_ = timestamp_filter_func(*getattr(condition, state)())
if omit_host_type_filter:
return filter_
else:
return and_(filter_, _host_type_filter(host_type))

condition = Conditions(staleness, host_type)
filtered_states = (state for state in staleness_states if state != "unknown")
return (_timestamp_and_host_type_filter(condition, state) for state in filtered_states)


def _registered_with_filter(registered_with: list[str], host_type_filter: set[str | None]) -> list:
_query_filter: list = []
if not registered_with:
Expand Down Expand Up @@ -257,6 +282,15 @@ def rbac_permissions_filter(rbac_filter: dict) -> list:
return _query_filter


def update_query_for_owner_id(identity, query):
# kafka based requests have dummy identity for working around the identity requirement for CRUD operations
logger.debug("identity auth type: %s", identity.auth_type)
if identity and identity.identity_type == IdentityType.SYSTEM:
return query.filter(and_(Host.system_profile_facts["owner_id"].as_string() == identity.system["cn"]))
else:
return query


def query_filters(
fqdn: str | None = None,
display_name: str | None = None,
Expand Down
2 changes: 1 addition & 1 deletion api/host.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from api.cache import CACHE
from api.cache import delete_cached_system_keys
from api.cache_key import make_system_cache_key
from api.filtering.db_filters import update_query_for_owner_id
from api.host_query import build_paginated_host_list_response
from api.host_query import staleness_timestamps
from api.host_query_db import get_all_hosts
Expand Down Expand Up @@ -58,7 +59,6 @@
from lib.host_repository import find_existing_host
from lib.host_repository import find_non_culled_hosts
from lib.host_repository import get_host_list_by_id_list_from_db
from lib.host_repository import update_query_for_owner_id
from lib.middleware import rbac

FactOperations = Enum("FactOperations", ("merge", "replace"))
Expand Down
4 changes: 2 additions & 2 deletions api/host_query_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
from api.filtering.db_filters import host_id_list_filter
from api.filtering.db_filters import query_filters
from api.filtering.db_filters import rbac_permissions_filter
from api.filtering.db_filters import update_query_for_owner_id
from api.host_query import staleness_timestamps
from api.staleness_query import get_staleness_obj
from app.auth import get_current_identity
from app.config import ALL_STALENESS_STATES
from app.exceptions import InventoryException
from app.instrumentation import log_get_host_list_succeeded
from app.logging import get_logger
Expand All @@ -29,8 +31,6 @@
from app.models import HostGroupAssoc
from app.models import db
from app.serialization import serialize_host_for_export_svc
from lib.host_repository import ALL_STALENESS_STATES
from lib.host_repository import update_query_for_owner_id

__all__ = (
"get_all_hosts",
Expand Down
1 change: 1 addition & 0 deletions app/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
PRODUCER_ACKS = {"0": 0, "1": 1, "all": "all"}

HOST_TYPES = ["edge", None]
ALL_STALENESS_STATES = ("fresh", "stale", "stale_warning")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a no possibility of checking "culled" state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All I'm doing is moving this constant from host_repository.py to config.py, not changing any values. But no, we don't allow querying hosts that are in the culled state

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is needed so I filed RHINENG-15328.



class Config:
Expand Down
8 changes: 1 addition & 7 deletions app/culling.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from datetime import timedelta
from datetime import timezone

__all__ = ("Conditions", "staleness_to_conditions", "Timestamps", "days_to_seconds")
__all__ = ("Conditions", "Timestamps", "days_to_seconds")


class _Config(namedtuple("_Config", ("stale_warning_offset_delta", "culled_offset_delta"))):
Expand Down Expand Up @@ -93,12 +93,6 @@ def find_host_state(stale_timestamp, stale_warning_timestamp):
return "stale warning"


def staleness_to_conditions(staleness, staleness_states, host_type, timestamp_filter_func):
condition = Conditions(staleness, host_type)
filtered_states = (state for state in staleness_states if state != "unknown")
return (timestamp_filter_func(*getattr(condition, state)(), host_type=host_type) for state in filtered_states)


def days_to_seconds(n_days: int) -> int:
factor = 86400
return n_days * factor
27 changes: 4 additions & 23 deletions lib/host_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
from sqlalchemy import not_
from sqlalchemy import or_

from api.filtering.db_filters import stale_timestamp_filter
from api.filtering.db_filters import staleness_to_conditions
from api.filtering.db_filters import update_query_for_owner_id
from api.staleness_query import get_staleness_obj
from api.staleness_query import get_sys_default_staleness
from app.auth import get_current_identity
from app.auth.identity import IdentityType
from app.config import ALL_STALENESS_STATES
from app.config import HOST_TYPES
from app.culling import staleness_to_conditions
from app.exceptions import InventoryException
from app.logging import get_logger
from app.models import Host
Expand All @@ -27,9 +29,7 @@
"find_host_by_multiple_canonical_facts",
"find_hosts_by_staleness",
"find_non_culled_hosts",
"stale_timestamp_filter",
"update_existing_host",
"update_query_for_owner_id",
)

AddHostResult = Enum("AddHostResult", ("created", "updated"))
Expand All @@ -44,7 +44,6 @@
IMMUTABLE_CANONICAL_FACTS = ("provider_id",)
MUTABLE_CANONICAL_FACTS = tuple(set(ELEVATED_CANONICAL_FACT_FIELDS).difference(set(IMMUTABLE_CANONICAL_FACTS)))

ALL_STALENESS_STATES = ("fresh", "stale", "stale_warning")
NULL = None

logger = get_logger(__name__)
Expand Down Expand Up @@ -262,15 +261,6 @@ def update_existing_host(existing_host, input_host, update_system_profile):
return existing_host, AddHostResult.updated


def stale_timestamp_filter(gt=None, lte=None, host_type=None):
filter_ = ()
if gt:
filter_ += (Host.modified_on > gt,)
if lte:
filter_ += (Host.modified_on <= lte,)
return and_(*filter_, (Host.system_profile_facts["host_type"].as_string() == host_type))


def contains_no_incorrect_facts_filter(canonical_facts):
# Does not contain any incorrect CF values
# Incorrect value = AND( key exists, NOT( contains key:value ) )
Expand Down Expand Up @@ -300,15 +290,6 @@ def matches_at_least_one_canonical_fact_filter(canonical_facts):
return or_(*filter_)


def update_query_for_owner_id(identity, query):
# kafka based requests have dummy identity for working around the identity requirement for CRUD operations
logger.debug("identity auth type: %s", identity.auth_type)
if identity and identity.identity_type == IdentityType.SYSTEM:
return query.filter(and_(Host.system_profile_facts["owner_id"].as_string() == identity.system["cn"]))
else:
return query


def update_system_profile(input_host, identity):
if not input_host.system_profile_facts:
raise InventoryException(
Expand Down