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

Migrate with cleared schema cache #401

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

jrafanie
Copy link
Member

Alternative to #398

Wrap each migration with a clearing of the schema cache to ensure the
schema/column information is not cached. Since migrations very often
change schema, these caches are likely to get busted.

Migration authors don't realize they may need to reset the column
information whenever they change the schema so it's often overlooked.
It often doesn't cause a problem, until it does:

20180424141617_azure_backslash_to_forward_slash.rb
 - loads stub: class OrchestrationStack < ActiveRecord::Base
 - orchestration_stacks table column information is cached by table name

20181010134649_add_evm_owner_to_orchestration_stacks.rb
 - adds evm_owner, miq_group, tenant references (_id columns)

20181016140921_migrate_orch_stacks_to_have_ownership_concept.rb
 - loads stub: class OrchestrationStack < ActiveRecord::Base
 - uses cached table column information
 - Blows up trying to store bigint into what rails thinks is an integer column:
   stack.update_attributes(:evm_owner_id => user.id, :tenant_id =>
   user.current_tenant.id, :miq_group_id => user.current_group.id)
   '34000000000001 is out of range for ActiveModel::Type::Integer with
   limit 4 bytes'

This commit eliminates the need to reset column information for any of
these migrations as we clear the schema cache for all tables before each
migration.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1732808

@jrafanie jrafanie force-pushed the migrate_with_cleared_schema_cache branch 2 times, most recently from 365cbd6 to 456ee9e Compare July 30, 2019 20:06
@jrafanie
Copy link
Member Author

@Fryguy @carbonin @d-m-u I believe this consolidates all of our ideas into one concept: clear the schema information before running migrate...

@jrafanie
Copy link
Member Author

yay it failed even though it didn't locally... looking...

@jrafanie jrafanie changed the title Migrate with cleared schema cache [WIP] Migrate with cleared schema cache Jul 31, 2019
@miq-bot miq-bot added the wip label Jul 31, 2019
@jrafanie jrafanie force-pushed the migrate_with_cleared_schema_cache branch from 2e7af20 to 0a7e9ec Compare August 1, 2019 15:14
@jrafanie jrafanie changed the title [WIP] Migrate with cleared schema cache Migrate with cleared schema cache Aug 1, 2019
@jrafanie
Copy link
Member Author

jrafanie commented Aug 1, 2019

Ok, finally, fixed. The complexities with resetting column information in migration tests are still needed around each example because the migration stub gets reused in tests. I verified this works in migration specs and also with a test database that was raising the 34000000000001 is out of range for ActiveModel::Type::Integer with limit 4 bytes error.

@jrafanie
Copy link
Member Author

jrafanie commented Aug 6, 2019

@Fryguy please review

@jrafanie jrafanie force-pushed the migrate_with_cleared_schema_cache branch from 0a7e9ec to 7fd57b0 Compare August 7, 2019 00:57
Wrap each migration with a clearing of the schema cache to ensure the
schema/column information is not cached.  Since migrations very often
change schema, these caches are likely to get busted.

Migration authors don't realize they may need to reset the column
information whenever they change the schema so it's often overlooked.
It often doesn't cause a problem, until it does:

20180424141617_azure_backslash_to_forward_slash.rb
 - loads stub: class OrchestrationStack < ActiveRecord::Base
 - orchestration_stacks table column information is cached by table name

20181010134649_add_evm_owner_to_orchestration_stacks.rb
 - adds evm_owner, miq_group, tenant references (_id columns)

20181016140921_migrate_orch_stacks_to_have_ownership_concept.rb
 - loads stub: class OrchestrationStack < ActiveRecord::Base
 - uses cached table column information
 - Blows up trying to store bigint into what rails thinks is an integer column:
   stack.update_attributes(:evm_owner_id => user.id, :tenant_id =>
   user.current_tenant.id, :miq_group_id => user.current_group.id)
   '34000000000001 is out of range for ActiveModel::Type::Integer with
   limit 4 bytes'

This commit eliminates the need to reset column information for any of
these migrations as we clear the schema cache for all tables before each
migration.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1732808
@jrafanie jrafanie force-pushed the migrate_with_cleared_schema_cache branch from 7fd57b0 to cdfce63 Compare August 7, 2019 15:14
@jrafanie
Copy link
Member Author

jrafanie commented Aug 7, 2019

@carbonin @bdunne I think I addressed all of the concerns. 🍪

@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2019

Checked commit jrafanie@cdfce63 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@jrafanie
Copy link
Member Author

jrafanie commented Aug 7, 2019

Note, I've retested the problematic database down and up using the "reset cache only before migrate" since I previously tested "before and after migrate".

@bdunne bdunne merged commit 44bbd51 into ManageIQ:master Aug 7, 2019
@bdunne bdunne self-assigned this Aug 7, 2019
@bdunne bdunne added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 7, 2019
@jrafanie jrafanie deleted the migrate_with_cleared_schema_cache branch August 7, 2019 16:15
simaishi pushed a commit that referenced this pull request Aug 7, 2019
@simaishi
Copy link
Contributor

simaishi commented Aug 7, 2019

Ivanchuk backport details:

$ git log -1
commit ed95ecf9b91de89c4de30702d2efc81ba0a5438d
Author: Brandon Dunne <bdunne@redhat.com>
Date:   Wed Aug 7 12:01:45 2019 -0400

    Merge pull request #401 from jrafanie/migrate_with_cleared_schema_cache
    
    Migrate with cleared schema cache
    
    (cherry picked from commit 44bbd51ddb5a8566ebe5c75da4c4175d726e0774)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1732808

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants