Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
object: remove default value us-east-1 as region from s3 client #9754
object: remove default value us-east-1 as region from s3 client #9754
Changes from all commits
9f0cf1a
5a2f5ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we mention the default here? Otherwise, I'm not sure what it means for Rook to "fill this value".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisn : I don't what I missed earlier if the user does not define the region, Rook will fill it from
Zonegroup
fromceph object store context
and don't useus-east-1
at all(at least that was my first intention of PR).us-east-1
is not the default value from RGW(at least not in RGW code base). It is a value that always works mysteriously. Maybe because a lot of s3 clients use this as default forregion
, but again not defined as default by RGWThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, does that mean rook can just take care of this internally? Why do we need to expose the
region
setting at all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisn : The
region
defined instorage class
of libbucket provisioner. Not sure whether Rook can hide it from user, maybe we can ignore the value. But this was available earlier release not sure whether it right to remove the variable. AFAIR I have sent #8766 as per request from the user, he wants to set the custom region for his use case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not very clear. What is the region set to if the user doesn't define it? "Accordingly" isn't clear enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BlaineEXE : earlier version was
Rook Operato fill this value from object store context internally
. But @travisn suggested it better not to give internal code details for the end-user. Hence changed like this. I can revert back the changeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even the text "fill this value from object store context internally" doesn't explain to the user what the value will be. In order for the user to understand when they need to modify the value, they must understand what the default value is. In words good for a user, what value will be the default?
This is also assuming that
region
is even a necessary parameter to have still.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay will update it again next push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a link to the example in the doc for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do users need to create all of the resources, or only zone group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly
zonegroup
is not an independent resource. Rooks mandates to haverealm
forzonegroup
and in RGW does not make sense to havezonegroup
withoutzones
(zonegroup is collection of zones and zone is collection of RGW servers in short)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this needs to match with the zone group, perhaps we should add documentation here in the example and also in the official docs to help users set this value correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BlaineEXE : I will try to add #9556 with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thotz Since the default is
us-east-1
, should we just leave this default uncommented?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit confused. What is the point of overriding
region
if the default is mapped toZoneGroup
and if the user has to create a ZoneGroup resource if they want to override region? Would it be simpler to just remove this parameter and rely on users to create aZoneGroup
instead?Is this only useful if there are multiple zone groups? What happens in the case where there are multiple zone groups? Then what is the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BlaineEXE :
region
is an optional value for storage-class. If it is not defined then we need to fill up withZonegroup
from object store context, This value changes in different scenarios:1.) normal cluster, it is name of object store
2.) external cluster it is
default
3.) if multiple zones are configured then it depends on value set in object-store CR.
If the user wants to use a custom region then he defines
Zonegroup
that's different scenario. Hope this avoids the confusionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all three of those cases, I don't think we need to have the
region
parameter.default
Is this not correct? Are there any cases missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BlaineEXE : Please see #9754 (comment) why the region is needed.
Above cases are right, but please note 2.) is not support in Rook atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a different region is specified in the storage class, will this client connection work with the store name? Or does it need to match the storage class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisn : I wondered magic behind
us-east-1
as region value. The actual value for s3 client needs tostoreName
and ideally we should have filled same value for storage class. Theus-east-1
works for RGW, I am guessing it skips checks(or maybe bug)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisn : [Another update]:
us-east-1
as a region no longer valid in master, it works with pacific though. So IMO we might see a similar issue when we start using new ceph versionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if we remove region from the default examples in rook, it will work with ceph pacific and master? Or will we need different examples for pacific vs quincy? And to be clear, does the ceph master change also affect quincy, or just post-quincy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it will work, this fills internal by Rook. Earlier we are using
us-east-1
from this patch we add the actual zone of the rgw server that's the difference.The
us-east-1
worked in quincy as well, but I don't see much code difference in that path for master and Quincy. Need to have a bit of time to understand thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documented examples we are working on in the other comments only apply to Quincy and newer, correct? So what is the behavior for region in Pacific? The docs need to give that detail, especially since Pacific is still the default version with Rook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisn : Sorry if I created confusion before for RGW in any versions,
region
is thezone group
but (again true for all versions) if we give region asus-east-1
it will bypass some checks in RGW and request succeed. I felt it is kind of a hack. But without knowing much about this context Rook was settingus-east-1
for the internal aws s3 SDK and this was working fine for our use case. But this may create confusion for users when they useus-east-1
works but other region's requests are failing. By this PR we are avoiding hack and using the required valueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still confused about how users can understand the region. If
us-east-1
is a workaround and a hack, maybe we should just keep that for the simple, local rgw case. What does the user gain by using a different region? Seems like it should only be important for external or multi-site where we need the region name in order to connect. But if it's just the local cluster, can we hide the region from the user so they don't need to worry about it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's find a time to discuss this next week since I'm having such a hard time. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisn: Sure :). Sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the storeName used here, but the zonegroup is used in the example for using a different region? Shouldn't this use the zonegroup as well? Or in the test, the storeName and zonegroup are the same anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this should be zone group, since we testing only internal ceph cluster without multisite, I just set directly
storeName
which is value ofzonegroup
as well