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.

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

HDDS-5279. OFS mkdir -p does not work when Volume is not pre-created #2412

Merged
merged 4 commits into from
Jul 29, 2021

Conversation

smengcl
Copy link
Contributor

@smengcl smengcl commented Jul 13, 2021

https://issues.apache.org/jira/browse/HDDS-5279

ClientProtocol#getBucketDetails can throw VOLUME_NOT_FOUND when ACL is enabled because checkAcls() will attempt to retrieve volume owner with getVolumeOwner(), which getBucket() didn't handle previously.

Thanks @avijayanhwx for the initial fix.

Updated the UT with new parameter.

smengcl added 3 commits July 13, 2021 14:14
Change-Id: I9e59ec7c2c895dd3c70ef20e6beba4a113ea2f24
Change-Id: Ifb321f6d1278c8c48c282876be586220cfcd288c
Change-Id: I9c288a4ffa66a66a25819282d6a09fedfae1fba9
// when ACL is disabled. Both exceptions need to be handled.
switch (ex.getResult()) {
case VOLUME_NOT_FOUND:
case BUCKET_NOT_FOUND:
Copy link
Contributor

@kerneltime kerneltime Jul 18, 2021

Choose a reason for hiding this comment

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

The case where volume is not found should not be as common as bucket not found. I think it would be better to avoid the redundant call to create volume.

Copy link
Contributor Author

@smengcl smengcl Jul 19, 2021

Choose a reason for hiding this comment

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

Thanks for the comment @kerneltime .

VOLUME_NOT_FOUND is actually frequently encountered during testing where we create volume+bucket+directories on one shot with mkdir -p when ACL is enabled.

If you think about it, in both cases the action(s) we need to take is the same. i.e. volume needs to be created.
It's just that the exception thrown from the call proxy.getBucketDetails is different -- which is bizzare, and we maybe want to fix getBucketDetails's behavior at some point (for example, to always throw VOLUME_NOT_FOUND regardless of whether ACL is enabled or not), after which we can throw away case BUCKET_NOT_FOUND.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to create a newbie jira to clean up the behavior to send the appropriate error which is the right fix.

Copy link
Contributor Author

@smengcl smengcl Jul 21, 2021

Choose a reason for hiding this comment

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

Yes I will file one. but probably not a newbie one since changing that could involve a series of changes -- throwing a different exception can break other existing behaviors.

Change-Id: Id2a5bca01f7300a4c190f0f16a9cc09e00644f62
@bshashikant
Copy link
Contributor

@smengcl , correct if i m wrong. Volumes in ozone are supposed to be created by admins only as per the doc. Are we going to break this notion here?

@smengcl
Copy link
Contributor Author

smengcl commented Jul 22, 2021

@smengcl , correct if i m wrong. Volumes in ozone are supposed to be created by admins only as per the doc. Are we going to break this notion here?

Nope. The behavior hasn't changed. This is client code. Server (OM) is responsible for checking if the client has the permission to create a volume. In this case, if a regular user (non-admin) tries to do mkdir -p ofs://om/vol2/bucket2/ and vol2 doesn't exist, the client will be rejected.

@smengcl smengcl merged commit 1c6e0fb into apache:master Jul 29, 2021
@smengcl
Copy link
Contributor Author

smengcl commented Jul 29, 2021

Thanks @kerneltime and @avijayanhwx for the reviews. I have merged this.

errose28 added a commit to errose28/ozone that referenced this pull request Jul 30, 2021
* master: (48 commits)
  HDDS-5514. Skip check for UNHEALTHY containers for datanode finalize. (apache#2469)
  HDDS-5279. OFS mkdir -p does not work when Volume is not pre-created (apache#2412)
  HDDS-5328. Remove delete container command from admin CLI (apache#2456)
  HDDS-5382. Increase default container report interval to 60 mins (apache#2363)
  HDDS-5378 Add APIs to retrieve Namespace Summary from Recon (apache#2417)
  HDDS-5466. Refactor BlockOutputStream. (apache#2442)
  HDDS-5465. Delete redundant code when set、add and remove bucket acl (apache#2439)
  HDDS-5184. Use separate DB profile for Datanodes. (apache#2214)
  HDDS-5494. Reduce retry in Kubernetes test (apache#2461)
  HDDS-5414. Data buffers incorrectly filtered for Ozone Insight (apache#2387)
  HDDS-5450. Avoid refresh pipeline for S3 headObject (apache#2431)
  HDDS-5500. New k3s version breaks kubernetes test (apache#2464)
  HDDS-5489. Install OS-specific flekszible (apache#2462)
  Multi-raft style placement with permutations for offline data generator (apache#2434)
  HDDS-5484. Intermittent failure in TestReplicationManager#testMovePrerequisites (apache#2454)
  HDDS-5443 Create and then recreate a bucket with a randomized name (apache#2436)
  HDDS-5492. Disable failing kubernetes test (apache#2459)
  HDDS-4330. Bootstrap new OM node (apache#1494)
  HDDS-5418. Let Recon send reregisterCommand to Datanodes if DatanodeDetails changed (apache#2392)
  HDDS-5479. s3g bucket list failed when there is non-english key name. (apache#2450)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants