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

Environment Load Order #282

Closed
waynerobinson opened this issue Dec 9, 2016 · 54 comments
Closed

Environment Load Order #282

waynerobinson opened this issue Dec 9, 2016 · 54 comments

Comments

@waynerobinson
Copy link
Contributor

We use Rails and our config data is loaded via Figaro into the environment.

In the latest version of Shoryuken, the shoryuken.yml file gets parsed/loaded before the Rails environment loads and Figaro has a chance to inject the extra options into ENV.

I haven't tested this with Dotenv, but I suspect it would have a similar problem if you were using environment variables in a shoryuken.yml file.

Can the Shoryuken::EnvironmentLoader#load_rails file be safely moved to the top of Shoryuken::EnvironmentLoader#setup_options?

@h3poteto
Copy link
Contributor

h3poteto commented Dec 9, 2016

hi @waynerobinson

Can the Shoryuken::EnvironmentLoader#load_rails file be safely moved to the top of Shoryuken::EnvironmentLoader#setup_options?

It is very difficult.
Because, shoryuken have to load config options before load Rails.
In the past, shoryuken loading order has been rearranged: #219
Because of that influence, shoryuken could not initialize with daemonize, so I open issue and fix: #249

What kind of options do you want to use in shoryuken.yml from figaro?.
If you want to use aws options, you can use configure_client and configure_server like this: #281 (comment)

@waynerobinson
Copy link
Contributor Author

But this breaks all forms of env-based config where the environment variables aren't set via an external process.

@waynerobinson
Copy link
Contributor Author

We set the queue names from env because our queues change based on environment.

@waynerobinson
Copy link
Contributor Author

Maybe the config file should be abandoned altogether for shoryuken-specific options. Or a separate config file for daemonizing options (which I see a lot).

@waynerobinson
Copy link
Contributor Author

By the way, where did the other issue come from? We'be been running the previous version of Shoryuken in production for a long time now and have encountered the issue adding a log file option is meant to solve. We just let systemd handle logging redirection from stdout/stderr. As well as handling running processes, etc.

@phstc
Copy link
Collaborator

phstc commented Dec 9, 2016

But this breaks all forms of env-based config where the environment variables aren't set via an external process.

@waynerobinson actually you can use ENV variables as @h3poteto suggested, just include an initializer in the list of required files like this:

Shoryuken.configure_server do |config|
  config.aws = {
    secret_access_key: ENV["AWS_SECRET_ACCESS_KEY"],
    access_key_id: ENV["AWS_ACCESS_KEY_ID"],
    region: ENV["AWS_REGION"]
  }
end

Shoryuken.configure_client do |config|
  config.aws = {
    secret_access_key: ENV["AWS_SECRET_ACCESS_KEY"],
    access_key_id: ENV["AWS_ACCESS_KEY_ID"],
    region: ENV["AWS_REGION"]
  }
end

WDYT?

I still have mixed feeling about aws-sdk initialization and Shoryuken, if we should keep this responsibility in Shoryuken or leave users to set that directly in the aws-sdk. Shoryuken is just a client using aws-sdk, I'm not sure how much it should abstract aws-sdk.

@waynerobinson
Copy link
Contributor Author

But it isn't the AWS config, but the Shoryuken config we want to use environment variables in. Especially the queue lists, but also our concurrency count differs in dev/prod to make it easier to diagnose issues.

We must be able to put per-environment environment variables in our Shoryuken file or Shoryuken config.

Either everything in Shoryuken should be configurable within configure_server or these parts of the daemon config should be specified elsewhere. Especially as we had never had a problem with the previous implementation in our production environment across multiple web services. I still don't understand what the issue is or why you're even daemonizing manually… this should most definitely be left up to the OS.

@waynerobinson
Copy link
Contributor Author

Another thing, aren't the log file and PID file meant to be set as command-line options?

@h3poteto
Copy link
Contributor

We can not use environment variables defined by Rails, but we can use normal environment variables, for example .bashrc and .bash_profile. Is not enough that?
If you want to switch queue option according to environment, you can use othere means than figaro (e.q. export QUEUE=..., .bashrc, direnv).

Queue option does not need to be kept secret, so we do not need to use environment variables.
I do not think it is necessary to support as a OSS product.

@waynerobinson
Copy link
Contributor Author

It's got nothing to do with secrecy, it's got to do with different options for various environments.

At the very least, if you're going to allow (or insist on) configuration of Shoryuken via the configure_server option, all the options should be settable there.

@waynerobinson
Copy link
Contributor Author

@h3poteto another note, the logfile option from the shoryuken.yml file doesn't appear to work in the EnvironmentLoader anyway. It appears you're referring to the incorrect variable in the loader.

@phstc
Copy link
Collaborator

phstc commented Dec 20, 2016

@waynerobinson I will try the logfile option. Just to clarify, you have it in the shoryuken.yml and you are not passing it thorough -l in the CLI?

@waynerobinson @h3poteto regarding the AWS setup, I planning to release a version 3.0.0, which will remove all AWS config from Shoryuken, so we would need to configure Aws using the SDK directly, not through Shoryuken, see this #293.

phstc pushed a commit that referenced this issue Dec 21, 2016
phstc pushed a commit that referenced this issue Dec 21, 2016
@phstc
Copy link
Collaborator

phstc commented Dec 21, 2016

@waynerobinson logfile fix: #296 🍻 Is there any other issue you found out so far?

@camkidman
Copy link

I also would be curious to set something up where the .yml file can load differently depending on the rails environment (without specifying it again in an ENV variable). Perhaps support can be added for something like this?

production:
  ... some config here ...
development:
  ... some other config here ...

@phstc
Copy link
Collaborator

phstc commented Dec 29, 2016

@camkidman The plan is to remove this AWS configuration from Shoryuken #293 and leverage that to the Aws.config (aws-sdk) directly.

@camkidman
Copy link

@phstc I actually meant the queues and shoryuken options. Here's a different example:

production:
  concurrency: 25
  delay: 25
  queues:
    - [production_queue]
development:
  concurrency: 5
  delay: 5
  queues:
    - [development_queue]

@phstc
Copy link
Collaborator

phstc commented Dec 29, 2016

@camkidman got it. I'm not sure if we can go with fixed environments like that, it can be very specific for each app.

One option is to have shoryuken.dev.yml and shoryuken.yml, then you decide which one to use when starting shoryuken -C ....

shoryuken.yml is also an ERB, so you could also:

  queues:
<% if production? %>
    - [production_queue]
<% else %>
    - [development_queue]
<% end %>

@camkidman
Copy link

I did try using <%= Rails.env %> in config/shoryuken.yml and it didn't seem to know how to handle Rails -- but that's another good suggestion. Perhaps I just change the run script on the server.

@tjsingleton
Copy link

Note: we are using dotenv in dev and this is working:

queues:
  - [<%= ENV.fetch('DEFAULT_JOB_QUEUE_NAME') %>, 2]

but you have to invoke it like: dotenv shoryuken -R -C config/shoryuken.yml which is setting up the environment before exec'ing shoryuken. I'm not sure figaro supports such a wrapper, a scan of it's source seems that it is tied to rails.

@waynerobinson
Copy link
Contributor Author

waynerobinson commented Jan 9, 2017 via email

@Kitton
Copy link

Kitton commented Jan 12, 2017

So looks like if you want to use Shoryuken with Figaro your better option is to rollback to Shoryuken, 2.0.11 for now

@phstc
Copy link
Collaborator

phstc commented Jan 12, 2017

@Kitton would setting Aws.config({ ... }) in an initializer work with Figaro?

@Kitton
Copy link

Kitton commented Jan 12, 2017

@phstc Sorry, I didn't express myself correctly.
If you want to name you queues dynamically depending on variables defined through Figaro, the only option I found is to rollback to Shoryuken, 2.0.11.
If there's a way to define queues in config/initializers/shoryuken.rb not in config/shoryuken.yml it will be great.

@phstc
Copy link
Collaborator

phstc commented Jan 12, 2017

@Kitton are your queues fixed, like a set of queue names in staging, another set in production or do they change dynamically? If they are fixed, you may use the shoryuken.dev.yml, shoryuken.prod.yml approach.

@phstc
Copy link
Collaborator

phstc commented Jan 12, 2017

@Kitton I'm not familiar with Figaro, but checking their docs, it seems that they have also Figaro.env, would that work in the shoryuken.yml instead of trying ENV?

@dimidd
Copy link

dimidd commented Feb 9, 2017

FWIW, I use a workaround similar to the one suggested by @phstc , but still looking for a better solution.
http://stackoverflow.com/questions/42123963/environment-specific-shoryuken-configuration

@phstc
Copy link
Collaborator

phstc commented Feb 20, 2017

@h3poteto I've naively moved load_rails to the first line in the setup_options and it seems to be working fine, I could start it with/without -R, with/without logfile. What's the issue again with this?

@waynerobinson
Copy link
Contributor Author

I think it had to do with specifying the log file via an environment variable rather than the command-line option.

Which I would suggest is an anti-pattern anyway as if you care this much about log files you should either specify it on the command-line during whatever startup script you're using, or just let the higher-level daemonizer do the logging (which is what we do).

@phstc
Copy link
Collaborator

phstc commented Feb 20, 2017

@waynerobinson I couldn't reproduce it, I tried to start it with -d, setting the log file with -l and in the shoryuken.yml, both worked fine.

@phstc
Copy link
Collaborator

phstc commented Feb 20, 2017

@waynerobinson sorry for taking long on this. I hope we can fix this soon.

@h3poteto
Copy link
Contributor

@phstc

I've naively moved load_rails to the first line in the setup_options and it seems to be working fine

I think it will work correctly, and it can resolve this issue. But, that changes revert #219. Do you think that there is no probolem?

phstc pushed a commit that referenced this issue Feb 20, 2017
@phstc phstc mentioned this issue Feb 20, 2017
@phstc
Copy link
Collaborator

phstc commented Feb 20, 2017

But, that changes revert #219. Do you think that there is no probolem?

@h3poteto hm could you check this #318? It isn't reverting #219. WDYT?

@h3poteto
Copy link
Contributor

h3poteto commented Feb 20, 2017

@phstc I read #318.
I think that #318 is still reverting #219.

#219 says to daemonize before loading Rails, so call daemonize before EnvironmentLoader#load_rails (At this time, EnvironmentLoader.load contains EnvironmentLoader#load_rails).
And now EnvironmentLoader.setup_options is called before daemonize (https://github.com/phstc/shoryuken/blob/af24203cf4fc80815e6efd278b008fcde0743600/lib/shoryuken/cli.rb#L34-L42), after that it call EnvironmentLoader#load which contains EnvironmentLoader#load_rails.

So, when calling EnvironmentLoader#load_rails at the beginning of EnvironmentLoader.setup_options in #318, load Rails before daemonize. I think that it is the same as reverting.

@eugeneius
Copy link
Contributor

@h3poteto is right - #318 will reintroduce the problems that #219 was intended to solve.

In case it isn't clear, there is a dependency cycle in the boot process:

We can't have all three of these things at the same time. No matter how we rearrange the order of operations, we have to give up one of them.

@waynerobinson
Copy link
Contributor Author

I'm not sure why the third is necessary when there is a command-line option for it and logging should largely be done to STDOUT/STDERR anyway and let the thing running it redirect appropriately.

@phstc
Copy link
Collaborator

phstc commented Feb 26, 2017

@eugeneius so Sidekiq does not support loading sidekiq.yml with Rails references when daemonize is on? I'm wondering if the way to fix this issue, would be to force the log a CLI param instead of reading from shoryuken.yml, so we load Rails after daemonization.

@phstc
Copy link
Collaborator

phstc commented Feb 26, 2017

ok, I've just confirmed that. Sidekiq does not seem to allow Rails in the sidekiq.yml as well.

So the options are:

  • a) Do nothing. As @eugeneius mentioned, there's no ✨ available. People using Figure/Dotenv should use ENV[...] in the shoryuken.yml and start shoryuken dotenv bundle exec shoryuken (same for figaro), so the ENVs will get set. Another alternative is to create configs per ENV shoryuken.development.yml.
  • b) Force logfile as a CLI param
  • c) Only initialize/configure log after the Rails initialization
    • c.1) Use some log buffer, log to it, once Rails is initialized, flush it to the supplied log file
    • c.2) Let it log to the SDTOUT until Rails is initialized

@phstc
Copy link
Collaborator

phstc commented Feb 26, 2017

@h3poteto @eugeneius @waynerobinson ☝️ any thoughts?

@waynerobinson
Copy link
Contributor Author

When it comes down to it, this was to solve one core problem:

How do you programmatically set the queues after Rails has loaded.

Sidekiq doesn't have this problem because it's super easy to keep a different dev & production Redis server around and configure it that way.

But with SQS, every developer needs their own SQS queue and, since the only place these can be set is in the shoryuken.yml file, if we can't easily make this programmatic, then we run into problems.

It's equally unreasonable to expect that the *only * configuration gem that is supposed by Shoryuken is Dotenv (and only because you can basically use it to modify the environment before it executes something else, rather than the currently running ruby process).

I vote to get rid of the shoryuken.yml file altogether… or keep if if people really want to put their logfiles in there (something that should be managed externally) or if people really want to have ruby daemonize their process (something that really, really should just be handled by a dedicated tool like SystemV, Upstart, or whatever).

If I can set queues somewhere like we setup the AWS config, my personal issues with this go away.

@phstc
Copy link
Collaborator

phstc commented Feb 26, 2017

But with SQS, every developer needs their own SQS queue and, since the only place these can be set is in the shoryuken.yml file, if we can't easily make this programmatic, then we run into problems.

@waynerobinson can't you use shoryuken.dev.yml?

I used to use <%= ENV['USER'] %>_queue_name before. So each developer could user their own queue. Can be an option too.

I vote to get rid of the shoryuken.yml file altogether… or keep if if people really want to put their logfiles in there (something that should be managed externally) or if people really want to have ruby daemonize their process (something that really, really should just be handled by a dedicated tool like SystemV, Upstart, or whatever).

I don't see the point of getting rid of it. I think there are other options for you problem as suggested above or just add something to Shoryuken to allow adding queues at runtime.

@phstc
Copy link
Collaborator

phstc commented Feb 26, 2017

@waynerobinson BTW, this:

# shoryuken_setup.rb
Shoryuken.configure_server do |config|
  config.queues << 'default'
  # you can use Rails in here, or anything initialized by Rails
end
bundle exec shoryuken -R -r ./shoryuken_setup.rb

worked just fine for me. It loaded Rails, then added the queue default at runtime.

Does that work for you?

@waynerobinson
Copy link
Contributor Author

It hasn't previously. Or however I attempted to do it via assumed API.

How do you specify priority with that syntax?

@phstc
Copy link
Collaborator

phstc commented Feb 26, 2017

How do you specify priority with that syntax?

@waynerobinson that's a great question. It would require a change in Shoryuken to make it easier, something like this Shoryuken.add_queue(queue_name, priority = 1). Without any change (using Shoryuken as is), you would need to:

Shoryuken.configure_server do |config|
  [
    ['default', 5],
    ['high', 10]
  ].each do |queue_name, priority|
    priority.times { config.queues << queue_name }
  end
end

Let me know if you were able to test it and if that works for you. So we can wrap that in a PR/Wiki.

@waynerobinson
Copy link
Contributor Author

I will check it out.

@h3poteto
Copy link
Contributor

How do you programmatically set the queues after Rails has loaded.

Sidekiq doesn't have this problem because it's super easy to keep a different dev & production Redis server around and configure it that way.

But with SQS, every developer needs their own SQS queue and, since the only place these can be set is in the shoryuken.yml file, if we can't easily make this programmatic, then we run into problems.

I agree with this opinion.
When I use Sidekiq, I prepare redis servers for each rails environment. And Sidekiq is desigend to do the setting of redis in initializer, so we can switch redis server for each rails environment very easy.

@phstc

It would require a change in Shoryuken to make it easier, something like this Shoryuken.add_queue(queue_name, priority = 1)

Therefore, I think it is very good solution.

@phstc
Copy link
Collaborator

phstc commented Feb 27, 2017

When I use Sidekiq, I prepare redis servers for each rails environment. And Sidekiq is desigend to do the setting of redis in initializer, so we can switch redis server for each rails environment very easy.

@h3poteto is it because for dev environments? I'm wondering if having something like the ActiveJob Inline Adapter natively in Shoryuken would help. Causing perform_later to be executed immediately in dev. Related discussion.

@h3poteto
Copy link
Contributor

h3poteto commented Feb 28, 2017

@phstc Oh, it looks very useful.

But, I assumed other cases which is not only development.
In my case, I have only one AWS account, and I operate staging and production in the account.

In Sidekiq:
If I use Sidekiq in staging and production, I have to create redis server for each environments. And, I can create two redis servers which have each endpoints in a AWS account. So, I can use same queue name regardless of the environments, because redis servers have discrete endpoints.

In Shoryuken:
If I use Shoryuken in staging and production, I have to create queues for each environments. Because, AWS SQS provides a single endpoint in a AWS account, like https://sqs.ap-northeast-1.amazonaws.com. So, I have to write environment name in queue(like: notification_staging, notification_production) to switch queue for each environments.

Therefore, I don't have to write environment name in queue and switch queue in Sidekiq, but it is required in Shoryuken.

phstc added a commit that referenced this issue Mar 6, 2017
@phstc
Copy link
Collaborator

phstc commented Mar 6, 2017

Fixed by #322

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

No branches or pull requests

10 participants