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

fix update phased restart symlink folder #3295

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nikhilbhatt
Copy link

Description

Fixes #3290

During phased restart updated the tag name with folder base name if tag is not set in puma config.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@dentarg
Copy link
Member

dentarg commented Dec 22, 2023

Any idea how we could test this?

@nikhilbhatt nikhilbhatt force-pushed the fix_phased_restart_update_destination_folder branch from 6fad6b8 to d32e670 Compare December 23, 2023 05:18
@nikhilbhatt
Copy link
Author

nikhilbhatt commented Dec 23, 2023

Any idea how we could test this?

If you are asking about testing the branch, here how I done it

Temporary changes

  1. cd to puma latest version.
  2. Create a folder releases/1 and releases/2.
  3. Copy file hello.ru to both the releases and update the content if needed.
  4. create link ln -s releases/1/ current
  5. Update settings.rb file and add following
    workers 2
    directory 'path to current'
    prune_bundler
    
  6. run bundle exec bin/puma -C "absolute path to test/config/settings.rb" hello.ru
  7. grep for puma
  8. unlink current
  9. ln -s releases/2/ current
  10. kill -SIGUSR1 puma_pid
  11. again grep for puma to view updated changes.

for 2nd testing update settings.rb with additional tag testing.
when you repeat above steps there should be no change in tags.

If you are asking about writing test cases for this
I am looking into this.

@@ -142,4 +145,10 @@ def with_unbundled_env
Bundler.with_unbundled_env { yield }
end
end

Copy link
Author

Choose a reason for hiding this comment

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

@dentarg

Added testcases in TestWorkerGemIndependence. not sure if it should be in separate or new class.

also used grep here as I did not find any alternative to get the title of Process running. we are using setproctitle to set the title but there is no equivalent to this for get.

@nikhilbhatt
Copy link
Author

@dentarg can you check this once?

@dentarg
Copy link
Member

dentarg commented Dec 27, 2023

I'll check whenever, no stress in open source

@nikhilbhatt
Copy link
Author

Is there anything remaining in this from my side?

@dentarg dentarg added the waiting-for-review Waiting on review from anyone label Jan 28, 2024
@nikhilbhatt nikhilbhatt force-pushed the fix_phased_restart_update_destination_folder branch from 6629294 to 558b048 Compare January 28, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Phased restart & symlinks doesn't change destination folder?
2 participants