Skip to content

cls/rbd: update last_read in group::snap_list #48549

Merged
idryomov merged 3 commits intoceph:mainfrom
pkalever:snap-list
Dec 2, 2022
Merged

cls/rbd: update last_read in group::snap_list #48549
idryomov merged 3 commits intoceph:mainfrom
pkalever:snap-list

Conversation

@pkalever
Copy link

@pkalever pkalever commented Oct 19, 2022

cls/rbd: update last read in group::snap_list

Problem:
rbd group snap ls shows 1025 records after creating 65 snaps
with rbd group snap create

$ for i in {1..65}; do rbd group snap create test_group@group_snap$i; done
$ rbd group snap ls test_group | wc -l
1025

Solution:
update last_read after getting RBD_MAX_KEYS_READ with cls_cxx_map_get_vals.

Fixes: https://tracker.ceph.com/issues/57066
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@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

@pkalever pkalever requested a review from a team as a code owner October 19, 2022 10:00
@pkalever
Copy link
Author

@idryomov will it be possible for you to schedule the lab tests for this PR, please? Thanks!

Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

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

LGTM, some nits on the test case

@pkalever
Copy link
Author

@idryomov Thanks for the review Ilya, addressed your review comments now.

Copy link
Contributor

@trociny trociny left a comment

Choose a reason for hiding this comment

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

LGTM

You could add the "list overflow" test to src/test/cls_rbd/test_cls_rbd.cc. We already did it for mirror_image_status_list, when fixed a similar issue. Though I don't mind if you don't, because we do not have such tests for other list functions.

@idryomov
Copy link
Contributor

idryomov commented Oct 23, 2022

You could add the "list overflow" test to src/test/cls_rbd/test_cls_rbd.cc. We already did it for mirror_image_status_list, when fixed a similar issue. Though I don't mind if you don't, because we do not have such tests for other list functions.

Let's add it for this one since it has lingered for so long it took a user to stumble upon this...

@idryomov
Copy link
Contributor

idryomov commented Nov 8, 2022

Please fold the fourth commit into the first one (so that the test is added together with the code change) and rename the third commit to "test/cls_rbd: delete unused image_id variable".

Prasanna Kumar Kalever added 3 commits November 9, 2022 11:19
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Problem:
`rbd group snap ls` shows 1025 records after creating 65 snaps
with `rbd group snap create`

$ for i in {1..65}; do rbd group snap create test_group@group_snap$i; done
$ rbd group snap ls test_group | wc -l
1025

Solution:
update last_read after getting RBD_MAX_KEYS_READ with cls_cxx_map_get_vals.

Fixes: https://tracker.ceph.com/issues/57066
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
@idryomov
Copy link
Contributor

idryomov commented Nov 9, 2022

One process-related nit: don't rebase when force-pushing a new version of the PR unless the rebase is necessary to pick up a relevant change or dependency that just showed up main. Keep the base commit the same -- this makes diffing much easier.

Otherwise LGTM!

@idryomov idryomov changed the title cls/rbd: update last read in group::snap_list cls/rbd: update last_read in group::snap_list Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants