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

Events are cleaned up if the resource is deleted #407

Closed
wants to merge 1 commit into from

Conversation

gauravgahlot
Copy link
Contributor

Description

Going forward if a resource (template, hardware, workflow, ...) is deleted, all the events for that particular resource will also be cleaned up (irrespective of configured TTL).

Why is this needed

Fixes: #396

How Has This Been Tested?

  • Manually over vagrant setup
  • Unit Tests

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@gauravgahlot gauravgahlot self-assigned this Dec 30, 2020
@gauravgahlot gauravgahlot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 30, 2020
@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #407 (4d7b2b2) into master (ad5c929) will increase coverage by 0.02%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
+ Coverage   21.38%   21.40%   +0.02%     
==========================================
  Files          24       24              
  Lines        2170     2191      +21     
==========================================
+ Hits          464      469       +5     
- Misses       1664     1679      +15     
- Partials       42       43       +1     
Impacted Files Coverage Δ
db/hardware.go 0.00% <0.00%> (ø)
db/workflow.go 0.20% <0.00%> (-0.01%) ⬇️
db/template.go 36.49% <62.50%> (+1.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad5c929...4d7b2b2. Read the comment docs.

@gauravgahlot gauravgahlot marked this pull request as draft December 30, 2020 05:24
@gauravgahlot gauravgahlot force-pushed the purge-events branch 3 times, most recently from 81d76f7 to 32b8737 Compare December 31, 2020 06:09
@gianarb
Copy link
Contributor

gianarb commented Jan 2, 2021

I am still not 100% sure that this is a valuable solution. Or the solution we want. And I want to ask @invidian to help me with this.

@gauravgahlot this solution as it is today has at least 1 issue. it is deleting all the events for a particular resource when it gets deleted. It also deletes the event that notifies that a resource is deleted I presume, it means that users and applications that rely on events to synchronize an external system or to build automation will have a hard time dealing with a deletion.

Excluding the event kind DELETED will be enough? Probably no.

Scenario: A user wrote an "audit logger" that tracks all the actions made via Tinkerbell with the default TTL of 1h. The extension for some reason stays offline for 30 minutes and in that hour the hardware gets created, updated, and deleted it will be impossible to reconstruct that part of the history. IMPOSSIBLE because events are not stored anywhere anymore.

I still think events should be immutable with the only exception of the garbage collector having the power of removing them. How Kubernetes handle this problem for example? Do we know?

Thanks

Signed-off-by: Gaurav Gahlot <gauravgahlot0107@gmail.com>
@gauravgahlot
Copy link
Contributor Author

gauravgahlot commented Jan 4, 2021

AFAIK, if we delete a resource in Kubernetes we can still get its events using kubectl get event(including DELETED event). And after a TTL (default is 1hr I think) those events are gone.

I'm glad most of the work in the PR is around tests and not the fix itself. 😄
I was anyway going to extract the test commits to a separate PR (#409).

@gianarb
Copy link
Contributor

gianarb commented Jan 4, 2021

well done @gauravgahlot but now that you said how Kubernetes works I will take it as a confirmation that this is a feature we should not implement 👍

@invidian
Copy link
Contributor

invidian commented Jan 4, 2021

IMO Documenting the TTL for Tinkerbell events (whatever it is or whatever is planned) should be sufficient to resolve #396 👍

@gianarb
Copy link
Contributor

gianarb commented Jan 4, 2021

Great, I will close this for now, about doc, atm I don't know where to write about it! I have to figure it out

@gianarb gianarb closed this Jan 4, 2021
@gauravgahlot gauravgahlot deleted the purge-events branch January 4, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events are not cleaned up when workflow is removed
3 participants