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

Remove seed on rails server start in development. #19535

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Nov 19, 2019

In a production setting, evmserverd will be resposible for seeding the
database. In a development setting, we should not be seeding when we
start the UI. Developers should instead be encouraged to use bin/update
or call db:seed directly when changes are made. Since most developers
need to call bin/update before starting a rails server, in order to
build the UI assets, this is part of the normal workflow.

This change should eliminate about 1 second at rails server boot in
development.

Some more:

  • Seeding the database will change records in the database, and if pointing to someone else's database (say a customer database or a testing database), then we are modifying the target environment, making diagnostics harder by dirtying the source.
  • Seeding the database is a blocking operation as it takes a lock on critical tables. While in development this doesn't really matter much, it's better to remove it to avoid that.

@Fryguy
Copy link
Member Author

Fryguy commented Nov 19, 2019

cc @jrafanie @kbrock

lib/vmdb/initializer.rb Outdated Show resolved Hide resolved
@jrafanie jrafanie requested a review from himdel November 19, 2019 18:25
@jrafanie
Copy link
Member

cc @h-kataria @himdel I'm not sure if anyone was depending on rails s automatically seeding the primordial tables but if so, they'd need to rake db:migrate db:seed OR bin/update whenever they pull new code

@Fryguy
Copy link
Member Author

Fryguy commented Nov 19, 2019

cc @himdel @skateman @h-kataria @martinpovolny

I'd like your input here and make sure I'm not making too many assumptions, particularly since this might affect the flow of UI developers.

In my mind this is the way it always should have been (I was actually surprised to find we did any seeding at all when calling rails s)

lib/vmdb/initializer.rb Outdated Show resolved Hide resolved
@h-kataria
Copy link
Contributor

@jrafanie i have always been running seeding manually either by running rake db:migrate db:seed or just running commands in rails console to seed any specific models

@Fryguy
Copy link
Member Author

Fryguy commented Nov 19, 2019

@jrafanie Marking as WIP cause I'm changing it as discussed above in #19535 (comment) , with tests.

@Fryguy Fryguy changed the title Remove seed on rails server start in development. [WIP] Remove seed on rails server start in development. Nov 19, 2019
@miq-bot miq-bot added the wip label Nov 19, 2019
@Fryguy Fryguy force-pushed the no_seed_on_rails_s branch from 5d3bd76 to fe2f02a Compare November 19, 2019 22:49
@Fryguy
Copy link
Member Author

Fryguy commented Nov 19, 2019

@jrafanie Updated. Thoughts?

@Fryguy Fryguy force-pushed the no_seed_on_rails_s branch from fe2f02a to c62c582 Compare November 19, 2019 22:50
lib/evm_database.rb Outdated Show resolved Hide resolved
@jrafanie
Copy link
Member

@jrafanie Updated. Thoughts?

This makes sense to me.

I mentioned in chat that I believe the original logic for automatically seeding primordial was added to rails server processes back when bundler might have been the only dependency management we had to do frequently. Back then, the UI workflow was update your code, bundle update, rake the database, and run rails server.

Now that bin/update is required often with javascript library dependency changes, you can't get around seeding properly as bin/update does the seeding for you.

@Fryguy Fryguy changed the title [WIP] Remove seed on rails server start in development. Remove seed on rails server start in development. Nov 20, 2019
@Fryguy Fryguy force-pushed the no_seed_on_rails_s branch 2 times, most recently from eeac334 to 97a7929 Compare November 20, 2019 02:30
@miq-bot miq-bot removed the wip label Nov 20, 2019
@skateman
Copy link
Member

I'm good with this, but we might want to focus on the remaining 9 seconds :trollface: 😆

@Fryguy
Copy link
Member Author

Fryguy commented Nov 20, 2019

@skateman I'll update the OP, but there's a few other reasons for this...

  • Seeding the database will change records in the database, and if pointing to someone else's database (say a customer database or a testing database), then we are modifying the target environment, making diagnostics harder by dirtying the source.
  • Seeding the database is a blocking operation as it takes a lock on critical tables. While in development this doesn't really matter much, it's better to remove it to avoid that.

@jrafanie
Copy link
Member

I'm good with this, but we might want to focus on the remaining 9 seconds :trollface: 😆

@skateman Want to talk routes with me? I can give you all the fun details

@skateman
Copy link
Member

Let's do that when I'm in Mahwah!

@himdel
Copy link
Contributor

himdel commented Nov 20, 2019

@Fryguy I'm seeing one potentially big issue... MiqDatabase.seed sets session_secret_token and csrf_secret_token.

If we only do primordial seed once on the appliance, those tokens are not really secure anymore, right? (Unless they are supposed to stay the same during the whole lifetime..)

EDIT: not seeing any other issues 👍

@skateman
Copy link
Member

@himdel do we care about that in development?

@himdel
Copy link
Contributor

himdel commented Nov 20, 2019

Not really, but does this only affect development though?

@himdel
Copy link
Contributor

himdel commented Nov 20, 2019

BTW we're also seeding region twice .. Zone.seed calls MiqRegion.seed

@himdel
Copy link
Contributor

himdel commented Nov 20, 2019

This also means changing the GUID file will need a bin/update / db:seed. (I'm fine with that.)

@Fryguy
Copy link
Member Author

Fryguy commented Nov 20, 2019

BTW we're also seeding region twice .. Zone.seed calls MiqRegion.seed

There are a number of these interdependencies...definitely needs to be fixed / cleaned up.

@Fryguy
Copy link
Member Author

Fryguy commented Nov 20, 2019

Not really, but does this only affect development though?

Yes this only affects development. In production, evmserverd is doing all of the seeding up front, and then we use forking, which bypasses Vmdb::Initializer.init

@Fryguy Fryguy force-pushed the no_seed_on_rails_s branch from 97a7929 to 36ab6ec Compare November 20, 2019 20:21
In a production setting, evmserverd will be resposible for seeding the
database.  In a development setting, we should not be seeding when we
start the UI.  Developers should instead be encouraged to use bin/update
or call db:seed directly when changes are made.  Since most developers
need to call bin/update before starting a rails server, in order to
build the UI assets, this is part of the normal workflow.

This change should eliminate about 1 second at rails server boot in
development.
@Fryguy Fryguy force-pushed the no_seed_on_rails_s branch from 36ab6ec to 56fab2e Compare November 20, 2019 20:26
@miq-bot
Copy link
Member

miq-bot commented Nov 20, 2019

Checked commit Fryguy@56fab2e with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 1 offense detected

app/models/mixins/miq_web_server_worker_mixin.rb

@kbrock
Copy link
Member

kbrock commented Nov 20, 2019

Can we drop the primordial concept and just say seeded?

@Fryguy
Copy link
Member Author

Fryguy commented Nov 21, 2019

Can we drop the primordial concept and just say seeded?

Maybe but not in this PR 😉

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM, I'll merge it tomorrow morning

@jrafanie jrafanie merged commit 0c596c4 into ManageIQ:master Nov 22, 2019
@jrafanie jrafanie added this to the Sprint 125 Ending Nov 25, 2019 milestone Nov 22, 2019
@Fryguy Fryguy deleted the no_seed_on_rails_s branch December 2, 2019 21:08
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