HMS-10723: add endpoint to return template advisory IDs#1536
Conversation
c20e13b to
2129e33
Compare
53e9dce to
ef2b48a
Compare
TenSt
left a comment
There was a problem hiding this comment.
lgtm!
There are a couple comments, but feel free to merge as is or I'll re-approve if you decide to do the changes.
| respTemplate.RepositoryUUIDS = reqTemplate.RepositoryUUIDS | ||
|
|
||
| event.SendTemplateEvent(*reqTemplate.OrgID, event.TemplateCreated, []event.TemplateEvent{event.MapTemplateResponse(respTemplate, nil, nil)}) | ||
|
|
There was a problem hiding this comment.
I'm concerned about removing these events from here because they keep the patch database in sync with our database. If we move them only to the task (which can take some time to start, or fail), I think we might start seeing situations where the databases get out of sync.
What if we left these here, and changed the task to just send TemplateUpdate events for the advisories?
There was a problem hiding this comment.
hmm good point :) keeping the create here makes sense so template data gets sent asap instead of waiting on the task. and i can change the event name for events emitted from the task for new templates to TemplateUpdated
i removed the update event from the dao too, would it make sense to restore that as well for consistency?
There was a problem hiding this comment.
I'm okay with both approaches. We're usually going for the "eventual consistency", so some delay is okay. Depends on the delay. But if we can keep updating right away without loosing any performance or spending too much resources then let's do it.
| PoolID *string // Add during task runtime | ||
| TemplateUUID string | ||
| RepoConfigUUIDs []string | ||
| PreviousSnapshotUUIDs []string |
There was a problem hiding this comment.
Should previousSnapshotUUIDs be found at runtime within the task? If the tasks fail or run in a different order than expected, I think this might cause some incorrect errata diffs. Thoughts?
There was a problem hiding this comment.
i see what you mean. if 2 update tasks are running for the same template, and one finishes first or fails halfway through, the other would have the wrong prev snapshot uuids.
in cases of out of order tasks or retries, wouldn't checking the prev snapshots inside the task also result in wrong errata diffs? i'll try to test these scenarios with prev snapshots in the payload vs the task to understand more clearly
There was a problem hiding this comment.
Hmm so the task takes the repositories that should be in the template in the payload, and when the task is complete the template is in that state. So I think the diff would be okay? But maybe there's still a race condition in there somewhere
Not sure what other options would be except to do this in the handler, and not use a task (maybe too slow?). Or send the whole list to patch and let patch calculate the changes against the system (maybe too many events?).
There was a problem hiding this comment.
Can we run the tasks sequentially for the same template? This will create some delay, but will solve these issues. As we're having here async tasks, some delay is inevitable and we can use it to make sure consistency of the data.
There was a problem hiding this comment.
the previous snapshots are sent in the payload because these are updated in the db via insertTemplateRepoConfigsAndSnapshots and lost before the task starts. but you're right that this could produce wrong diffs when tasks finish out of order :/ i don't see a way to retrieve or recompute this previous state in the task without storing it somewhere else first, i might have overlooked something though.
doing this in the handler would be slower and maybe too optimistic, meaning we send a diff to patch even if a step afterward fails. sending everything to patch would mean more events, and maybe too many, but it might make sense to have the diff logic where the template-advisory relationships are stored.
if storing the previous snapshots for a template somewhere makes sense or if there's another way to fetch the previous snapshot in the task, i can switch to that. or if we can ensure tasks are run sequentially (maybe this can be done with task dependencies?), that's another option :) otherwise, sending the full set to patch seems like a reasonable approach, but that would be a big design shift so we should talk through it with the team.
There was a problem hiding this comment.
discussed a different approach in slack, we're going to add an endpoint patch can use to fetch the current advisories in a template. in patch, whenever there's an event that a template has been updated, we'll query this endpoint and calculate the diff. moving this back to draft while i update this :)
6a0e43f to
b5cc428
Compare
e8d6e23 to
6331274
Compare
rverdile
left a comment
There was a problem hiding this comment.
delete-snapshots and delete-repository-snapshots also modify templates, so those need to send an update event too
thank you! was about to ask which other 2 tasks needed to send an event :) will add that in |
Summary
Testing steps
/templates/:uuid/advisories/ids. This should return all the advisory IDs in the template.make kafka-topic-consume KAFKA_TOPIC=platform.content-sources.templateupdate-template-contenttask.update-latest-snapshottask, and a message should be sent in the consumer output during that task.delete-snapshotstask and a message should be sent in the consumer output.delete-repository-snapshotstask and a message should be sent during this task too.