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
Bundler error prevents phased-restarting puma after upgrading to rails 7 #2843
Comments
@nateberkopec Could you please give us some guidance on how to investigate it? How can we debug certain puma instances and check why they don't |
@joker-777 Without an app that reproduces the issue we can't really help you, so building a minimal app which shows the issue (even intermittently) would be the place to start. |
Hey @nateberkopec 👋 . I work with @joker-777. We completely understand the need to reproduce it. We can reproduce it some times on our staging environment, but we so far weren't able to pinpoint what triggers it, nor able to reproduce it in a sterile test-app environment unfortunately. Our app has many moving parts, so pairing it down is difficult. In order to try to understand what triggers the problem, we were wondering if there's a way for us to somehow "query" or debug the puma workers state in some way. For example: be able to see the full path of the Gemfile and code that the worker has loaded. We bumped into similar problems in the past, see #2018 and #2471, so my hunch is that it's somehow related to having folders with symlinks and bundler losing its orientation somehow, but unfortunately the error in the puma.log doesn't have much information to go with. Hopefully if we can gather a bit more info from the puma workers, we can then get better clues to help us reproduce it with a sample app. |
This error logging was added here: 04e4b1a We could increase what's logged there, but I don't think that would help you, because you've already zeroed in on the problem. I'm not sure what to say, you can tell me what info you want and we can add it (or you can fork, try adding the info you want, and then we can add it if it's useful)? |
We're still bumping into this, yet it's hard to reproduce reliably, even on our staging environment. One thing that definitely jumps out however is that the With this patch in place, we can see this in the puma.log
When we run We keep investigating but wanted to post what we found so far. |
side-note: would you be happy with a PR to better log when worker fails? at the moment it only logs the first item from the backtrace, and not even the error itself. |
I think it makes sense to have better logging when this happens |
@nateberkopec we found a workaround, and also managed to reproduce the issue hopefully. The workaround for us was to install The repro can be found at https://github.com/gingerlime/puma-phased-restart-bundler-error |
std-lib extension gems have been an issue for Bundler and also Puma. Possibly the solution is to lock std-lib extension gems (used with Puma) to the version included with the Ruby used, and if the Ruby does not have the gem, install it locally as you suggested? |
I'm not sure I fully understand, but obviously I'm not familiar with Bundler or Puma at an intimate level. In particular why the problem only surfaces with phased-restart, after clearing one of the original folders. It doesn't happen when we load puma initially. Did you get a chance to try out the repro? it's complaining about not being able to find |
No yet, but soon. |
Ok, ran it, updating to use Puma master. First of all, has this worked with previous versions of Puma/Rails/Bundler? Anyway, you might review info on phased-restart/SIGUSR1. A phased-restart cannot reload Puma, and your code is deleting the Puma files (calling Re the issue I referred to previously, think of running Note that one can delete old releases, just not the original one that started Puma. I.think. |
We definitely delete old releases, including the one that started puma and it works. Unfortunately, the repro doesn't exactly have the same error with Perhaps @cjlarose can chime in? he worked on a similar problem with phased-restart and I kinda based my repro on his original one, albeit with Rails rather than just rack + some tweaks to bundler to more closely match what we do on our staging and live environments. |
I just noticed that when we run I'm still very confused about the bundler behaviour here though. Especially when using |
Sorry, AFK. I'll look more this weekend. io/wait was added Jul 25, 2021, v5.40, in #6e4257fece, so it's been around for a while. I think deployment switches to |
Thank you @MSP-Greg ! I was referring to the rails io-wait dependency rather than puma, but as far as I can tell there’s no explicit dependency in puma, just relying on stdlib, right? |
Puma gemspec:
There is no io-wait gem available for Ruby 2.2, as first release (0.1.0) was '>= 2.3.0'. So, we can't add an explicit dependency. Maybe next major release... |
Since These all depend on
So when you upgraded From https://rubygems.org/gems/rails/versions
I think one should restart Puma when applying dependencies changes, like upgrading Rails. |
So since Puma v5.4.0 (#2666) the Puma master process depend on |
I'm not sure which part of the documentation explains this particular case? if It's definitely a tricky question, and I still have very little understanding of bundler and puma, but I still think it's a question worth asking. At the very least also recommend how to use bundler when using phased restarts? (e.g. perhaps it's not safe to use |
What is documented is
Since Puma v5.4.0 this includes If we had known about this edge-case, perhaps we would have been hesitant to make the change #2666. But it this issue also exist because Ruby is moving many parts of the standard library to gems... so I guess we theoretically could end up in this situation again down the road. I feel it is hard to get phased restarts 100% correct. I think would pick another deployment strategy if it was my app, or at least only use it when updating application code. |
Thank you. I appreciate it.
Yes, I'm starting to think the same. We're still running a simple app, so not ready to go to Kubernetes etc, yet most of the heroku-like services out there abstract a bit too much. Anything docker-based could be interesting, but docker-compose/swarm doesn't have a solid zero-downtime deploy option as far as I can tell. Any tips would be extremely valuable :) |
One thing that may help with these issues would be to log all extensions loaded in the main Puma process on startup? Not sure how easy that would be, nor how long the list would be... Also, we could vendor the methods being used from io-wait, but that might be a PITA given all the Ruby versions that are currently supported. |
I was thinking that too, but also not sure exactly how to do that
Yeah, I don't think we should do that. It is hard enough already to maintain Puma, we should try to avoid making it even harder. |
Agreed. It would probably work almost all of the time, but sooner or later it would break, and that would be a mad dash to update it... Re the 'loaded extensions', maybe something like the below? We may be able to only use io-wait when nio4r is loaded, which for clustered servers, would remove the problem with io-wait. I.think. It might be messy. Single mode:
Cluster mode:
EDIT: Above were generated without io-wait in Gemfile.lock |
I looked at Rails, and it also loads So, it looks like Rails may have decided that 'gemified' std-lib extensions are not listed as gem dependencies, so they just use a |
So action item here is we need to explicitly mark our iowait dependency, right? @MSP-Greg: re the 'loaded extensions', I'm in favor of adding that logging behind some kind of "verbose mode" switch (idk what switches we already have which would be appropriate). |
Ok. I'll look at the current switches, may add one. For everyone, see PR #2903. In Ruby master, the io/wait methods being used in Puma have been moved into the native Ruby IO class. So, many years from now, when Ruby 3.2 is the Puma minimum required Ruby version... |
Behind environment variable |
That, or maybe |
Maybe it's time to have a debug ENV variable that can be a set of values that can be split? Would allow Puma to have several types of 'debug' logging, and one would just combine what options one wanted? Not sure what everyone thinks of that, and what to name it... |
I vote for keeping it simple |
How is that done? Add https://rubygems.org/gems/io-wait as runtime dependency in gemspec? But with Ruby 3.2 it wouldn't be needed, or even be a conflict? I extracted the other actionable item here to its own issue: #3020 If we should change the gemspec, we should create a new issue about that. It could link to this issue for background. I'll close this issue now. |
Describe the bug
We saw two different errors
which should have raised this kind of error
raise GemNotFound, "Could not find #{missing_specs.map(&:full_name).join(", ")}" in any of the sources"
and
which should have raised this kind of error
Gem::LoadError.new "You have already activated #{activated_spec.name} #{activated_spec.version}, but your Gemfile requires #{spec.name} #{spec.version}. #{suggestion}"
Why didn't we see these error messages in the puma logs and how can we fix them?
Puma config:
To Reproduce
/usr/bin/bundle exec pumactl -F config/puma.rb phased-restart
Expected behavior
phased-restart should restart all puma instances correctly
Desktop (please complete the following information):
The text was updated successfully, but these errors were encountered: