Skip to content

test/libcephfs: update delegation test assertions#52569

Merged
vshankar merged 1 commit intoceph:mainfrom
petrutlucian94:deleg_tests
Aug 2, 2023
Merged

test/libcephfs: update delegation test assertions#52569
vshankar merged 1 commit intoceph:mainfrom
petrutlucian94:deleg_tests

Conversation

@petrutlucian94
Copy link
Contributor

@petrutlucian94 petrutlucian94 commented Jul 21, 2023 •
edited
Loading

Some of the libcephfs tests ensure that the delegations are recalled as a result of certain open requests.

The issue is that we're expecting the "opened" flag to be false after the delegation gets recalled, which is racy.

  /home/ubuntu/ceph/src/test/libcephfs/deleg.cc:392: Failure
  Expected equality of these values:
    opened.load()
      Which is: true
    false

We'll update those checks, asserting that "opened" is true after the delegation breaker thread returns.

Contribution Guidelines

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

@petrutlucian94
test/libcephfs: update delegation test assertions
7488dee 
Some of the libcephfs tests ensure that the delegations are recalled
as a result of certain open requests.

The issue is that we're expecting the "opened" flag to be false
after the delegation gets recalled, which is racy.

  /home/ubuntu/ceph/src/test/libcephfs/deleg.cc:392: Failure
  Expected equality of these values:
    opened.load()
      Which is: true
    false

We'll update those checks, asserting that "opened" is true after
the delegation breaker thread returns.

Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
@github-actions github-actions bot added cephfs Ceph File System tests labels Jul 21, 2023
@petrutlucian94 petrutlucian94 mentioned this pull request Jul 21, 2023
Merged
14 tasks
@petrutlucian94 petrutlucian94 requested a review from a team July 21, 2023 10:37
@petrutlucian94
Copy link
Contributor Author

petrutlucian94 commented Jul 21, 2023

jenkins test make check

@petrutlucian94
Copy link
Contributor Author

petrutlucian94 commented Jul 24, 2023 •
edited by vshankar
Loading

@vshankar Hi, I've submitted the changes that you were mentioning here >#52427 (comment). Please let me know what you think.

The Windows CI job is very unstable without this fix.

I think I accidentally deleted @petrutlucian94 comment. Will have a look today.

@vshankar vshankar requested a review from lxbsz July 25, 2023 04:11
@vshankar
Copy link
Contributor

vshankar commented Jul 25, 2023

LGTM. Requesting another eye on the change from @lxbsz

lxbsz
lxbsz approved these changes Jul 27, 2023
View reviewed changes
Copy link
Member

@lxbsz lxbsz left a comment

Choose a reason for hiding this comment

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

Makes sense.
LGTM.

@vshankar
Copy link
Contributor

vshankar commented Jul 31, 2023

@petrutlucian94 I nudged @mchangir to run this through fs suite over the weekend, but our shaman build have started acting otherwise, which means this change couldn't be tested. I suggest if this fixes the windows failures, then we merge it and @mchangir will run it through fs suite anyway and we can fix failures (if any). WDYT?

@petrutlucian94
Copy link
Contributor Author

petrutlucian94 commented Aug 1, 2023

@petrutlucian94 I nudged @mchangir to run this through fs suite over the weekend, but our shaman build have started acting otherwise, which means this change couldn't be tested. I suggest if this fixes the windows failures, then we merge it and @mchangir will run it through fs suite anyway and we can fix failures (if any). WDYT?

@vshankar Sounds good, thanks! It's a simple patch but please ping me if there are any issues.

Sorry for the late reply, I was OOF yesterday.

@vshankar
Copy link
Contributor

vshankar commented Aug 1, 2023

@petrutlucian94 I nudged @mchangir to run this through fs suite over the weekend, but our shaman build have started acting otherwise, which means this change couldn't be tested. I suggest if this fixes the windows failures, then we merge it and @mchangir will run it through fs suite anyway and we can fix failures (if any). WDYT?

@vshankar Sounds good, thanks! It's a simple patch but please ping me if there are any issues.

I'll merge this in a while.

@vshankar vshankar merged commit a920634 into ceph:main Aug 2, 2023
@vshankar
Copy link
Contributor

vshankar commented Aug 2, 2023

@mchangir I merged this, however, please run this through the integration tests.

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

Reviewers

@vshankar vshankar vshankar approved these changes

@lxbsz lxbsz lxbsz approved these changes

Assignees

No one assigned

Labels

cephfs Ceph File System tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@petrutlucian94 @vshankar @lxbsz @mchangir