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

Production migration fail due to cached schema for model #8209

Merged
merged 1 commit into from May 2, 2016

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Apr 25, 2016

Production migration fails due to cached schema for model. Model
contains old schema, so the field parent_ems_id, that we created
couple of migrations back, will not be in the model
ExtManagementSystem and it will fail on create!. Seems like
this appears only on production env, where the schema is not
reloaded. Also this occurs only when we do e.g. upgrade, so
when migration has some data, before we continue with other
migrations. This migration is data migration touching OpenStack.

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

Production migration fails due to cached schema for model. Model
contains old schema, so the field parent_ems_id, that we created
couple of migrations back, will not be in the model
ExtManagementSystem and it will fail on create!. Seems like
this appears only on production env, where the schema is not
reloaded. Also this occurs only when we do e.g. upgrade, so
when migration has some data, before we continue with other
migrations. This migration is data migration touching OpenStack.

Fixes BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1329137
@miq-bot
Copy link
Member

miq-bot commented Apr 25, 2016

Checked commit Ladas@fd2430f with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 🍰

@Ladas
Copy link
Contributor Author

Ladas commented Apr 25, 2016

@Fryguy I was talking with QE guys about having this tested. So we could run nightly tests on upgraded envs. Not sure if we can do some tests in CI?

@Ladas Ladas closed this Apr 25, 2016
@Ladas Ladas reopened this Apr 25, 2016
@Fryguy
Copy link
Member

Fryguy commented Apr 25, 2016

@Ladas This should not be needed, so I would really like to understand this. I think this might be a bug in the migration_spec_helper.rb in not properly clearing caches now that we are on Rails 5. We had a similar problem in #8167

cc @lpichler @jrafanie

@jrafanie
Copy link
Member

related: #7476

rails/rails 24300 (merged from @chrisarcand)

From the BZ:

== 20160226092911 SeparateOpenstackNetworkManagerFromOpenstackCloudManager: migrating 
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

unknown attribute 'parent_ems_id' for SeparateOpenstackNetworkManagerFromOpenstackCloudManager::ExtManagementSystem./opt/rh/cfme-gemset/bundler/gems/rails-efaa6e4f79d4/activemodel/lib/active_model/attribute_assignment.rb:48:in `_assign_attribute'

It looks like this PR's migration is the next migration... the thing that's interesting is the use of the stub model in the error. I wonder if ExtManagementSystem column information is cleared but not the stub model's?

@chrisarcand
Copy link
Member

😕 Yeah, something is borked with caches. I think this might indeed be a Rails 5 issue (or how we handle it given Rails 5), likely due to the new attributes API OR something is just borked with how the spec helper references the correct model to clear the cache.

What bothers me is that we can't seem to narrow down why it suddenly becomes a problem in some cases. I'd expect that if the caching is stuck from migration to migration we would have seen all these failures of models trying to access non-existent attributes right away.

I'll try and look into this first thing in the morning, if @jrafanie or @Fryguy don't get to it first.

TBH if this is something that needs fixing ASAP in a production environment I don't think manually clearing the schema/model/adapter caches (AR has so many caches...) in the migrations as here is a bad idea. It shouldn't be required, true, but seems to clear as expected.

@Ladas
Copy link
Contributor Author

Ladas commented Apr 26, 2016

This definitively fixes a current issue where we can't upgrade from previous version, with OpenStack provider present, in rails Production env. If we can do something that will solve this globally, it would be awesome. :-)

@Fryguy
Copy link
Member

Fryguy commented Apr 26, 2016

I don't mind merging this, but I'm sensing a new pattern of "migration fails, add some cache clearing", which I'm concerned will be a never-ending battle.

@chrisarcand
Copy link
Member

I'm sensing a new pattern of "migration fails, add some cache clearing", which I'm concerned will be a never-ending battle.

+1, something's gotta be overlooked in the spec helper, glancing now.

@@ -33,6 +33,8 @@ def affected_classes

def up
# Separate NetworkManager from CloudManager and move network models under NetworkManager
ExtManagementSystem.connection.schema_cache.clear!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ladas is this line needed? I thought only the reset_column_information was needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrafanie not sure, I took this from other migration which has this used :-)

@jrafanie jrafanie self-assigned this Apr 29, 2016
@gtanzillo gtanzillo added this to the Sprint 40 Ending May 9, 2016 milestone May 2, 2016
@gtanzillo
Copy link
Member

👍 LGTM

@gtanzillo gtanzillo merged commit 95da1ba into ManageIQ:master May 2, 2016
chessbyte pushed a commit that referenced this pull request May 2, 2016
Production migration fail due to cached schema for model
(cherry picked from commit 95da1ba)
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.

None yet

7 participants