object: unique username for OBC even when preceding OBC was retained #12884
Conversation
haslersn
force-pushed
the
unique-username-for-obc
branch
3 times, most recently
from
a8851ba
to
a42098c
Compare
|
I think this is working to address the same class of bug as this other PR #12880 (comment). I believe that this implementation is a simpler approach that I would prefer. One requirement, @haslersn : please copy the commit description to this PR itself for easier discussion within GitHub. |
| // user name can be deterministically generated from the OBC's UID. We | ||
| // cannot simply use the OBC's namespace and name, because they can be | ||
| // reused, while the preceding bucket might be retained by reclaimPolicy. | ||
| return "obc-" + string(obc.UID) |
There was a problem hiding this comment.
Personally, I like the idea of keeping obcNamespace and obcName here to help with visual inspection of users on the backend and mapping them into the k8s domain. This isn't a hard requirement, but I believe it's a worthwhile "hint" that can help when doing complex debugging, and it's essentially free.
| return "obc-" + string(obc.UID) | |
| return "obc-" + obc.Namespace + "-" obc.Name + string(obc.UID) |
There was a problem hiding this comment.
I think this can be disregarded. While namespace names are limited to 63 chars, the k8s spec says that resource names can be up to 253, which is approaching the known 255 limit for obc "friendly" user names. While I seem to recall being limited to 63 chars for CR names, I may be mistaken, and it seems safest to proceed with this PR as-is.
For an OBC's name we cannot simply use the OBC's namespace and name, because they can be reused, while the preceding bucket might be retained by reclaimPolicy. (Commit 2733375 also didn't solve this issue, as it is an intra-cluster issue.) Therefore we instead use the OBC's UID which should be unique within the cluster and across clusters. Signed-off-by: Sebastian Hasler <sebastian.hasler@stuvus.uni-stuttgart.de>
haslersn
force-pushed
the
unique-username-for-obc
branch
from
a42098c
to
041fc1a
Compare
|
Considering point 2 of Travis' question here: #12880 (comment) I think it would be good to create an OBC before this change and then ensure it still works afterwards. I thought we checked this in Rook's upgrade test, but I am seeing that isn't the case. |
Related Issue describing the need for OBC testing during upgrade CI: #12887 |
|
I tested this PR by cherry-picking 041fc1a on top of v1.12.3, building and deploying that. I confirm that now deleting and recreating an ObjectBucketClaim with the same namespace and name works, even if the previous bucket is retained. |
|
@travisn thoughts on 1.11 bp here? |
Sounds good to could include. In general, if someone asks for a 1.11 backport we can still do it if needed. |
object: unique username for OBC even when preceding OBC was retained (backport #12884)
object: unique username for OBC even when preceding OBC was retained (backport #12884)
Description of your changes:
For an OBC's name we cannot simply use the OBC's namespace and name, because they can be reused, while the preceding bucket might be retained by reclaimPolicy. (PR #11665 also didn't solve this issue, as it is an intra-cluster issue.) Therefore we instead use the OBC's UID which should be unique within the cluster and across clusters.
Similar to how it was after #11665, existing users will continue to work, i.e., rook will not update the
ObjectBucket.spec.additionalState.cephUserfield of existing buckets.Which issue is resolved by this Pull Request:
Resolves #11345
Checklist: