Skip to content

mon: Remove any pg_upmap_primary mapping during remove a pool#58470

Merged
yuriw merged 1 commit intoceph:mainfrom
mohit84:issue_66867
Aug 4, 2024
Merged

mon: Remove any pg_upmap_primary mapping during remove a pool#58470
yuriw merged 1 commit intoceph:mainfrom
mohit84:issue_66867

Conversation

@mohit84
Copy link
Contributor

@mohit84 mohit84 commented Jul 9, 2024

The pg_upmap_primary items are not deleted after the pool is deleted from the cluster.

Fixes: https://tracker.ceph.com/issues/66867

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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

@mohit84 mohit84 requested a review from a team as a code owner July 9, 2024 04:39
@mohit84
Copy link
Contributor Author

mohit84 commented Jul 9, 2024

jenkins test make check

@ljflores ljflores requested review from JoshSalomon and ljflores July 9, 2024 16:31
<< dendl;
pending_inc.old_pg_upmap_primary.insert(p.first);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this @mohit84! Can you also include removal of pending_inc.new_pg_upmap_primary? Similar to the above section which removes new_pg_upmap_items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@mohit84
Copy link
Contributor Author

mohit84 commented Jul 12, 2024

jenkins test make check

@ljflores
Copy link
Member

Will review this soon..

<< " removing obsolete and pending pg_upmap_primaries " << p.first
<< dendl;
pending_inc.old_pg_upmap_primary.insert(p.first);
pending_inc.new_pg_upmap_primary.erase(p.first);
Copy link
Contributor

Choose a reason for hiding this comment

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

No - this is incorrect - look above. You should have one loop over pg_upmap_primaries with the code old_pg_upmap_primary.insert() (this code exists) - but you should remove line 14369 and replace it with a loop over new_pg_upmap_primary (there will be different entries there. Basically, you should duplicate the loop between lines 14619 and 14631 using the newer iteration syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing the patch, done !!

Copy link
Contributor

@JoshSalomon JoshSalomon left a comment

Choose a reason for hiding this comment

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

Line 14639 should be replaced with a loop similar to the loop between lines 14619 and 14631

@mohit84
Copy link
Contributor Author

mohit84 commented Jul 16, 2024

jenkins test make check

}
}

// remove any new pg_upmap_primary mapping for this pool
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the comment to "remove any pending pg_upmap_primary ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@JoshSalomon JoshSalomon left a comment

Choose a reason for hiding this comment

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

One small comment issue

@mohit84
Copy link
Contributor Author

mohit84 commented Jul 16, 2024

jenkins test make check

Copy link
Member

@ljflores ljflores left a comment

Choose a reason for hiding this comment

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

Nit: Can you change the commit title to mon instead of osdmon?

@mohit84 mohit84 changed the title osdmon: Remove any pg_upmap_primary mapping during remove a pool mon: Remove any pg_upmap_primary mapping during remove a pool Jul 23, 2024
@mohit84
Copy link
Contributor Author

mohit84 commented Jul 23, 2024

Nit: Can you change the commit title to mon instead of osdmon?

done

@ljflores
Copy link
Member

jenkins test api

@ljflores
Copy link
Member

jenkins test windows

The pg_upmap_primary items are not deleted after the pool is deleted
from the cluster.

Fixes: https://tracker.ceph.com/issues/66867
Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>
@ljflores
Copy link
Member

ljflores commented Jul 30, 2024

I noticed a compilation error, so I fixed it. @mohit84 Hope you don't mind that I pushed to your branch- I would like to include this in the next squid RC, so it had to be fixed quickly.

I verified that the fix works as expected.

@ljflores
Copy link
Member

Created the squid backport to test in parallel.

@NitzanMordhai
Copy link
Contributor

@mohit84
Copy link
Contributor Author

mohit84 commented Aug 4, 2024

I noticed a compilation error, so I fixed it. @mohit84 Hope you don't mind that I pushed to your branch- I would like to include this in the next squid RC, so it had to be fixed quickly.

I verified that the fix works as expected.

Of course i don't mind it.

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.

6 participants