Skip to content

Comments

rgw/cloudtier: Correct option ordering in RGWZoneGroupPlacementTier#61569

Merged
ivancich merged 1 commit intoceph:mainfrom
soumyakoduri:wip-skoduri-tier-config
Mar 6, 2025
Merged

rgw/cloudtier: Correct option ordering in RGWZoneGroupPlacementTier#61569
ivancich merged 1 commit intoceph:mainfrom
soumyakoduri:wip-skoduri-tier-config

Conversation

@soumyakoduri
Copy link
Contributor

@soumyakoduri soumyakoduri commented Jan 29, 2025 •
edited
Loading

Two tier-config options (related to cloud-restore) were incorrectly added in the middle of the encoding and decoding methods of RGWZoneGroupPlacementTier. This modification can cause compatibility issues with older decoders when attempting to read v2-encoded REST objects.

The fix is to correct the option order and update the decode() function to properly interpret the structure based on the encoded version.

Fixes: https://tracker.ceph.com/issues/69713
Signed-off-by: Soumya Koduri [email protected]

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 January 29, 2025 16:57
@github-actions github-actions bot added the rgw label Jan 29, 2025
cbodley
cbodley reviewed Jan 29, 2025
View reviewed changes
Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

looks great, just a suggestion to simplify future changes

ivancich
ivancich approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@ivancich ivancich left a comment

Choose a reason for hiding this comment

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

Thanks, Soumya, for quickly addressing this.

@soumyakoduri
Copy link
Contributor Author

soumyakoduri commented Jan 31, 2025

http://pulpito.front.sepia.ceph.com/soumyakoduri-2025-01-30_17:20:30-rgw-wip-skoduri-tier-config-distro-default-smithi/

verify and bucket_logging tests failed but seem unrelated to this PR. Below are the two s3 tests which are failing in those runs -
test_cross_account_topic
test_put_bucket_logging_errors

@ivancich are these known failures?

@ivancich
Copy link
Member

ivancich commented Feb 6, 2025

@soumyakoduri : Those errors are due to a regression that has its own PR to fix. The easiest way to get past them is to revert commit 0884e99 in your test branch.

@soumyakoduri soumyakoduri mentioned this pull request Feb 10, 2025
Merged
14 tasks
@soumyakoduri
Copy link
Contributor Author

soumyakoduri commented Feb 13, 2025

@soumyakoduri : Those errors are due to a regression that has its own PR to fix. The easiest way to get past them is to revert commit 0884e99 in your test branch.

Even after reverting that commit, test_cross_account_topic failed.

http://qa-proxy.ceph.com/teuthology/soumyakoduri-2025-02-13_06:21:51-rgw-wip-skoduri-testing-distro-default-smithi/8128476/teuthology.log

@ivancich
Copy link
Member

ivancich commented Feb 13, 2025

@soumyakoduri So the fixes are in main. So try rebasing to main. I think you'll get a clean run then.

@soumyakoduri
Copy link
Contributor Author

soumyakoduri commented Feb 19, 2025

verify - http://pulpito.front.sepia.ceph.com/soumyakoduri-2025-02-18_17:57:45-rgw:verify-wip-skoduri-testing-distro-default-smithi
bucket-logging - http://pulpito.front.sepia.ceph.com/soumyakoduri-2025-02-18_17:55:26-rgw:bucket-logging-wip-skoduri-testing-distro-default-smithi

@ivancich
Copy link
Member

ivancich commented Feb 24, 2025

verify - http://pulpito.front.sepia.ceph.com/soumyakoduri-2025-02-18_17:57:45-rgw:verify-wip-skoduri-testing-distro-default-smithi bucket-logging - http://pulpito.front.sepia.ceph.com/soumyakoduri-2025-02-18_17:55:26-rgw:bucket-logging-wip-skoduri-testing-distro-default-smithi

I think we need a full rgw suite to make sure there aren't unexpected effects. That was an issue recently where testing broke widely because someone merged after only narrow focused testing.

@soumyakoduri soumyakoduri force-pushed the wip-skoduri-tier-config branch from 8bb9192 to 5e6d375 Compare February 24, 2025 19:41
@soumyakoduri
rgw/cloudtier: Correct option ordering in RGWZoneGroupPlacementTier
c2d226a 
Two tier-config options (related to `cloud-restore`) were incorrectly added
in the middle of the encoding and decoding methods of RGWZoneGroupPlacementTier.
This modification can cause compatibility issues with older decoders when
attempting to read v2-encoded REST objects.

The fix is to correct the option order and update the decode() function to
properly interpret the structure based on the encoded version.

Signed-off-by: Soumya Koduri <[email protected]>
@soumyakoduri soumyakoduri force-pushed the wip-skoduri-tier-config branch from 5e6d375 to c2d226a Compare March 4, 2025 18:27
@soumyakoduri
Copy link
Contributor Author

soumyakoduri commented Mar 5, 2025

jenkins test api

1 similar comment
@soumyakoduri
Copy link
Contributor Author

soumyakoduri commented Mar 5, 2025

jenkins test api

@soumyakoduri
Copy link
Contributor Author

soumyakoduri commented Mar 5, 2025

@ivancich .. this is the latest run - https://pulpito.ceph.com/soumyakoduri-2025-03-05_08:23:24-rgw-wip-skoduri-tier-config-distro-default-smithi/

There is multisite test failure but doesn't seem related to this change.

@ivancich ivancich merged commit ba75391 into ceph:main Mar 6, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbodley cbodley cbodley left review comments

@ivancich ivancich ivancich approved these changes

@thotz thotz Awaiting requested review from thotz

@NitzanMordhai NitzanMordhai Awaiting requested review from NitzanMordhai

Assignees

No one assigned

Labels

bug-fix rgw

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@soumyakoduri @ivancich @cbodley