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

THREESCALE-10697: Add debug mode for the worker and add RubyMine configurations for both worker and listener #363

Merged
merged 2 commits into from
Feb 29, 2024

Conversation

lvillen
Copy link
Contributor

@lvillen lvillen commented Feb 5, 2024

What this PR does / why we need it
This PR adds a debug mode for the worker and suggest RubyMine configs for both worker and listeners so developers can easily configure them in their RubyMine scripts. It comes from the request in #352, which suggested to create some scripts, but simplifies the process (thanks @mayorova).

Verification steps
Following the proposed configurations at DEVELOPMENT.md create the three RubyMine configurations.

Then launch both worker a listener in debug mode with a breaking point and confirm it's working.

@lvillen lvillen requested a review from jlledom February 5, 2024 15:25
@lvillen lvillen self-assigned this Feb 5, 2024
@mayorova
Copy link
Contributor

mayorova commented Feb 6, 2024

To be honest, I don't see much value in explicit scripts for listeners.
The developer needs to configure the Run Configurations in the IDE anyway, and they can just add the already existing Ruby script (bin/3scale_backend), and Script arguments, I don't think that's much more effort for a one-time setup. And it's also easier to change if the dev wants to, for example, configure the listener on another port.

Alternatively, we could add .idea/runConfigurations directory to git, and it would have the configurations pre-defined.

This is how for example the configuration looks like for falcon listener:

<component name="ProjectRunConfigurationManager">
  <configuration default="false" name="listener (falcon)" type="RubyRunConfigurationType" factoryName="Ruby">
    <module name="apisonator" />
    <RUBY_RUN_CONFIG NAME="RUBY_ARGS" VALUE="" />
    <RUBY_RUN_CONFIG NAME="WORK DIR" VALUE="$MODULE_DIR$" />
    <RUBY_RUN_CONFIG NAME="SHOULD_USE_SDK" VALUE="false" />
    <RUBY_RUN_CONFIG NAME="ALTERN_SDK_NAME" VALUE="" />
    <RUBY_RUN_CONFIG NAME="myPassParentEnvs" VALUE="true" />
    <envs>
      <env name="LISTENER_WORKERS" value="1" />
      <env name="RACK_ENV" value="development" />
    </envs>
    <EXTENSION ID="BundlerRunConfigurationExtension" BUNDLE_MODE="AUTO" bundleExecEnabled="true" />
    <EXTENSION ID="RubyCoverageRunConfigurationExtension" track_test_folders="true" runner="rcov" ENABLE_BRANCH_COVERAGE="true" ENABLE_FORKED_COVERAGE="true">
      <COVERAGE_PATTERN ENABLED="true">
        <PATTERN REGEXPS="/.rvm/" INCLUDED="false" />
      </COVERAGE_PATTERN>
    </EXTENSION>
    <EXTENSION ID="org.jetbrains.plugins.ruby.rails.run.RailsRunConfigurationExtension" SCRATCH_USE_RAILS_RUNNER="false" />
    <RUBY_RUN_CONFIG NAME="SCRIPT_PATH" VALUE="$MODULE_DIR$/bin/3scale_backend" />
    <RUBY_RUN_CONFIG NAME="SCRIPT_ARGS" VALUE="-s falcon start -p 3001" />
    <method v="2" />
  </configuration>
</component>

@mayorova
Copy link
Contributor

mayorova commented Feb 6, 2024

For worker, indeed debugging doesn't work without an extra script, because 3scale_backend_worker demonizes the process, and even with --no-daemonize argument, daemonizing is simulated (IO streams get closed, and I guess this is what makes the debugger shutdown.

But IMO, rather than creating another script, just for the debugging purposes, I'd introduce a new argument in 3scale_backend_worker, say --debug, which will skip deamonizing. Something along the lines of the following (maybe there is a better way, and in any case that would need to be documented):

if ARGV.delete '--debug'
  ThreeScale::Backend::Worker.work
else
  Daemons.run_proc('3scale_backend_worker', options) do
    ThreeScale::Backend::Worker.work
  end
end

And then in the run configuration we'd just have /bin/3scale_backend_worker in Ruby script, with script arguments --debug.

@mayorova
Copy link
Contributor

mayorova commented Feb 6, 2024

So, in conclusion, rather than creating extra scripts, I would just document properly how to start the listener and worker processes locally, also in a way that is suitable for debugging (for example, making sure that only one worker/thread is listening in the listener).

I think it's good to not only be able to run the app as quickly as possible, but also understand to some extent what happens behind the scenes.

@jlledom
Copy link
Contributor

jlledom commented Feb 6, 2024

@mayorova booo spoilsport!! 👎👎

I agree on reuse as much as possible and not create new scripts if existing ones can be reused. However:

  1. Running the worker script as non-root fails at line 25 due to access denied to /var/run/3scale. That should be corrected.
  2. Simply calling ThreeScale::Backend::Worker.work is not enough for sentinels, you need to run it in a loop to simulate the failover that Kubernetes provides in production
  3. You probably need to wrap ThreeScale::Backend::Worker.work under a Sync/Async call. I'm not sure all calls to Redis during the worker execution happen under a reactor. If there's only one that doesn't, it will fail without a general reactor. So better add it.
  4. I imported the falcon launcher to my .idea/runConfigurations folder and it doesn't work for me. I get the error:
You have already activated json 2.5.1, but your Gemfile requires json 2.3.1. Since json is a default gem, you can either remove your dependency on it or try updating to a newer version of bundler that supports json as a default gem. (Gem::LoadError)

I also see that error with @lvillen scripts. That's because both scripts end up calling system or exec which create a new process and causes that error.

@mayorova
Copy link
Contributor

mayorova commented Feb 6, 2024

Thanks for your comments @jlledom

Running the worker script as non-root fails at line 25 due to access denied to /var/run/3scale. That should be corrected.

Yes, this is true, I forgot about it, because at some point I created that folder manually 😅
I'd say this should also be fixed in 3scale_backend_worker itself, IMO it's not good to hardcode such a path, I think it should be configurable (it's OK to keep the current path as default).

Simply calling ThreeScale::Backend::Worker.work is not enough for sentinels, you need to run it in a loop to simulate the failover that Kubernetes provides in production

But is it only to simulate failovers? I mean, if the dev is not working on the Redis failover feature specifically, would they need it?

I remembered now your point also that it is nice to catch all exceptions and print errors in the logs and keep the worker running instead of shutting down on exceptions... But - is it a common scenario? And also - wouldn't it "mask" some incorrect behavior? (I mean if the dev just misses something, because the worker keeps running, so "looks OK")

You probably need to wrap ThreeScale::Backend::Worker.work under a Sync/Async call. I'm not sure all calls to Redis during the worker execution happen under a reactor. If there's only one that doesn't, it will fail without a general reactor. So better add it.

Shouldn't it rather be fixed by this PR? https://github.com/3scale/apisonator/pull/361/files#diff-2f501e81093c9eaa568d634cc294ab565cada69ebbc59570588c314b0f159cfe

Here (as of today) we opted for not adding a global reactor. As I understand it, currently the production runs just the 3scale_backend_worker run script, with no global reactor wrapping (unlike falcon-based listener, where, if I understand correctly, falcon takes care of creating the needed reactor).

So, if we add it just in the development script, then what would happen in production?

I imported the falcon launcher to my .idea/runConfigurations folder and it doesn't work for me. I get the error:

Hm... interesting, not sure why this would happen, I can't reproduce.

@jlledom
Copy link
Contributor

jlledom commented Feb 7, 2024

Simply calling ThreeScale::Backend::Worker.work is not enough for sentinels, you need to run it in a loop to simulate the failover that Kubernetes provides in production

But is it only to simulate failovers? I mean, if the dev is not working on the Redis failover feature specifically, would they need it?

Yeah, maybe you're right.

You probably need to wrap ThreeScale::Backend::Worker.work under a Sync/Async call. I'm not sure all calls to Redis during the worker execution happen under a reactor. If there's only one that doesn't, it will fail without a general reactor. So better add it.

Shouldn't it rather be fixed by this PR? https://github.com/3scale/apisonator/pull/361/files#diff-2f501e81093c9eaa568d634cc294ab565cada69ebbc59570588c314b0f159cfe

That PR should ensure there's always a reactor for the worker tasks. OK, let's remove the Async and consider adding it back if we found a scenario where it fails.

So, if we add it just in the development script, then what would happen in production?

Sure, the more closer to production behavior, the better.

bin/3scale_backend_worker Outdated Show resolved Hide resolved
bin/3scale_backend_worker Outdated Show resolved Hide resolved
@jlledom
Copy link
Contributor

jlledom commented Feb 14, 2024

With the changes from #364 I can launch successfully the listener and the worker in both sync ans async mode. Only two issues:

  1. The listener in sync mode (puma) shows me a deprecation warning:
[DEPRECATED] `Bundler.with_clean_env` has been deprecated in favor of `Bundler.with_unbundled_env`. If you instead want the environment before bundler was originally loaded, use `Bundler.with_original_env` (called at /home/jlledom/.asdf/installs/ruby/3.0.2/lib/ruby/gems/3.0.0/bundler/gems/puma-e7d3c53583db/lib/puma/launcher.rb:289)
  1. The listener in sync mode (puma) doesn't stop in breakpoints when being launched in debug mode.

Don't you see this errors? This is my sync listener launcher:

image

@lvillen lvillen force-pushed the THREESCALE-10697_scripts-for-worker-and-listener branch from 3e943b1 to d9fe7fe Compare February 15, 2024 11:48
bin/3scale_backend_worker Outdated Show resolved Hide resolved
@lvillen
Copy link
Contributor Author

lvillen commented Feb 15, 2024

  1. The listener in sync mode (puma) shows me a deprecation warning:
[DEPRECATED] `Bundler.with_clean_env` has been deprecated in favor of `Bundler.with_unbundled_env`. If you instead want the environment before bundler was originally loaded, use `Bundler.with_original_env` (called at /home/jlledom/.asdf/installs/ruby/3.0.2/lib/ruby/gems/3.0.0/bundler/gems/puma-e7d3c53583db/lib/puma/launcher.rb:289)

I don't see this deprecation warning. After the json upgrade I did a bundle clean and after the bundle install everything seems ok.

  1. The listener in sync mode (puma) doesn't stop in breakpoints when being launched in debug mode.

I've put a breakpoint in backend.rb:56 and it is stopping.

Also, I've integrated your suggestions @jlledom in this last commit. I'm going to share here my configs hoping it helps:

Worker
Captura de pantalla 2024-02-15 a las 12 53 42

Listener (puma)
Captura de pantalla 2024-02-15 a las 12 53 59

Listener (falcon)
Captura de pantalla 2024-02-15 a las 12 54 13

@jlledom
Copy link
Contributor

jlledom commented Feb 16, 2024

It all works for me now 👍

@mayorova
Copy link
Contributor

  1. The listener in sync mode (puma) doesn't stop in breakpoints when being launched in debug mode.

Don't you see this errors? This is my sync listener launcher:

@jlledom The important piece here is -X "-w 0" in the arguments, it starts puma in so-called "single mode". Compare the logs without this arg:

[3736875] Puma starting in cluster mode...
[3736875] * Version 4.3.9 (ruby 3.0.2-p107), codename: Mysterious Traveller
[3736875] * Min threads: 1, max threads: 1
[3736875] * Environment: development
[3736875] * Process workers: 1
[3736875] * Phased restart available
[3736875] * Listening on tcp://0.0.0.0:3001
[3736875] Use Ctrl-C to stop
[3736875] * Starting control server on unix:///home/dmayorov/Projects/3scale/apisonator/3scale_backend.sock

and with it:

Puma starting in single mode...
* Version 4.3.9 (ruby 3.0.2-p107), codename: Mysterious Traveller
* Min threads: 1, max threads: 1
* Environment: development
* Listening on tcp://0.0.0.0:3001
* Starting control server on unix:///home/dmayorov/Projects/3scale/apisonator/3scale_backend.sock
Use Ctrl-C to stop

In the second case (with "-s ="), the breakpoints work fine for me. Also, in this case LISTENER_WORKERS env var doesn't make any difference, so no need to include it in the run configuration.

@mayorova
Copy link
Contributor

The changes in bin/3scale_backend_worker look good to me. I don't know what @akostadinov thinks 😬

But the PR title and description need some changes too. Also, I would like to see the RubyMine configurations somewhere, maybe in DEVELOPMENT.md file.

@jlledom
Copy link
Contributor

jlledom commented Feb 16, 2024

@jlledom The important piece here is -X "-w 0" in the arguments, it starts puma in so-called "single mode". Compare the logs without this arg:

Yes, it's working now, thanks!

In the second case (with "-s ="), the breakpoints work fine for me. Also, in this case LISTENER_WORKERS env var doesn't make any difference, so no need to include it in the run configuration.

"-s =" ?

@mayorova
Copy link
Contributor

"-s =" ?

I meant "-w 0" 😅

@jlledom
Copy link
Contributor

jlledom commented Feb 22, 2024

I think we can merge this one, right @lvillen @mayorova

@mayorova
Copy link
Contributor

I think we can merge this one, right @lvillen @mayorova

I would like to see my comments here addressed first 😇

@lvillen lvillen changed the title THREESCALE-10697: Add scripts to launch both listener and worker THREESCALE-10697: Add debug mode for both listener and worker Feb 27, 2024
@lvillen lvillen changed the title THREESCALE-10697: Add debug mode for both listener and worker THREESCALE-10697: Add debug mode for the worker and add RubyMine configurations for both worker and listener Feb 27, 2024
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
@lvillen lvillen force-pushed the THREESCALE-10697_scripts-for-worker-and-listener branch from fd035bb to c8ec79c Compare February 27, 2024 16:11
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
@lvillen lvillen force-pushed the THREESCALE-10697_scripts-for-worker-and-listener branch from c8ec79c to 53e0f82 Compare February 28, 2024 09:41
@lvillen lvillen requested review from jlledom and mayorova February 28, 2024 09:41
Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

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

LGTM, just a suggestion.

DEVELOPMENT.md Outdated Show resolved Hide resolved
@lvillen lvillen force-pushed the THREESCALE-10697_scripts-for-worker-and-listener branch from 53e0f82 to e8b30af Compare February 28, 2024 11:35
@lvillen lvillen merged commit 51825f1 into master Feb 29, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

4 participants