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

boot skips all seeding with env variable #14207

Merged
merged 1 commit into from Mar 14, 2017
Merged

Conversation

@kbrock
Copy link
Member

kbrock commented Mar 6, 2017

This is a follow-up to #6672

We need to reboot rails s dozens of times a day as we run performance tests, often in production mode.

On appliance startup, the environment variable SKIP_PRIMORDIAL_SEEDSKIP_SEEDING is consulted when booting the appliance to skip seeding the database.

Using a docker db, local rails s takes 18s. Using this flag takes it down to 9s (50% savings).

For evm server, it was only skipping the first half of the seeding. This PR skips the second half of seeding

/cc @jrafanie @Fryguy @chrisarcand same actors as last round.

@jrafanie

This comment has been minimized.

Copy link
Member

jrafanie commented Mar 7, 2017

On appliance startup, the environment variable SKIP_PRIMORDIAL_SEED is consulted when booting the appliance to skip seeding the database. [

@kbrock Are you saying the appliance skips primordial seed by default? I don't see it being set here

Is this setting only for performance and development testing?

@kbrock

This comment has been minimized.

Copy link
Member Author

kbrock commented Mar 7, 2017

@jrafanie that setting is only used for performance and development
It is a hidden lever that would allow high latency customers to boot up.

Copy link
Contributor

isimluk left a comment

👍 I am tentatively liking it.

@jrafanie

This comment has been minimized.

Copy link
Member

jrafanie commented Mar 10, 2017

@kbrock My only problem is this SKIP_PRIMORDIAL_SEED would basically skip all seeding if you have it set and the MiqDatabase has been seeded. It makes the env variable name wrong. 😢

@kbrock

This comment has been minimized.

Copy link
Member Author

kbrock commented Mar 13, 2017

@jrafanie Ok. As it stands, the environment variable will skip the primordial seeding but not skip the other seeding. Not good.

This PR is a minimal change to fix the problem and buy us time before we properly seed the database. Not on every evm start.

@jrafanie

This comment has been minimized.

Copy link
Member

jrafanie commented Mar 13, 2017

@jrafanie Ok. As it stands, the environment variable will skip the primordial seeding but not skip the other seeding. Not good.

@kbrock yeah, but it's called SKIP_PRIMORDIAL_SEED, that's what it does. If you change it to do all seeds, the variable needs to be renamed. If you want to have a ENV to skip it all, create SKIP_ALL_SEED right or we'll have to rename it. I'm not sure how many places know about the existing variable. Maybe just rename variable.

@chessbyte

This comment has been minimized.

Copy link
Member

chessbyte commented Mar 13, 2017

Maybe the simpler SKIP_SEEDING will do

@jrafanie

This comment has been minimized.

Copy link
Member

jrafanie commented Mar 13, 2017

Maybe the simpler SKIP_SEEDING will do

hehe, naming is hard. Great idea, @chessbyte

@kbrock

This comment has been minimized.

Copy link
Member Author

kbrock commented Mar 13, 2017

thanks. I was told that was too short ;) #6672 (comment) 🚌

@jrafanie

This comment has been minimized.

Copy link
Member

jrafanie commented Mar 13, 2017

@kbrock maybe because that's when we were just skipping primordial but not the rest. Maybe it's fine if we skip all of the seeding if MiqDatabase is present?

We should go with WE_STINK_AND_OUR_SEEDS_ARE_SLOW from here

😆

@kbrock kbrock force-pushed the kbrock:seed_server branch Mar 14, 2017
@kbrock

This comment has been minimized.

Copy link
Member Author

kbrock commented Mar 14, 2017

@chessbyte thanks. changed name to SKIP_SEEDING

Any other changes?

Copy link
Member

jrafanie left a comment

LGTM, just squash the commits unless you think they stand alone... and update the description @kbrock

@kbrock kbrock changed the title skip remaining seeding as well boot skips all seeding with env variable Mar 14, 2017
with environment variable.

It was just skipping half the seeds.

rename SKIP_PRIMORDIAL_SEED to SKIP_SEEDING
@kbrock kbrock force-pushed the kbrock:seed_server branch to 31241cd Mar 14, 2017
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Mar 14, 2017

Some comments on commit kbrock@31241cd

lib/evm_database.rb

  • ⚠️ - 48 - Detected puts. Remove all debugging statements.
  • ⚠️ - 49 - Detected puts. Remove all debugging statements.
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Mar 14, 2017

Checked commit kbrock@31241cd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 2 offenses detected

lib/evm_database.rb

  • ❗️ - Line 48, Col 7 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
  • ❗️ - Line 49, Col 7 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.
@jrafanie jrafanie merged commit e7cf728 into ManageIQ:master Mar 14, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0002%) to 44.397%
Details
kbrock added a commit to kbrock/manageiq that referenced this pull request Mar 16, 2017
commit 31241cd (PR ManageIQ#14207 )
no longer seeds the remaining data.

swapped the if to seed it by default
@kbrock kbrock mentioned this pull request Mar 16, 2017
Fryguy pushed a commit to ManageIQ/manageiq-schema that referenced this pull request Jun 20, 2017
commit 31241cd37 (PR ManageIQ/manageiq#14207 )
no longer seeds the remaining data.

swapped the if to seed it by default
(transferred from ManageIQ/manageiq@37c433d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.