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

CORE-2459 Fix UBSAN diagnostics #17026

Merged
merged 7 commits into from
Mar 20, 2024
Merged

CORE-2459 Fix UBSAN diagnostics #17026

merged 7 commits into from
Mar 20, 2024

Conversation

nvartolomei
Copy link
Contributor

@nvartolomei nvartolomei commented Mar 13, 2024 •
edited

A couple creeped in because we weren't crashing on UBSAN diagnostics for a while. That is being fixed too in #17027

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Mar 13, 2024 •
edited

new failures in https://buildkite.com/redpanda/redpanda/builds/46094#018e3736-e47b-4595-90ed-3bb04cd35d39:

"rptest.tests.consumer_group_balancing_test.ConsumerGroupBalancingTest.test_coordinator_nodes_balance"

new failures in https://buildkite.com/redpanda/redpanda/builds/46148#018e3a41-27c6-4a5e-9d91-6f481889ee76:

"rptest.tests.controller_log_limiting_test.ControllerLogLimitPartitionBalancerTests.test_partition_balancer_with_limits"

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Mar 13, 2024 •
edited

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46094#018e3733-3309-45ac-b3f4-4546c2368cfb

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46458#018e5866-eb81-4a4a-ad0f-bf69fa07bea4

@nvartolomei
Copy link
Contributor Author

/dt
tests/rptest/tests/consumer_group_balancing_test.py

@nvartolomei nvartolomei changed the title storage/tests: shortcircuit reading empty log Fix UBSAN diagnostics Mar 13, 2024
@nvartolomei nvartolomei mentioned this pull request Mar 13, 2024
Merged
6 tasks
@nvartolomei
Copy link
Contributor Author

DT failure seems unrelated

Module: rptest.tests.controller_log_limiting_test
Class:  ControllerLogLimitPartitionBalancerTests
Method: test_partition_balancer_with_limits

raise BadLogLines(bad_lines)
rptest.services.utils.BadLogLines: <BadLogLines nodes=docker-rp-12(4) example="ERROR 2024-03-14 00:16:21,198 [shard 0:main] cluster - controller_backend.cc:932 - [{kafka/topic-ayxklmwtgn/84}] reconciliation seems stuck (last retried 67s. ago), state: {changed_at: {95}, properties_changed_at: {nullopt}, shard_changed_at: {nullopt}, removed: false, cur_operation: {{revision: 95, type: update, assignment: { id: 84, group_id: 85, replicas: {{node_id: 1, shard: 0}, {node_id: 2, shard: 0}, {node_id: 3, shard: 1}} }, retries: 7, last_error: cluster::errc:20 (Waiting for partition reconfiguration to be finished)}}}">

dotnwat
dotnwat reviewed Mar 18, 2024
View reviewed changes
src/v/storage/tests/storage_e2e_test.cc Outdated Show resolved Hide resolved
src/v/cloud_roles/refresh_credentials.cc Show resolved Hide resolved
src/v/compat/tests/compat_test.cc Show resolved Hide resolved
@nvartolomei
storage/tests: shortcircuit reading empty log
2490598 
Prevents signed integer overflow in the following expression:
  `std::max(model::offset(0), offset.dirty_offset - model::offset(10))`
@nvartolomei
cloud_roles: remove redundant trace log
cb64d6a 
We log the same data in the same function few lines later.
@nvartolomei
cloud_roles: handle negative sleep duration in the caller
59feb02 
Relevant for the next change which addresses UBSAN diagnostic. The input
to the function is an unsigned value.

See
redpanda-data#14205 (comment)
for discussion.
@nvartolomei
cloud_roles: fix ubsan diagnostic
8d8c2bb 
Triggered by `test_cloud_roles_rpunit`:

```
runtime error: 3.67167e+09 is outside the range of representable values of type 'int'

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/nv/redpanda/src/v/cloud_roles/refresh_credentials.cc:226:17 in
```
@nvartolomei
compat/tests: prevent integer underflow
1c25174
@nvartolomei
storage/tests: fix integer underflow
a219984
@nvartolomei
storage/tests: test with default model::offset value
0cbdb4e 
This comparison has richer semantic meaning in this context.
rockwotj
rockwotj approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

LGTM

@nvartolomei nvartolomei merged commit 70a781d into redpanda-data:dev Mar 20, 2024
16 checks passed
@redpanda-data redpanda-data deleted a comment from vbotbuildovich Mar 20, 2024
@nvartolomei
Copy link
Contributor Author

/backport 23.3

@vbotbuildovich
Copy link
Collaborator

Oops! Something went wrong.

Workflow run logs.

@mmaslankaprv
Copy link
Member

/backport 23.2

@vbotbuildovich
Copy link
Collaborator

Oops! Something went wrong.

Workflow run logs.

@mmaslankaprv mmaslankaprv mentioned this pull request Apr 16, 2024
Merged
@dotnwat dotnwat changed the title Fix UBSAN diagnostics CORE-2459 Fix UBSAN diagnostics Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotnwat dotnwat dotnwat left review comments

@rockwotj rockwotj rockwotj approved these changes

@andijcr andijcr Awaiting requested review from andijcr

@Lazin Lazin Awaiting requested review from Lazin

@BenPope BenPope Awaiting requested review from BenPope

@ballard26 ballard26 Awaiting requested review from ballard26

@graphcareful graphcareful Awaiting requested review from graphcareful

@abhijat abhijat Awaiting requested review from abhijat

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nvartolomei @vbotbuildovich @mmaslankaprv @dotnwat @rockwotj