MGMT-18863: Fix bug in host event list endpoint #6846
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@paul-maidment: This pull request references MGMT-18863 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.18.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: paul-maidment The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6846 +/- ##
==========================================
- Coverage 68.73% 68.70% -0.03%
==========================================
Files 249 249
Lines 37278 37302 +24
==========================================
+ Hits 25624 25630 +6
- Misses 9373 9382 +9
- Partials 2281 2290 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
paul-maidment
force-pushed
the
MGMT-18863
branch
2 times, most recently
from
3ed455b
to
4882798
Compare
|
/lgtm |
|
/hold Revision 4882798 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
paul-maidment
force-pushed
the
MGMT-18863
branch
from
4882798
to
79cf96a
Compare
|
New changes are detected. LGTM label has been removed. |
paul-maidment
force-pushed
the
MGMT-18863
branch
from
79cf96a
to
7cc9fba
Compare
There is a scenario in which a user will use the same host for multiple installation attempts, in this scenario the cluster, infra env and original host will be deleted each time an installation is attempted. When a new cluster is created and the host is added, it will have the same ID as the previously installed host. The events will contain entries for all previous incarnations of that host. The problem occurs when the user attempts to fetch "host bound" events for that host, providing the host ID exclusively.The existing implementation of the events endpoint will return all events for a given host ID. This creates an excessively large number of results, many of which are invalid. The solution implemented in this PR is to ensure that when fetching host exclusive events we condsider only events for which there is an infra env and a host, this will ensure that the events are for the "current" version of that host. Additionally, some changes have been made to the way in which we fetch events from the ORM (gorm), the previous implementation focussed on binding to models of Hosts, Clusters or InfraEnvs, when in fact the core fields we use are event related.The queries have been rephrased to emphasize this "event centric" nature and to hopefully make the queries clearer to understand.
paul-maidment
force-pushed
the
MGMT-18863
branch
from
7cc9fba
to
1553507
Compare
|
/test edge-subsystem-kubeapi-aws |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
| return tx.Model(&common.Event{}).Select("events.*, infra_envs.user_name, infra_envs.org_id"). | ||
| Joins("INNER JOIN infra_envs ON infra_envs.id = events.infra_env_id"). | ||
| Joins("INNER JOIN hosts ON hosts.id = events.host_id"). // This join is here to ensure that only events for a host that exists are fetched | ||
| Where("hosts.deleted_at IS NULL") // Only interested in active hosts |
There was a problem hiding this comment.
Is this a new behaviour? Would we be able to see deleted host right now?
There was a problem hiding this comment.
@rccrdpccl
tx.Model.Select(...) usually considers fields such as "deleted at" because we are dealing with entities (GORM models the entities with fields such as deleted_at and uses this field to understand which entities should be considered "soft deleted") .
The only entity that we use in this query is Event, the remaining tables are not represented as GORM entities in this query (and I think it would be overkill/bad for performance)
It is my understanding that the various pieces of SQL, joins and so on, will not be aware of the deleted/non deleted status of the Host.
So it's not new behaviour but it is something we need to take care of when changing the model from Host to Event.
|
@paul-maidment: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There is a scenario in which a user will use the same host for multiple installation attempts, in this scenario the cluster, infra env and original host will be deleted each time an installation is attempted.
When a new cluster is created and the host is added, it will have the same ID as the previously installed host. The events will contain entries for all previous incarnations of that host.
The problem occurs when the user attempts to fetch "host bound" events for that host, providing the host ID exclusively.The existing implementation of the events endpoint will return all events for a given host ID.
This creates an excessively large number of results, many of which are invalid.
The solution implemented in this PR
is to ensure that when fetching host exclusive events we condsider only events for which there is an infra env and a host, this will ensure that the events are for the "current" version of that host.
Additionally, some changes have been made to the way in which we fetch events from the ORM (gorm), the previous implementation focussed on binding to models of Hosts, Clusters or InfraEnvs, when in fact the core fields we use are event related.The queries have been rephrased to emphasize this "event centric" nature and to hopefully make the queries clearer to understand.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist