Soft delete instead of disconnect for containers models #18

Ladas · 2017-06-23T06:41:12Z

Soft delete instead of disconnect for containers models

Ladas · 2017-06-23T06:42:21Z

Fryguy · 2017-06-23T15:25:44Z

db/migrate/20170530102506_add_deleted_to_containers_tables.rb

+class AddDeletedToContainersTables < ActiveRecord::Migration[5.0]
+ def change
+ add_index :container_definitions, :deleted_on,
+ :name => "index_container_definitions_on_deleted_on"


Can you remove the name and let Rails use the default name?

might be that the default name will pass the length limit now, will do

Fryguy · 2017-06-23T15:25:55Z

db/migrate/20170530102506_add_deleted_to_containers_tables.rb

+ add_index :container_definitions, :deleted_on,
+ :name => "index_container_definitions_on_deleted_on"
+ add_index :container_groups, :deleted_on,
+ :name => "container_groups_on_deleted_on"


Does this one not have the "index" prefix intentionally?

fixed by autonaming

Fryguy · 2017-06-23T15:26:36Z

db/migrate/20170530102536_use_deleted_in_containers_tables.rb

+
+ say_with_time("Change ':deleted_on not nil' :ems_id to :old_ems_id for Container") do
+ disconnect_to_soft_delete(Container)
+ end


Minor, but you can massively DRY this up with a loop on the models

I know, I was too lazy. @lpichler has said the same thing :-D

Fryguy · 2017-06-23T15:27:57Z

Code-wise, looks good to me. I want a 👍 from @simon3z before merging, though.

Fryguy · 2017-06-23T15:28:47Z

@Ladas Would it make sense to squash commits here? For example, you have commits in the middle about partial indexes, but you don't actually have any partial indexes in the end.

simon3z · 2017-06-23T15:34:05Z

Code-wise, looks good to me. I want a from @simon3z before merging, though.

I was waiting for @cben to take a look at ManageIQ/manageiq#15250 but probably shouldn't block on that.

+1 for letting Rails use the default name for the indexes.

Overall LGTM 👍

cben · 2017-06-25T10:49:07Z

spec/migrations/20170530102536_use_deleted_in_containers_tables_spec.rb

+ def assert_after_migration_data_of(model)
+ expect(model.where.not(:deleted_on => nil).count).to eq 3
+ expect(model.where(:deleted_on => nil).count).to eq 2
+ expect(model.where(:ems_id => nil).count).to eq 1


consider also testing :ems_id => 20 and/or :ems_id => 10 counts.

Ladas · 2017-07-03T14:52:53Z

@Fryguy @simon3z ok, will squash it as this seems the final version. Will default and dry it up. :-)

Add indexes to :deleted_on cols for Container tables

Use soft delete instead of disconnect

Test data migration of disconnect to soft delete

Ladas · 2017-07-04T07:35:49Z

@simon3z @Fryguy @cben done, done and done. :-D Added also the ContainerNode model.

Fix rubocop issues

cben

Neat 👍

cben · 2017-07-04T10:04:46Z

db/migrate/20170704102536_use_deleted_in_containers_tables.rb

+ end
+
+ def disconnect_to_soft_delete(model)
+ model.where.not(:deleted_on => nil).update_all("ems_id = old_ems_id")


@zeari @zakiva @simon3z
Is there any scenario where a record could have ems_id set and old_ems_id == nil, but deleted_on set?
Then this would make a bad situation worse, setting ems_id = old_ems_id = nil.

It "shouldn't happen", I just remember we saw ems_id == old_ems_id == nil in a customer's DB, and suspected it might have resulted from double disconnect overwriting old_ems_id, so trying to think along similar lines...

Same question for soft_delete_to_disconnect.

@zeari @zakiva @simon3z
Is there any scenario where a record could have ems_id set and old_ems_id == nil, but deleted_on set?
Then this would make a bad situation worse, setting ems_id = old_ems_id = nil.

@cben technically there shouldn't be but I suppose it depends on whether it was possible that a crash left unfinished the updates (if multiple ones and not atomic). Also you're never 100% sure of not having had bugs.

If there's a better logic that we could use to avoid possible mistakes then let's try to improve this (maybe we should use model.where.not(:deleted_on => nil, :ems_id => nil).update_all("ems_id = old_ems_id") to be on the safe side?).

so doing this, before doing update_all("ems_id = old_ems_id")

model.where.not(:deleted_on => nil).where.not(:ems_id => nil).update_all(:deleted_on => nil)

so having :ems_id will mean the record is not disconnected

I wonder if I should also clean up the case where :ems_id is nil but :deleted_on is also nil? Putting Time.now into deleted_on?

For :deleted_on => nil, :ems_id => nil, I am filling deleted_on with a date

Correct naming of the migrations

Clean up deleted_on for non diconected records

Cover :deleted_on => nil, :ems_id => nil case by filling deleted_on

miq-bot · 2017-07-12T11:59:17Z

Checked commits Ladas/manageiq-schema@dda9800~...539e757 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 6 offenses detected

db/migrate/20170704102536_use_deleted_on_in_containers_tables.rb

❗ - Line 24, Col 53 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.
❗ - Line 25, Col 67 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.
❗ - Line 26, Col 41 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.
❗ - Line 30, Col 41 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

spec/migrations/20170704102536_use_deleted_on_in_containers_tables_spec.rb

❗ - Line 33, Col 7 - Style/IdenticalConditionalBranches - Move expect(model.where.not(:deleted_on => nil).count).to eq 4 out of the conditional.
❗ - Line 42, Col 7 - Style/IdenticalConditionalBranches - Move expect(model.where.not(:deleted_on => nil).count).to eq 4 out of the conditional.

cben · 2017-07-12T14:20:27Z

Sorry, I've been reviewing another migration today, and my brain is refusing to concentrate 🤕

But yeah, looks excellent (thanks for separate commits). In both directions, you make deleted_on consistent before you act based on it. 👍

Ladas · 2017-07-13T11:23:54Z

@Fryguy @simon3z this should be ready

ems_id always set since we switched to soft delete. Backfilled for old records too by ManageIQ/manageiq-schema#18.

miq-bot assigned Fryguy Jun 23, 2017

Ladas mentioned this pull request Jun 23, 2017

Soft delete instead of disconnect for containers models migrations ManageIQ/manageiq#15250

Closed

Fryguy reviewed Jun 23, 2017

View reviewed changes

cben approved these changes Jun 25, 2017

View reviewed changes

Ladas mentioned this pull request Jul 3, 2017

Soft delete instead of disconnect for containers models ManageIQ/manageiq#15251

Merged

Fryguy mentioned this pull request Jul 3, 2017

Archive Container Nodes #22

Merged

Ladas added 3 commits July 4, 2017 09:30

Add indexes to :deleted_on cols for Container tables

dda9800

Add indexes to :deleted_on cols for Container tables

Use soft delete instead of disconnect

249f7ec

Use soft delete instead of disconnect

Test data migration of disconnect to soft delete

0f8511b

Test data migration of disconnect to soft delete

Ladas force-pushed the soft_delete_instead_of_disconnect_for_containers_models branch from 2136910 to 0f8511b Compare July 4, 2017 07:32

Fix rubocop issues

f55e807

Fix rubocop issues

cben reviewed Jul 4, 2017

View reviewed changes

Ladas added 3 commits July 12, 2017 11:57

Correct naming of the migrations

ec7338c

Correct naming of the migrations

Clean up deleted_on for non diconected records

58812dc

Clean up deleted_on for non diconected records

Cover :deleted_on => nil, :ems_id => nil case by filling deleted_on

539e757

Cover :deleted_on => nil, :ems_id => nil case by filling deleted_on

simon3z approved these changes Jul 13, 2017

View reviewed changes

Fryguy merged commit 9ac5bfb into ManageIQ:master Jul 17, 2017

Fryguy added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 17, 2017

Fryguy added the enhancement label Jul 17, 2017

chessbyte added data index labels Jul 25, 2017

cben mentioned this pull request Oct 26, 2017

Add deleted_on (with index) to container_quotas, container_quota_items #110

Merged

cben mentioned this pull request Dec 27, 2017

Get rid of old_ems_id ManageIQ/manageiq-providers-kubernetes#196

Closed

7 tasks

cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request Jul 4, 2018

Stop falling back to old_ems_id

6722278

ems_id always set since we switched to soft delete. Backfilled for old records too by ManageIQ/manageiq-schema#18.

cben mentioned this pull request Jul 4, 2018

Stop falling back to old_ems_id ManageIQ/manageiq-providers-kubernetes#267

Merged

cben added a commit to cben/manageiq that referenced this pull request Jul 11, 2018

Chargeback: stop falling back to old_ems_id

299e50c

ems_id always set since we switched to soft delete. Backfilled for old records too by ManageIQ/manageiq-schema#18.

cben added a commit to cben/manageiq that referenced this pull request Jul 11, 2018

Stop using OldEmsMixin to fall back to old_ems_id

a486b34

ems_id always set since we switched to soft delete. Backfilled for old records too by ManageIQ/manageiq-schema#18.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Soft delete instead of disconnect for containers models #18

Soft delete instead of disconnect for containers models #18

Ladas commented Jun 23, 2017

Ladas commented Jun 23, 2017

Fryguy Jun 23, 2017

Ladas Jul 3, 2017

Ladas Jul 4, 2017

Fryguy Jun 23, 2017

Ladas Jul 4, 2017

Fryguy Jun 23, 2017

Ladas Jul 3, 2017 •

edited

Loading

Ladas Jul 4, 2017

Fryguy commented Jun 23, 2017

Fryguy commented Jun 23, 2017

simon3z commented Jun 23, 2017 •

edited

Loading

cben Jun 25, 2017

Ladas commented Jul 3, 2017

Ladas commented Jul 4, 2017

cben left a comment

cben Jul 4, 2017

simon3z Jul 4, 2017

Ladas Jul 12, 2017 •

edited

Loading

Ladas Jul 12, 2017

Ladas Jul 12, 2017

miq-bot commented Jul 12, 2017

cben commented Jul 12, 2017

Ladas commented Jul 13, 2017

Soft delete instead of disconnect for containers models #18

Soft delete instead of disconnect for containers models #18

Conversation

Ladas commented Jun 23, 2017

Ladas commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jun 23, 2017

Fryguy commented Jun 23, 2017

simon3z commented Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Ladas commented Jul 3, 2017

Ladas commented Jul 4, 2017

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jul 12, 2017

cben commented Jul 12, 2017

Ladas commented Jul 13, 2017

Ladas Jul 3, 2017 •

edited

Loading

simon3z commented Jun 23, 2017 •

edited

Loading

Ladas Jul 12, 2017 •

edited

Loading