Remove soft-deletion pattern #2327

nyanshak · 2020-10-11T22:32:52Z

Perform migration to delete any entries with deleted set, and
subsequently drop columns deleted and deleted_at.
Remove deleted and deleted_at references.

nyanshak · 2020-10-11T22:33:07Z

Addresses #2146

nyanshak · 2020-10-11T22:38:56Z

@zwass I took a stab at solving #2146 here. It's still a bit of a work in progress, but I could use some help with the migrations since I haven't worked with that before.

I'm following the instructions in docs/development/database-migrations.md, but having a bit of trouble.

When I run ./build/fleet prepare db, my migration file is called (added debug prints in init), but the Up function doesn't seem to ever be called.

Could you take a look and see if I'm doing something wrong?

zwass

Thank you for tackling this!

server/datastore/mysql/migrations/data/20171212182459_DeleteSoftDeletedPacks.go

server/datastore/mysql/migrations/data/20181119180000_DeleteSoftDeletedEntities.go

zwass · 2020-10-14T16:11:52Z

server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go

+)
+
+func init() {
+	goose.AddMigration(Up20201011162341, Down20201011162341)


This needs to be MigrationClient.AddMigration. That will solve your issue with the migration not running. Do we need to update the docs about this?

The docs say to run goose create AddColumnFooToUsers, then 'edit it', but is less clear on what needs to be done.

I see two things that must be edited (as far as I can tell) each time you run the goose create command:

delete the import line for goose: github.com/pressly/goose

change goose.AddMigration(...) to `MigrationClient.AddMigration(...)

I can add a note to the docs to fix both of these if you'd like.

It would also be good to have the goose create command generate this file as close to 'correctly' as possible, so it might be worth updating the goose library to do this.

Also while I was looking into this, I was a bit confused on what the kolide goose fork did, but it seemed like the fork hasn't been updated since 2018, while the pressly source is still actively maintained. Long-term - should we consider moving back to pressly upstream?

server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go

zwass

It seems there are a couple of test failures that need to be updated.

In regards to your comment about Goose (Github no longer seems to allow me to reply to that comment), please do update the documentation with what you found.

We had initially forked Goose for at least one reason: We needed to be able to make the separate migration "clients" for tables and data, and needed to refactor Goose from using a single, global, migration client. I cannot remember if there were other motivations for the fork.

I did look recently at the activity on Goose and did not get the impression that the core had changed in a way that would allow us to achieve the goal of separate clients. That said, I do think it could be worthwhile to try to rebase the fork onto the new changes.

In regards to making the command generate a migration closer to what we need, that definitely seems worthwhile.

server/datastore/mysql/migrations/tables/20201011162341_CleanupSoftDeletedColumns.go

server/datastore/mysql/migrations/data/20171212182459_DeleteSoftDeletedPacks.go

* Perform migration to delete any entries with `deleted` set, and subsequently drop columns `deleted` and `deleted_at`. * Remove `deleted` and `deleted_at` references.

* Change `goose.AddMigration` to `MigrationClient.AddMigration` * If columns don't exist, skip instead of failing the migration. This might happen in the case of a partial or broken migration.

* Specify changes that need to be made after running the goose create command.

nyanshak · 2020-10-19T18:40:45Z

One of the test failures is when deleting hosts. There is an existing test for testIdempotentDeleteHost that creates a host and tries to delete it twice. In the 'soft-deleted' pattern, this was fine (just set deleted to true each time). There are lots of places that call deleteEntity but DeleteHost was the only one with a test expecting this to be idempotent.

I'm not sure what the reasoning is behind that, but I'm going to put up a commit that removes this test, as deleting is not actually idempotent any more. Give me a shout if this doesn't work.

* Without soft-delete, host deleting is no longer idempotent. Removing the test for this.

* Replace definitions with the mysqlerr package constants instead of defining them here.

* Return appropriate error message when attempting to insert a query by ID that already exists in the DB.

zwass · 2020-10-19T20:05:27Z

I am totally on board with removing tests that no longer make sense. Thank you. I will give this some more review and testing when I have a chance.

zwass

I just pushed a commit to remove the debug prints. Thank you for sorting this out!

Changes in kolide/fleet#2327 broke the MySQL syntax for listing hosts with online status. This was not caught due to the lack of a unit test for the functionality. This PR adds a unit test and fixes the regression.

This is another error introduced in kolide/fleet#2327 we did not catch previously due to insufficient unit test coverage. Test is now added

This is another error introduced in kolide/fleet#2327 we did not catch previously due to insufficient unit test coverage. Test is now added.

zwass suggested changes Oct 14, 2020

View reviewed changes

zwass added Component: Databases Component: Technical Debt labels Oct 16, 2020

zwass suggested changes Oct 19, 2020

View reviewed changes

nyanshak added 3 commits October 19, 2020 12:53

Remove soft-deletion pattern

835d3df

* Perform migration to delete any entries with `deleted` set, and subsequently drop columns `deleted` and `deleted_at`. * Remove `deleted` and `deleted_at` references.

Fix migration and skip non-existent columns

a8faf66

* Change `goose.AddMigration` to `MigrationClient.AddMigration` * If columns don't exist, skip instead of failing the migration. This might happen in the case of a partial or broken migration.

Update migration docs

2c4cac6

* Specify changes that need to be made after running the goose create command.

nyanshak added 3 commits October 19, 2020 13:52

Remove test for idempotency with host delete

6070814

* Without soft-delete, host deleting is no longer idempotent. Removing the test for this.

Use VividCortex/mysqlerr constants

6b576ad

* Replace definitions with the mysqlerr package constants instead of defining them here.

Fix duplicate query insert error message

102ea1b

* Return appropriate error message when attempting to insert a query by ID that already exists in the DB.

nyanshak force-pushed the issue-2146-remove-soft-deletion branch from 5e13345 to 102ea1b Compare October 19, 2020 18:55

Remove debug prints

2149457

zwass approved these changes Oct 21, 2020

View reviewed changes

zwass changed the title ~~WIP: Remove soft-deletion pattern~~ Remove soft-deletion pattern Oct 21, 2020

Merge branch 'master' into issue-2146-remove-soft-deletion

f9ec739

zwass merged commit c6b285c into kolide:master Oct 22, 2020

zwass mentioned this pull request Oct 22, 2020

Fix #2021: Allow users to be deleted #2123

Open

zwass mentioned this pull request Nov 4, 2020

Fix regression in list hosts fleetdm/fleet#6

Merged

zwass added a commit to zwass/fleet that referenced this pull request Nov 5, 2020

Fix regression creating packs

5c4c20b

This is another error introduced in kolide/fleet#2327 we did not catch previously due to insufficient unit test coverage. Test is now added

zwass mentioned this pull request Nov 5, 2020

Fix regression creating packs fleetdm/fleet#17

Merged

zwass added a commit to fleetdm/fleet that referenced this pull request Nov 5, 2020

Fix regression creating packs (#17)

a1720db

This is another error introduced in kolide/fleet#2327 we did not catch previously due to insufficient unit test coverage. Test is now added.

zwass mentioned this pull request Nov 18, 2020

Unexpected migration warning message when upgrading to 3.3.0 fleetdm/fleet#48

Closed

zwass mentioned this pull request Dec 3, 2020

Cannot add same query to pack more than once (results in 5xx) fleetdm/fleet#88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove soft-deletion pattern #2327

Remove soft-deletion pattern #2327

nyanshak commented Oct 11, 2020

nyanshak commented Oct 11, 2020

nyanshak commented Oct 11, 2020

zwass left a comment

zwass Oct 14, 2020

nyanshak Oct 16, 2020 •

edited

Loading

zwass left a comment

nyanshak commented Oct 19, 2020

zwass commented Oct 19, 2020

zwass left a comment

Remove soft-deletion pattern #2327

Remove soft-deletion pattern #2327

Conversation

nyanshak commented Oct 11, 2020

nyanshak commented Oct 11, 2020

nyanshak commented Oct 11, 2020

zwass left a comment

Choose a reason for hiding this comment

zwass Oct 14, 2020

Choose a reason for hiding this comment

nyanshak Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

zwass left a comment

Choose a reason for hiding this comment

nyanshak commented Oct 19, 2020

zwass commented Oct 19, 2020

zwass left a comment

Choose a reason for hiding this comment

nyanshak Oct 16, 2020 •

edited

Loading