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

remove pubsub from the RGW #48996

Merged
merged 4 commits into from Dec 1, 2022
Merged

remove pubsub from the RGW #48996

merged 4 commits into from Dec 1, 2022

Conversation

yuvalif
Copy link
Contributor

@yuvalif yuvalif commented Nov 21, 2022

pubsub is deprecated in favor of persistent notifications

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

@yuvalif yuvalif added the DNM label Nov 21, 2022
@yuvalif yuvalif requested a review from a team as a code owner November 21, 2022 18:40
@yuvalif yuvalif mentioned this pull request Nov 21, 2022
Closed
14 tasks
@yuvalif yuvalif requested a review from a team as a code owner November 22, 2022 09:26
@github-actions github-actions bot added the core label Nov 22, 2022
@yuvalif yuvalif removed the core label Nov 22, 2022
@yuvalif yuvalif requested review from mattbenjamin, cbodley and yehudasa and removed request for a team November 22, 2022 09:32
ronen-fr
ronen-fr reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@ronen-fr ronen-fr left a comment

Choose a reason for hiding this comment

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

LGTM. Added some minor notes.

doc/radosgw/s3-notification-compatibility.rst Outdated Show resolved Hide resolved
src/rgw/rgw_notify_event_type.cc Show resolved Hide resolved
src/rgw/rgw_pubsub.h Show resolved Hide resolved
src/rgw/rgw_rest_pubsub.cc Outdated
bool verify_transport_security(CephContext *cct, const RGWEnv& env) {
const auto is_secure = rgw_transport_is_secure(cct, env);
if (!is_secure && g_conf().get_val<bool>("rgw_allow_notification_secrets_in_cleartext")) {
ldout(cct, 0) << "WARNING: bypassing endpoint validation, allow sending password over insecure transport" << dendl;
Copy link
Contributor

Choose a reason for hiding this comment

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

allows?

src/rgw/rgw_rest_pubsub.cc Outdated Show resolved Hide resolved
src/rgw/rgw_rest_pubsub.cc Outdated Show resolved Hide resolved
src/rgw/rgw_rest_pubsub.cc Outdated Show resolved Hide resolved
src/rgw/rgw_rest_pubsub.cc Outdated Show resolved Hide resolved
src/rgw/rgw_rest_pubsub.cc Outdated Show resolved Hide resolved
@yuvalif
Copy link
Contributor Author

yuvalif commented Nov 23, 2022

jenkins test make check

@yuvalif yuvalif removed the DNM label Nov 23, 2022
@yuvalif yuvalif changed the title [WIP] remove pubsub from the RGW remove pubsub from the RGW Nov 23, 2022
@yuvalif
Copy link
Contributor Author

yuvalif commented Nov 28, 2022

teuthology: http://pulpito.front.sepia.ceph.com/yuvalif-2022-11-23_13:29:52-rgw-wip-yuval-remove-pubsub-distro-default-smithi/
failures are due to:

@cbodley
Copy link
Contributor

cbodley commented Nov 28, 2022

teuthology: http://pulpito.front.sepia.ceph.com/yuvalif-2022-11-23_13:29:52-rgw-wip-yuval-remove-pubsub-distro-default-smithi/ failures are due to:

* multisite crash: https://tracker.ceph.com/issues/57905

* boto/ubuntu issue: https://tracker.ceph.com/issues/58059

please --rerun to verify that the ubuntu jobs succeed. there were also 15 dead jobs that need to run

@yuvalif
Copy link
Contributor Author

yuvalif commented Nov 28, 2022

teuthology: http://pulpito.front.sepia.ceph.com/yuvalif-2022-11-23_13:29:52-rgw-wip-yuval-remove-pubsub-distro-default-smithi/ failures are due to:

* multisite crash: https://tracker.ceph.com/issues/57905

* boto/ubuntu issue: https://tracker.ceph.com/issues/58059

please --rerun to verify that the ubuntu jobs succeed. there were also 15 dead jobs that need to run

I tried: http://pulpito.front.sepia.ceph.com/yuvalif-2022-11-27_20:40:16-rgw-wip-yuval-remove-pubsub-distro-default-smithi/
but sepia does not seem to be able to run tests

@github-actions
Copy link

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

@yuvalif
rgw/pubsub: remove pusub sync module functionality
0989166 
also update the documentation

Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
@yuvalif
rgw/pubsub: remove pusub subscription functionality
a7963d7 
Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
@yuvalif
rgw/pubsub: remove deprecated pubsub event types
ea7089c 
Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
@yuvalif
rgw/pubsub: remove unnecessary REST abstraction layer
0fecbe2 
this was used for an already deprecated non s3 API for pubsub
also, some texts changes in docs and error messages

Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
@yuvalif
Copy link
Contributor Author

yuvalif commented Dec 1, 2022

jenkins test make check

@yuvalif
Copy link
Contributor Author

yuvalif commented Dec 1, 2022

latest teithology passing: http://pulpito.front.sepia.ceph.com/yuvalif-2022-11-28_15:13:00-rgw-wip-yuval-remove-pubsub-distro-default-smithi/

failures are in the multisite tests:

@cbodley cbodley merged commit ce0bec6 into ceph:main Dec 1, 2022
@cbodley
Copy link
Contributor

cbodley commented Dec 1, 2022

thanks @yuvalif! i think this deserves a release note, but didn't want to delay the merge - could you please raise a separate PR for that?

@yuvalif
Copy link
Contributor Author

yuvalif commented Dec 1, 2022

thanks @yuvalif! i think this deserves a release note, but didn't want to delay the merge - could you please raise a separate PR for that?

#49177

@cbodley cbodley mentioned this pull request Mar 2, 2023
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dang dang dang approved these changes

@cbodley cbodley cbodley approved these changes

@ronen-fr ronen-fr ronen-fr approved these changes

@mattbenjamin mattbenjamin Awaiting requested review from mattbenjamin

@yehudasa yehudasa Awaiting requested review from yehudasa

Assignees
No one assigned
Labels
build/ops documentation needs-qa rgw tests
Projects
None yet
Milestone
No milestone
4 participants
@yuvalif @cbodley @dang @ronen-fr