RGW - Zipper - remove RGWObjectCtx from SAL API by dang · Pull Request #44616 · ceph/ceph

dang · 2022-01-17T19:50:28Z

RGWObjectCtx is a RADOS-specific concept, but it's threaded through the
whole of RGW. Remove it from the SAL API, and from everything above the
line, leaving it only in RadosStore. As part of this, move RGWObjState
to SAL and use it as the key for an Object.

Signed-off-by: Daniel Gryniewicz dang@fprintf.net

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)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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

soumyakoduri · 2022-01-18T12:05:29Z

src/rgw/rgw_putobj_processor.cc

+		  obj_ctx, head_obj.get());
  RGWRados::Object::Write obj_op(&op_target);

+  ldpp_dout(dpp, 0) << "Luke 2 if_match=" << if_match << dendl;


nit: cleanup?

Thanks, missed these

soumyakoduri · 2022-01-18T13:04:28Z

src/rgw/rgw_op.cc

    emplace_attr(RGW_ATTR_OBJECT_RETENTION, std::move(obj_retention_bl));
  }

+  ldpp_dout(this, 0) << "Luke 1 if_match=" << if_match << dendl;


nit: cleanup

soumyakoduri

Overall changes look good to me.

soumyakoduri · 2022-01-18T13:31:56Z

src/rgw/store/dbstore/common/dbstore.cc

-  return 0;
-}
-
+//int DB::Object::get_manifest(const DoutPrefixProvider *dpp, RGWObjManifest **pmanifest)


This function may be removed completely from DBStore.

dang · 2022-01-21T14:41:35Z

Teuthology passes:
https://pulpito.ceph.com/dang-2022-01-20_12:00:43-rgw-wip-dang-zipper-objctx-distro-basic-smithi/

soumyakoduri · 2022-01-25T15:14:16Z

src/rgw/rgw_sal.h

    /** Mark further operations on this object as being atomic */
-    virtual void set_atomic(RGWObjectCtx* rctx) const = 0;
+    virtual void set_atomic() {
+      state.is_atomic = true;


what does "is_atomic" signify? what semantics should the backend store ensure if it is set to true?

It seems to cause atomic RADOS ops to be used instead of non-atomic ops.

okay..thanks! I am unsure if its needed for other stores. But I guess that can be confirmed and cleaned up once the other stores are fully ready.

soumyakoduri · 2022-01-25T15:14:58Z

src/rgw/store/dbstore/common/dbstore.h


      int get_state(const DoutPrefixProvider *dpp, RGWObjState **pstate, bool follow_olh);
-      int get_manifest(const DoutPrefixProvider *dpp, RGWObjManifest **pmanifest);
+      /*int get_manifest(const DoutPrefixProvider *dpp, RGWObjManifest **pmanifest);*/


nit: cleanup

soumyakoduri

sal* changes look good to me.

github-actions · 2022-02-08T17:32:06Z

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

dang · 2022-02-15T13:33:40Z

jenkins test make check

dang · 2022-02-15T13:33:53Z

jenkins test make check arm64

dang · 2022-02-15T13:34:03Z

jenkins test api

dang · 2022-02-15T18:45:35Z

@andriytk can you review the MOTR bits?

andriytk

@dang all looks fine, thanks!

github-actions · 2022-03-07T14:38:25Z

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

RGWObjectCtx is a RADOS-specific concept, but it's threaded through the whole of RGW. Remove it from the SAL API, and from everything above the line, leaving it only in RadosStore. As part of this, move RGWObjState to SAL and use it as the key for an Object. Signed-off-by: Daniel Gryniewicz <dang@fprintf.net>

dang · 2022-03-09T13:21:07Z

Rebase still passes:
https://pulpito.ceph.com/dang-2022-03-08_08:10:53-rgw-wip-dang-zipper-objctx-distro-default-smithi/

dang · 2022-03-09T13:22:22Z

jenkins test api

dang · 2022-03-22T12:54:43Z

jenkins test api

dang · 2022-03-22T12:55:19Z

jenkins test windows

andriytk · 2022-09-08T11:39:54Z

Hello @dang, I have a question: in the latest motr sal code we are using RGWObjectCtx to set the error msg from MotrObject::get_obj_state(), in case the object does not exist, see Seagate#368. Could you advise, please, how/where this error can be set after this change?

BTW, it seems the problem with setting the correct error msg also exists with RADOS backend, but I'm not 100% sure. Here is what I found in the latest code in the main branch:

ceph/src/rgw/rgw_rest_s3.cc

Lines 5255 to 5259 in 72f8cfa

    
              * FIXME Missing headers: 
        
              * With a working errordoc, the s3 error fields are rendered as HTTP headers, 
        
              *   x-amz-error-code: NoSuchKey 
        
              *   x-amz-error-message: The specified key does not exist. 
        
              *   x-amz-error-detail-Key: foo

Thank you in advance!

cdeshmukh · 2022-09-14T05:49:35Z

Hi @dang, can you please suggest some way if you know, i believe you must of thought of same when doing these changes. We(cortx-rgw) are based out of quincy release right now and we will face issues when we rebase with main in future.

cbodley · 2022-09-14T13:12:33Z

in the latest motr sal code we are using RGWObjectCtx to set the error msg from MotrObject::get_obj_state()

@andriytk @cdeshmukh this feels like a layering violation to me. MotrObject::get_obj_state() should't assume that a req_state even exists - these sal apis can be called through background threads like GC/LC, radosgw-admin commands, or test cases where there is no request context

if there's a need to pass more-specific errors back up to the calling RGWOp, we might look at adding new ERR_ codes to rgw_common.h to represent those. but in this case, it looks like some RGWOps would just need to map ENOENT-> "The specified key does not exist." and ERR_PRECONDITION_FAILED -> "At least one of the pre-conditions you specified did not hold."

if i understand correctly, all radosgw backends would lack these error messages, so this shouldn't be solved in the motr layer

that linked PR mentions "copy object operation" specifically, so i'd start with RGWCopyObj and RGWPutObj. if we only want these messages in S3 and not Swift responses, they would go in RGWCopyObj_ObjStore_S3::send_partial_response() and RGWPutObj_ObjStore_S3::send_response()

get_obj_head_ioctx returns an int where 0 indicates success. When called in RGWRados::check_disk_state the result was treated as a booleann with inverted logic. This fixes that error. This was already fixed in PR ceph#44616 as part of a larger commit, but this PR unifies the codebase, so various backports are consistent. Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>

get_obj_head_ioctx returns an int where 0 indicates success. When called in RGWRados::check_disk_state the result was treated as a booleann with inverted logic. This fixes that error. This was already fixed in PR ceph#44616 as part of a larger commit, but this PR unifies the codebase, so various backports are consistent. This also adds additional logging. Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>

cdeshmukh · 2022-09-16T05:01:27Z

in the latest motr sal code we are using RGWObjectCtx to set the error msg from MotrObject::get_obj_state()

@andriytk @cdeshmukh this feels like a layering violation to me. MotrObject::get_obj_state() should't assume that a req_state even exists - these sal apis can be called through background threads like GC/LC, radosgw-admin commands, or test cases where there is no request context

thanks @cbodley for the details, we raised this to understand proper ways of handling it.
And probably we can add additional check that req_state exists and then only proceed to use it, until we correct this approach & finish testing thoroughly.

if there's a need to pass more-specific errors back up to the calling RGWOp, we might look at adding new ERR_ codes to rgw_common.h to represent those. but in this case, it looks like some RGWOps would just need to map ENOENT-> "The specified key does not exist." and ERR_PRECONDITION_FAILED -> "At least one of the pre-conditions you specified did not hold."

Yes, its needed for AWS S3 compliance

if i understand correctly, all radosgw backends would lack these error messages, so this shouldn't be solved in the motr layer

Agree, our approach was for time being until we understand proper ways, for which this query was raised.

that linked PR mentions "copy object operation" specifically, so i'd start with RGWCopyObj and RGWPutObj. if we only want these messages in S3 and not Swift responses, they would go in RGWCopyObj_ObjStore_S3::send_partial_response() and RGWPutObj_ObjStore_S3::send_response()

Yes, targeting in S3 alone for AWS compliance.

cbodley · 2022-09-16T15:43:05Z

Yes, its needed for AWS S3 compliance

we've never really considered these error messages to be part of the s3 api for compliance purposes - only the error codes like NoSuchKey/PreconditionFailed along with the http status

these error messages are helpful for clients trying to debug issues so i definitely support adding more, but we shouldn't require them to match AWS' error messages verbatim

get_obj_head_ioctx returns an int where 0 indicates success. When called in RGWRados::check_disk_state the result was treated as a booleann with inverted logic. This fixes that error. This was already fixed in PR ceph#44616 as part of a larger commit, but this PR unifies the codebase, so various backports are consistent. This also adds additional logging. Signed-off-by: J. Eric Ivancich <ivancich@redhat.com> (cherry picked from commit 28d04e8)

dang added the rgw label Jan 17, 2022

dang requested review from cbodley and soumyakoduri January 17, 2022 19:50

soumyakoduri reviewed Jan 18, 2022

View reviewed changes

dang force-pushed the wip-dang-zipper-objctx branch 3 times, most recently from e191996 to e25f775 Compare January 19, 2022 19:28

soumyakoduri reviewed Jan 25, 2022

View reviewed changes

soumyakoduri approved these changes Jan 27, 2022

View reviewed changes

github-actions bot added the needs-rebase label Feb 8, 2022

dang force-pushed the wip-dang-zipper-objctx branch from e25f775 to 1bdd4fb Compare February 8, 2022 19:39

github-actions bot removed the needs-rebase label Feb 8, 2022

dang force-pushed the wip-dang-zipper-objctx branch from 1bdd4fb to 94b30f7 Compare February 15, 2022 18:41

andriytk approved these changes Feb 15, 2022

View reviewed changes

github-actions bot added the needs-rebase label Mar 7, 2022

dang force-pushed the wip-dang-zipper-objctx branch from 94b30f7 to 88bcacc Compare March 7, 2022 15:53

github-actions bot removed the needs-rebase label Mar 7, 2022

dang merged commit 115c8b2 into ceph:master Mar 22, 2022

soumyakoduri mentioned this pull request Mar 29, 2022

rpm: use system libpmem on Centos 9 Stream #45341

Merged

14 tasks

andriytk mentioned this pull request Sep 8, 2022

rgw_sal_motr: [CORTX-33105] - Handle errors for copy object operation Seagate/cortx-rgw#368

Merged

14 tasks

ivancich mentioned this pull request Sep 14, 2022

rgw: fix bool/int logic error when calling get_obj_head_ioctx #48095

Merged

14 tasks

cdeshmukh mentioned this pull request Sep 16, 2022

rgw_sal_motr:[CORTX-34158] Fix PutObject tagging, GetObject tagging and DeleteObject tagging versioning discrepancies. Seagate/cortx-rgw#433

Closed

14 tasks

cbodley mentioned this pull request Jun 27, 2024

rgw/rgw_rados: fix server side-copy orphans tail-objects #58213

Merged

14 tasks

cbodley mentioned this pull request Apr 1, 2025

rgw: keep the tails when copying object to itself #62604

Merged

14 tasks

Conversation

dang commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soumyakoduri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dang commented Jan 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soumyakoduri left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 8, 2022

Uh oh!

dang commented Feb 15, 2022

Uh oh!

dang commented Feb 15, 2022

Uh oh!

dang commented Feb 15, 2022

Uh oh!

dang commented Feb 15, 2022

Uh oh!

andriytk left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 7, 2022

Uh oh!

dang commented Mar 9, 2022

Uh oh!

dang commented Mar 9, 2022

Uh oh!

dang commented Mar 22, 2022

Uh oh!

dang commented Mar 22, 2022

Uh oh!

andriytk commented Sep 8, 2022

Uh oh!

cdeshmukh commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Sep 14, 2022

Uh oh!

cdeshmukh commented Sep 16, 2022

Uh oh!

cbodley commented Sep 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dang commented Jan 17, 2022 •

edited

Loading

cdeshmukh commented Sep 14, 2022 •

edited

Loading