feat(RHINENG-22144): Update MQ to use new serialization #3269
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideRefactors host serialization, MQ event headers, and notifications to use the new normalized host fields and system profile tables instead of legacy canonical_facts/system_profile_facts, while adding host_type backward-compat behavior and centralizing UUID/datetime handling. Sequence diagram for producing host update MQ event with normalized fieldssequenceDiagram
actor Caller
participant HostSynchronizeModule as HostSynchronize
participant SerializationModule as Serialization
participant Host as HostModel
participant StaticSystemProfile as StaticProfile
participant DynamicSystemProfile as DynamicProfile
participant EventsModule as Events
participant EventProducer as MQ
Caller->>HostSynchronize: _synchronize_hosts_for_org(org_hosts_query, custom_staleness_dict, event_producer)
loop for each host
HostSynchronize->>Serialization: serialize_host(host, timestamps, staleness, fields, for_mq, system_profile_fields)
activate Serialization
Serialization->>HostModel: read basic fields (id, account, org_id, reporter, host_type, etc.)
Serialization->>Serialization: serialize_uuid(host.id, host.insights_id, host.subscription_manager_id, host.openshift_cluster_id)
Serialization->>Serialization: _build_system_profile_from_normalized(host, system_profile_fields)
activate Serialization
Serialization->>StaticProfile: iterate columns, filter, convert UUID fields
Serialization-->>Serialization: partial system_profile (static)
Serialization->>DynamicProfile: iterate columns, filter, convert datetime fields
Serialization-->>Serialization: merged system_profile (static + dynamic)
deactivate Serialization
Serialization-->>HostSynchronize: serialized_host
deactivate Serialization
HostSynchronize->>Events: extract_system_profile_fields_for_headers(host.static_system_profile)
Events-->>HostSynchronize: host_type, os_name, bootc_booted
HostSynchronize->>Serialization: serialize_uuid(host.insights_id)
Serialization-->>HostSynchronize: insights_id_str
HostSynchronize->>Events: message_headers(EventType.updated, insights_id_str, host.reporter, host_type, os_name, bootc_booted)
Events-->>HostSynchronize: headers
HostSynchronize->>MQ: write_event(event, serialized_host.id, headers, wait=True)
end
HostSynchronize-->>Caller: done
Class diagram for updated host serialization and event helper modulesclassDiagram
class Host {
+uuid id
+string account
+string org_id
+string display_name
+string ansible_host
+uuid insights_id
+uuid subscription_manager_id
+string satellite_id
+string fqdn
+string bios_uuid
+list~string~ ip_addresses
+list~string~ mac_addresses
+string provider_id
+string provider_type
+string reporter
+string host_type
+StaticSystemProfile static_system_profile
+DynamicSystemProfile dynamic_system_profile
}
class StaticSystemProfile {
+uuid org_id
+uuid host_id
+uuid owner_id
+string host_type
+string os_release
+map operating_system
+map bootc_status
}
class DynamicSystemProfile {
+uuid org_id
+uuid host_id
+datetime captured_date
+datetime last_boot_time
+map other_dynamic_fields
}
class SerializationModule {
+tuple _CANONICAL_FACTS_FIELDS
+tuple ADDITIONAL_HOST_MQ_FIELDS
+dict serialize_host(host, staleness_timestamps, staleness, fields, for_mq, system_profile_fields)
+dict _build_system_profile_from_normalized(host, system_profile_fields)
+dict serialize_host_for_export_svc(host, staleness_timestamps, staleness)
+dict serialize_group_without_host_count(group)
+dict serialize_group_with_host_count(group, host_count)
+dict serialize_host_system_profile(host)
+string serialize_uuid(u)
+dict serialize_staleness_response(staleness)
+dict deserialize_canonical_facts(raw_data, all)
+void remove_null_canonical_facts(serialized_host)
+string _map_host_type_for_backward_compatibility(host_type)
}
class EventsModule {
+tuple EventType
+tuple message_headers(event_type, insights_id, reporter, host_type, os_name, bootc_booted)
+tuple host_delete_event(event_type, host, initiated_by_frontend, platform_metadata)
+tuple extract_system_profile_fields_for_headers(static_system_profile)
}
class HostMQModule {
+Host _set_owner(host, identity)
+void sync_event_message(message, session, event_producer)
+void write_delete_event_message(event_producer, result, initiated_by_frontend)
+void write_add_update_event_message(event_producer, result, initiated_by_frontend)
}
class GroupRepositoryModule {
+void _produce_host_update_events(event_producer, serialized_groups, host_list, identity)
+void _invalidate_system_cache(host_list, identity)
}
class HostSynchronizeModule {
+void _synchronize_hosts_for_org(org_hosts_query, custom_staleness_dict, event_producer)
}
class OutboxRepositoryModule {
+dict _create_update_event_payload(host)
}
class NotificationsModule {
+dict build_base_notification_obj(notification_type, host)
+dict populate_events(base_notification_obj, host_list, extra_fields)
}
SerializationModule --> Host : uses
SerializationModule --> StaticSystemProfile : builds_system_profile
SerializationModule --> DynamicSystemProfile : builds_system_profile
EventsModule --> StaticSystemProfile : reads_for_headers
HostMQModule --> SerializationModule : serialize_host
HostMQModule --> EventsModule : message_headers
GroupRepositoryModule --> SerializationModule : serialize_host
GroupRepositoryModule --> EventsModule : message_headers
HostSynchronizeModule --> SerializationModule : serialize_host
HostSynchronizeModule --> EventsModule : message_headers
OutboxRepositoryModule --> SerializationModule : serialize_uuid
NotificationsModule --> SerializationModule : deserialize_canonical_facts
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
_build_system_profile_from_normalized, theUUID_FIELDS,DATETIME_FIELDS, andEXCLUDE_FIELDSsets are recreated on every call; consider lifting these to module-level constants to avoid unnecessary allocations on hot paths like host serialization. _build_system_profile_from_normalizedsilently swallowsDetachedInstanceErrorfor both static and dynamic system profile relationships; consider at least logging at debug level so unexpected lazy-loading issues don’t get completely hidden.- _map_host_type_for_backward_compatibility
mapsNoneand any non-edgevalue to an empty string; if the intent is to distinguish between “unknown” and “non-edge” host types in headers, consider documenting this explicitly or adjusting the mapping to preserveNone` where appropriate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_build_system_profile_from_normalized`, the `UUID_FIELDS`, `DATETIME_FIELDS`, and `EXCLUDE_FIELDS` sets are recreated on every call; consider lifting these to module-level constants to avoid unnecessary allocations on hot paths like host serialization.
- `_build_system_profile_from_normalized` silently swallows `DetachedInstanceError` for both static and dynamic system profile relationships; consider at least logging at debug level so unexpected lazy-loading issues don’t get completely hidden.
- _map_host_type_for_backward_compatibility` maps `None` and any non-`edge` value to an empty string; if the intent is to distinguish between “unknown” and “non-edge” host types in headers, consider documenting this explicitly or adjusting the mapping to preserve `None` where appropriate.
## Individual Comments
### Comment 1
<location> `app/queue/events.py:146-154` </location>
<code_context>
"type": event_type.name,
"id": host.id,
- **serialize_canonical_facts(host.canonical_facts),
+ "insights_id": host.insights_id,
+ "subscription_manager_id": host.subscription_manager_id,
+ "satellite_id": host.satellite_id,
+ "fqdn": host.fqdn,
+ "bios_uuid": host.bios_uuid,
+ "ip_addresses": host.ip_addresses,
+ "mac_addresses": host.mac_addresses,
+ "provider_id": host.provider_id,
+ "provider_type": host.provider_type,
"org_id": host.org_id,
"account": host.account,
</code_context>
<issue_to_address>
**issue (bug_risk):** Canonical fact UUIDs in delete events are no longer serialized to strings
`host_delete_event` used to rely on `serialize_canonical_facts`, which converted UUID-like canonical facts to strings. With this change, `insights_id`, `subscription_manager_id`, and possibly `provider_id` may now be `uuid.UUID` instances, which can break JSON serialization or consumers that expect strings.
To keep behavior consistent with `serialize_host` and `serialize_uuid`, wrap any UUID-typed canonical facts in `serialize_uuid(...)` before adding them to the event payload.
</issue_to_address>
### Comment 2
<location> `app/serialization.py:144` </location>
<code_context>
del serialized_host[field_name]
+def _build_system_profile_from_normalized(host, system_profile_fields=None) -> dict:
+ """
+ Build system profile dict from static and dynamic tables.
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `_build_system_profile_from_normalized` to use shared helpers and module-level constants to avoid duplicated loops and error handling.
You can keep all the new behavior but significantly reduce duplication in `_build_system_profile_from_normalized` by:
1. Lifting the per-call constants to module scope.
2. Using a single traversal helper with per-field transforms.
3. Sharing a single `try/except DetachedInstanceError` per relationship via that helper.
### Example refactor
```python
# module-level constants
UUID_FIELDS = {"owner_id", "rhc_client_id", "rhc_config_state", "virtual_host_uuid"}
DATETIME_FIELDS = {"captured_date", "last_boot_time"}
EXCLUDE_PROFILE_FIELDS = {"org_id", "host_id"}
def _add_profile_fields(model, system_profile, system_profile_fields, transforms) -> None:
if not model:
return
for column in model.__table__.columns:
name = column.name
if name in EXCLUDE_PROFILE_FIELDS:
continue
if system_profile_fields and name not in system_profile_fields:
continue
value = getattr(model, name, None)
if value is None:
continue
transform = transforms.get(name)
system_profile[name] = transform(value) if transform else value
```
Then `_build_system_profile_from_normalized` becomes:
```python
def _build_system_profile_from_normalized(host, system_profile_fields=None) -> dict:
system_profile: dict = {}
static_transforms = {name: serialize_uuid for name in UUID_FIELDS}
dynamic_transforms = {name: _serialize_datetime for name in DATETIME_FIELDS}
try:
_add_profile_fields(host.static_system_profile, system_profile, system_profile_fields, static_transforms)
except DetachedInstanceError:
pass # relationship not loaded - skip
try:
_add_profile_fields(host.dynamic_system_profile, system_profile, system_profile_fields, dynamic_transforms)
except DetachedInstanceError:
pass # relationship not loaded - skip
return system_profile
```
Benefits:
- Removes the two duplicated loops and duplicated `DetachedInstanceError` handling.
- Constants are created once and are clearly visible at module scope.
- UUID/datetime handling is explicit and centralized via the `transforms` dict while preserving the current field-based behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+146
to
+154
| "insights_id": host.insights_id, | ||
| "subscription_manager_id": host.subscription_manager_id, | ||
| "satellite_id": host.satellite_id, | ||
| "fqdn": host.fqdn, | ||
| "bios_uuid": host.bios_uuid, | ||
| "ip_addresses": host.ip_addresses, | ||
| "mac_addresses": host.mac_addresses, | ||
| "provider_id": host.provider_id, | ||
| "provider_type": host.provider_type, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Canonical fact UUIDs in delete events are no longer serialized to strings
host_delete_event used to rely on serialize_canonical_facts, which converted UUID-like canonical facts to strings. With this change, insights_id, subscription_manager_id, and possibly provider_id may now be uuid.UUID instances, which can break JSON serialization or consumers that expect strings.
To keep behavior consistent with serialize_host and serialize_uuid, wrap any UUID-typed canonical facts in serialize_uuid(...) before adding them to the event payload.
| del serialized_host[field_name] | ||
|
|
||
|
|
||
| def _build_system_profile_from_normalized(host, system_profile_fields=None) -> dict: |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring _build_system_profile_from_normalized to use shared helpers and module-level constants to avoid duplicated loops and error handling.
You can keep all the new behavior but significantly reduce duplication in _build_system_profile_from_normalized by:
- Lifting the per-call constants to module scope.
- Using a single traversal helper with per-field transforms.
- Sharing a single
try/except DetachedInstanceErrorper relationship via that helper.
Example refactor
# module-level constants
UUID_FIELDS = {"owner_id", "rhc_client_id", "rhc_config_state", "virtual_host_uuid"}
DATETIME_FIELDS = {"captured_date", "last_boot_time"}
EXCLUDE_PROFILE_FIELDS = {"org_id", "host_id"}
def _add_profile_fields(model, system_profile, system_profile_fields, transforms) -> None:
if not model:
return
for column in model.__table__.columns:
name = column.name
if name in EXCLUDE_PROFILE_FIELDS:
continue
if system_profile_fields and name not in system_profile_fields:
continue
value = getattr(model, name, None)
if value is None:
continue
transform = transforms.get(name)
system_profile[name] = transform(value) if transform else valueThen _build_system_profile_from_normalized becomes:
def _build_system_profile_from_normalized(host, system_profile_fields=None) -> dict:
system_profile: dict = {}
static_transforms = {name: serialize_uuid for name in UUID_FIELDS}
dynamic_transforms = {name: _serialize_datetime for name in DATETIME_FIELDS}
try:
_add_profile_fields(host.static_system_profile, system_profile, system_profile_fields, static_transforms)
except DetachedInstanceError:
pass # relationship not loaded - skip
try:
_add_profile_fields(host.dynamic_system_profile, system_profile, system_profile_fields, dynamic_transforms)
except DetachedInstanceError:
pass # relationship not loaded - skip
return system_profileBenefits:
- Removes the two duplicated loops and duplicated
DetachedInstanceErrorhandling. - Constants are created once and are clearly visible at module scope.
- UUID/datetime handling is explicit and centralized via the
transformsdict while preserving the current field-based behavior.
Contributor
Author
|
/retest |
jpramos123
force-pushed
the
RHINENG-22144-mq
branch
from
ddfba5d to
c612acb
Compare
jpramos123
force-pushed
the
RHINENG-22144-mq
branch
from
c612acb to
26a5dc1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Overview
This PR is being created to address RHINENG-22144.
This PR make changes to the MQ and notifications so it uses the new serialization and host_type fields
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Update host MQ serialization and notifications to use normalized host fields and new system profile storage while preserving backward compatibility for downstream consumers.
New Features:
Bug Fixes:
Enhancements: