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.

Sign up for GitHub

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

Jump to bottom

rgw/lc: advance head if the current entry doesn't exist #56486

Merged
merged 2 commits into from Apr 25, 2024
Merged

rgw/lc: advance head if the current entry doesn't exist #56486

merged 2 commits into from Apr 25, 2024

Conversation

soumyakoduri
Copy link
Contributor

This is an extension to #47595.

When skipping the non-exist LC entry, instead of get_next_entry(), use advance_head() to fetch the next entry. In case the cycle is finished for that shard, head should be reset to avoid lc process being stuck in indefinite loop.

Fixes: https://tracker.ceph.com/issues/65160
Signed-off-by: Soumya Koduri skoduri@redhat.com

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@soumyakoduri soumyakoduri requested a review from a team as a code owner March 26, 2024 19:13
@github-actions github-actions bot added the rgw label Mar 26, 2024
@soumyakoduri soumyakoduri requested a review from dang March 26, 2024 19:15
@mattbenjamin
Copy link
Contributor

mattbenjamin commented Mar 27, 2024 •
edited

the one thing I notice is that the larger flow of try-advance-head; try-proceed, potentially-handle-shard-rollover is now duplicated in full, so it's quite a bit of duplicated code that should behave identically; I would not insist, but is it possible that we could write an inline function that incorporates that?

@soumyakoduri
Copy link
Contributor Author

the one thing I notice is that the larger flow of try-advance-head; try-proceed, potentially-handle-shard-rollover is now duplicated in full, so it's quite a bit of duplicated code that should behave identically; I would not insist, but is it possible that we could write an inline function that incorporates that?

yes even I was thinking about it but was sceptical to do this at end of the release. I guess we can cleanup the code in the upstream at least

@mattbenjamin
Copy link
Contributor

that seems reasonable, sure, @soumyakoduri

@soumyakoduri soumyakoduri force-pushed the wip-skoduri-lc-stall branch 2 times, most recently from 5e06a92 to ce1d002 Compare April 12, 2024 06:47
@soumyakoduri soumyakoduri removed the request for review from a team April 12, 2024 06:48
@soumyakoduri
Copy link
Contributor Author

the one thing I notice is that the larger flow of try-advance-head; try-proceed, potentially-handle-shard-rollover is now duplicated in full, so it's quite a bit of duplicated code that should behave identically; I would not insist, but is it possible that we could write an inline function that incorporates that?

I included inline functions. Please review.

mattbenjamin
mattbenjamin approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

looks good!

@soumyakoduri
rgw/lc: advance head if the current entry doesn't exist
86a8709 
This is extension to ceph#47595.

When skipping the non-exist LC entry, instead of get_next_entry(),
use advance_head() to fetch the next entry. In case the cycle
is finished for that shard, head should be reset to avoid lc process
being stuck in indefinite loop.

Fixes: https://tracker.ceph.com/issues/65160
Signed-off-by: Soumya Koduri <skoduri@redhat.com>
@soumyakoduri
rgw/lc: cleanup duplicate code
e2b3a30 
Cleaning up duplicate code around updating head

Signed-off-by: Soumya Koduri <skoduri@redhat.com>
@soumyakoduri
Copy link
Contributor Author

http://pulpito.front.sepia.ceph.com/soumyakoduri-2024-04-23_17:17:50-rgw:cloud-transition-wip-skoduri-lc-stall-distro-default-smithi

http://pulpito.front.sepia.ceph.com/soumyakoduri-2024-04-23_17:18:17-rgw:lifecycle-wip-skoduri-lc-stall-distro-default-smithi

http://pulpito.front.sepia.ceph.com/soumyakoduri-2024-04-23_17:18:46-rgw-wip-skoduri-lc-stall-distro-default-smithi

@soumyakoduri
Copy link
Contributor Author

@yuvalif .. I see an abort in notifications test - http://qa-proxy.ceph.com/teuthology/soumyakoduri-2024-04-23_17:18:46-rgw-wip-skoduri-lc-stall-distro-default-smithi/7670058/teuthology.log

Is this known issue? I think its not related to this change. Can you please confirm?

@soumyakoduri
Copy link
Contributor Author

jenkins test make check arm64

@soumyakoduri
Copy link
Contributor Author

jenkins test api

@soumyakoduri
Copy link
Contributor Author

jenkins test windows

@yuvalif
Copy link
Contributor

yuvalif commented Apr 25, 2024

@yuvalif .. I see an abort in notifications test - http://qa-proxy.ceph.com/teuthology/soumyakoduri-2024-04-23_17:18:46-rgw-wip-skoduri-lc-stall-distro-default-smithi/7670058/teuthology.log

Is this known issue? I think its not related to this change. Can you please confirm?

yes: https://tracker.ceph.com/issues/64184

@cbodley cbodley merged commit 6ac316b into ceph:main Apr 25, 2024
11 checks passed
@cbodley
Copy link
Contributor

cbodley commented Apr 25, 2024

@mattbenjamin @soumyakoduri there was a request for squid backport in https://tracker.ceph.com/issues/65160. should it go to reef/quincy also? cc @jzhu116-bloomberg

@soumyakoduri
Copy link
Contributor Author

@mattbenjamin @soumyakoduri there was a request for squid backport in https://tracker.ceph.com/issues/65160. should it go to reef/quincy also? cc @jzhu116-bloomberg

Thanks Casey. I will backport it to squid.
If needed for reef/quincy, we need to backport https://github.com/ceph/ceph/pull/47595/files (#57162) as well then.

@jzhu116-bloomberg
Copy link
Contributor

@mattbenjamin @soumyakoduri there was a request for squid backport in https://tracker.ceph.com/issues/65160. should it go to reef/quincy also? cc @jzhu116-bloomberg

No need to backport to reef/quincy from our side

@soumyakoduri soumyakoduri mentioned this pull request Apr 26, 2024
Merged
14 tasks
@cbodley
Copy link
Contributor

cbodley commented Apr 26, 2024

No need to backport to reef/quincy from our side

up to you @soumyakoduri. feel free to close the quincy and reef trackers with Rejected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattbenjamin mattbenjamin mattbenjamin approved these changes

@dang dang Awaiting requested review from dang

Assignees
No one assigned
Labels
needs-review rgw
Projects
None yet
Milestone
No milestone
5 participants
@soumyakoduri @mattbenjamin @yuvalif @cbodley @jzhu116-bloomberg