Skip to content

pacific: revert backport of #45529#46610

Closed
rzarzynski wants to merge 2 commits intoceph:pacificfrom
rzarzynski:wip-55989-pacific
Closed

pacific: revert backport of #45529#46610
rzarzynski wants to merge 2 commits intoceph:pacificfrom
rzarzynski:wip-55989-pacific

Conversation

@rzarzynski
Copy link
Contributor

@rzarzynski rzarzynski commented Jun 9, 2022

Technically this isn't a backport but a full revert of an already merged backport.

"Backport" ticket for the sake of tracking: https://tracker.ceph.com/issues/55989

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

rzarzynski added 2 commits June 9, 2022 21:22
@rzarzynski
Revert "osd/PGLog.cc: Trim duplicates by number of entries"
ea7429d 
This reverts commit d49ff13.
which is the in-OSD part of the fix for accumulation of `dup`
entries in a PG Log. Brainstorming it has brought questions
on the OSD's behaviour during an upgrade if there are tons of
dups in the log. What must be double-checked before bringing
it back is ensuring we chunk the deletions properly to not
impose OOMs / stalls in, to exemplify, RocksDB.

The backport ticket is: https://tracker.ceph.com/issues/55989

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
@rzarzynski
Revert "tools/ceph_objectstore_took: Add duplicate entry trimming"
8842de4 
This reverts commit 1f3fede.

Although the chunking in off-line `dups` trimming (via COT) seems
fine, the `ceph-objectstore-tool` is a client of `trim()` of
`PGLog::IndexedLog` which means than a partial revert is not
possible without extensive changes.

The backport ticket is: https://tracker.ceph.com/issues/55989

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
@rzarzynski rzarzynski added this to the pacific milestone Jun 9, 2022
@rzarzynski rzarzynski requested a review from neha-ojha June 9, 2022 21:33
@rzarzynski rzarzynski requested a review from a team as a code owner June 9, 2022 21:33
@xiexingguo
Copy link
Member

xiexingguo commented Jun 13, 2022

@rzarzynski I don't quite understand. Does the original fix do anything wrong?

@rzarzynski
Copy link
Contributor Author

rzarzynski commented Jun 13, 2022

@xiexingguo: yes, there are two problems:

  1. the on-online trimming doesn't chunk dups removal in some cases,
  2. the off-line trimming touches regular entries of of pg log but doesn't modify pg_info_t accordingly (this came over the weekend).

A new incarnation of the off-line trimming is #46631.

@xiexingguo
Copy link
Member

xiexingguo commented Jun 13, 2022

@rzarzynski Thanks!

  1. the on-online trimming doesn't chunk dups removal in some cases,

So https://tracker.ceph.com/issues/53729 remains. What's our new plan for it?

@rzarzynski
Copy link
Contributor Author

rzarzynski commented Jun 13, 2022 •
edited
Loading

@xiexingguo: implement a proper chunking for dups in PGLog::IndexedLog::trim(). WIP.

@wvh-github wvh-github mentioned this pull request Jun 14, 2022
Merged
14 tasks
@wvh-github
Copy link

wvh-github commented Jun 14, 2022 •
edited
Loading

Is it possible to keep the code in the ceph-objectstore-tool. Part of the problem here is that fact that OSD are using too much memory when the dups aren't being trimmed. We are reverting this to make sure the OSD is not going to use too much memory during the upgrade.

I think there should at least b an offline way of trimming the dups. Then more time can be spent on making the trimming inside the OSD work well.

Also if the revert moves forward we'd have to reopen https://tracker.ceph.com/issues/55631

@wvh-github
Copy link

wvh-github commented Jun 15, 2022

@xiexingguo: yes, there are two problems:

1. the on-online trimming doesn't chunk dups removal in some cases,

2. the off-line trimming touches regular entries of of pg log but doesn't modify `pg_info_t` accordingly (this came over the weekend).

A new incarnation of the off-line trimming is #46631.

Sorry, I didn't read this.

@pponnuvel
Copy link
Contributor

pponnuvel commented Jun 16, 2022

@rzarzynski Are the Octopus commits (PR #46253 - already merged & likely to make it to 15.2.17) going to be reverted as well?

@wvh-github
Copy link

wvh-github commented Jun 16, 2022

Hi @pponnuvel Does this PR answer you question? #46610
I think the intention is to revert it, I just don't know if there is going to be a release of Octopus first

@pponnuvel
Copy link
Contributor

pponnuvel commented Jun 16, 2022

Hi @pponnuvel Does this PR answer you question? #46610 I think the intention is to revert it, I just don't know if there is going to be a release of Octopus first

This PR (#46610) is for Pacific, right? I was checking if this will be backported to Octopus as well.

I believe, there's one more point release for Octopus planned/in progress: https://www.spinics.net/lists/dev-ceph/msg04468.html

@wvh-github
Copy link

wvh-github commented Jun 16, 2022

Sorry, i meant this one 46611

@pponnuvel
Copy link
Contributor

pponnuvel commented Jun 16, 2022

@wvh-github Ah, right. Thanks - that does answer.

Hopefully it makes to 15.2.17 too!

@github-actions
Copy link

github-actions bot commented Jun 19, 2022

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@k0ste
Copy link
Contributor

k0ste commented Jul 5, 2022

@rzarzynski, still this should be reverted or not after #46631 merge?

@rzarzynski
Copy link
Contributor Author

rzarzynski commented Jul 5, 2022

@k0ste: we don't need this PR as #46631 already merged these two revert commits (and added the new op to COT). Closing.

@rzarzynski rzarzynski closed this Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@neha-ojha neha-ojha Awaiting requested review from neha-ojha

Assignees

No one assigned

Labels

core needs-rebase tests

Projects

None yet

Milestone

pacific

Development

Successfully merging this pull request may close these issues.

5 participants

@rzarzynski @xiexingguo @wvh-github @pponnuvel @k0ste