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

nautilus: rgw: GET/HEAD and PUT operations on buckets w/lifecycle expiration configured do not return x-amz-expiration header #30020

Closed
wants to merge 5 commits into from
Closed

nautilus: rgw: GET/HEAD and PUT operations on buckets w/lifecycle expiration configured do not return x-amz-expiration header #30020

wants to merge 5 commits into from
+210 −8

Conversation

pdvian
Copy link

@pdvian pdvian commented Aug 30, 2019

@tchaikov tchaikov added this to the nautilus milestone Aug 30, 2019
@tchaikov
Copy link
Contributor

retest this please.

@smithfarm
Copy link
Contributor

smithfarm commented Aug 30, 2019 •
edited

Build failure:

-- Could not find fmt, will build it
CMake Error at src/CMakeLists.txt:287 (add_subdirectory):
  add_subdirectory given source "fmt" which is not an existing directory.
  • looks like it's caused by the changes in this PR.

@smithfarm smithfarm added the DNM label Aug 30, 2019
@tchaikov
Copy link
Contributor

retest this please.

@smithfarm
Copy link
Contributor

Well, to start with, there's no src/fmt directory in nautilus...

tchaikov
tchaikov reviewed Aug 30, 2019
View reviewed changes
src/CMakeLists.txt Outdated
find_package(fmt 5.2.1 QUIET)
if(NOT fmt_FOUND)
message(STATUS "Could not find fmt, will build it")
add_subdirectory(fmt)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to continue using seastar/fmt.

Copy link
Author

Choose a reason for hiding this comment

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

@tchaikov Shall I modify this PR to reflect "seastar/fmt" dir ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pdvian yeah, to be specific. we just need to move the

  find_package(fmt 5.2.1 QUIET)		
   if(NOT fmt_FOUND)		
     message(STATUS "Could not find fmt, will build it")		
     add_subdirectory(seastar/fmt)		
   endif()

block out of if(WITH_SEASTAR).

@tchaikov
Copy link
Contributor

@smithfarm thanks for reminder! i just recalled that we lifted fmt from seastar/fmt after nautilus was released..

@smithfarm
Copy link
Contributor

@pdvian Ping, could you please make the change suggested by Kefu in his comment #30020 (comment) ?

@mattbenjamin @smithfarm
rgw: project and return lc expiration from GET/HEAD and PUT ops
f21f3df 
https://tracker.ceph.com/issues/38055

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
(cherry picked from commit c89a889)

Conflicts:
	src/CMakeLists.txt
- moved "find_package(fmt ...)" block into "if(WITH_SEASTAR)"
	src/rgw/rgw_common.h
- resolved for rgw_tag.h
	src/rgw/rgw_tag.h
- Resolved for RGWObjTags
@mattbenjamin @smithfarm
RGWLC: debug tags in rgwlc_s3_expiration_header
5eb8a1c 
Dump object tags to log at debug level 16.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
(cherry picked from commit 45f463f)
@mattbenjamin @smithfarm
rgw: complete expiration header (object tags)
ca4e3ef 
The expiration header tag processing is complete, but the
passed RGWObjTags argument was never initialized.  Now it is
initialized in RGWGetObj and RGWPutObj, as required.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
(cherry picked from commit 8981c5e)
@mattbenjamin @smithfarm
rgw: fix header timestamp
ca6228d 
The AWS example of this header intends to be RFC822-compliant.
Found by Tyler Brekke <tbrekke@redhat.com>.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
(cherry picked from commit 6da5be5)
@mattbenjamin @smithfarm
RGWLC: fix expiration header tag match
c4dc9ff 
Need to match key->value

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
(cherry picked from commit af327f2)
smithfarm
smithfarm reviewed Jan 25, 2020
View reviewed changes
src/rgw/rgw_lc.cc
// prefer header-only fmt, in general
#undef FMT_HEADER_ONLY
#define FMT_HEADER_ONLY 1
#include "fmt/format.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no fmt in nautilus - closing

@smithfarm
Copy link
Contributor

there is no fmt submodule in nautilus

@smithfarm smithfarm closed this Jan 25, 2020
@mattbenjamin mattbenjamin reopened this Jan 25, 2020
@mattbenjamin
Copy link
Contributor

in our downstream I experimented with using seastar/fmt as Kefu mentions, had trouble with jenkins, and just put the minimal header-only fmt files in src/rgw/fmt. n.b., this is considered a valuable feature by applications that use lifecycle expiration

Matt

@yuvalif
Copy link
Contributor

yuvalif commented Jan 26, 2020

pleas note that this PR is required so that the work in this PR: #31878
could be backported to nautilus.

@yuvalif yuvalif mentioned this pull request Jan 27, 2020
Merged
@yuvalif
Copy link
Contributor

yuvalif commented Jan 27, 2020

this PR: #32924 was created for backporting the above commits

@smithfarm
Copy link
Contributor

closing in favor of #32924

@smithfarm smithfarm closed this Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov left review comments

@smithfarm smithfarm smithfarm left review comments

Assignees
No one assigned
Labels
DNM
Projects
None yet
Milestone
nautilus
5 participants
@pdvian @tchaikov @smithfarm @mattbenjamin @yuvalif