HDDS-11705. Snapshot operations on linked buckets should work on actual underlying bucket by swamirishi · Pull Request #7434 · apache/ozone

swamirishi · 2024-11-14T17:43:34Z

What changes were proposed in this pull request?

Snapshot operations on linked buckets create a snapshot on a linked bucket. However, there are potential downsides since the key prefixes in the FileTable/KeyTable/DirTable would correspond to the prefix of the actual bucket. In such a case even though a snapshot is created on the linked bucket but an SSTFilteringService run on the snapshot, the fileTable, dirTable, and keyTable would be empty since there would be no keys with the linked volume and linked bucket prefix. The same problem with other operations on the snapshot as well CreateSnapshot, DeleteSnapshot, RenameSnapshot, GetSnapshot, get key from snapshot, listing snapshots, listing snapDiff jobs, and submitting a snapshot diff job.

The solution is to resolve the bucket link to the actual bucket and perform all the snapshot operations on the actual bucket instead of the linked bucket

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tests & integration tests to follow

Change-Id: I821e19b67e405d9bdffc49ad5fe8f483f224a6dd

…on actual bucket Change-Id: I23b167deca04f5d6d8156c011300f005351892fc

Change-Id: I2138306c1a3077ed4b77aeb1d2366c7acb8d2d66

Change-Id: I12e98b5a4f5896f1871f8b019fd6929f37e39f4a

hemantk-12

Thanks @swamirishi for the quick patch.

Changes look good to me, Let's add tests around this. Specially for SnapshotDiff and listing key on Snapshot on a linked bucket.

prashantpogde · 2024-11-15T00:56:14Z

Overall changes look good to me. Let run a full range of snapshot operations on a linked bucket to be sure that we didn't miss anything.

Change-Id: Ie9fec98f741c875362e904ad48b00bf9c1f286b7

swamirishi · 2024-11-15T02:35:53Z

Overall changes look good to me. Let run a full range of snapshot operations on a linked bucket to be sure that we didn't miss anything.

That's the plan. I have made TestOmSnapshot & TestOmSnapshotFileSystemParameterized and testing a full test run on it

Change-Id: I73ca6a308e1aa4f2fa6d4de224881803b078ef25

sumitagrawl

Few comments is given, and IMO, we should not allow snapshots over linked buckets.

Change-Id: Ib85be9bdb30ec1ea7ad055ad6cbd696ff886bcff

… perform check only external requests Change-Id: If23f82aa5681588ac32bedb8def12d43c7bd8a5a

sadanand48 · 2024-11-15T06:12:28Z

 if (isSnapshotKey(keyParts)) {
 String snapshotName = keyParts[1];
-
+ // Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a


The method above this "deleteKeysFromDelKeyTableInSnapshotScope" also needs correction. Or are we planning to handle garbage collection separately?

We have changed the snapshot sym link bucket name to the actual bucket name. We don't have to do anything about garbage collection code.

Ok you mean that the callers ensure that the method is passed the real bucket name. Makes sense in that case.

Change-Id: I65418735f650ecf4f3e6901617f8b01e5702f146

Change-Id: I9cf926bfd4cf79dfd3709622108220128bbf5751

Change-Id: I4e4bfc336e70a4d5f99cb02701e494d5ea64ec4b

Change-Id: I6fd76cdbc5bce189e0d12e2295e4119f9082a317

Change-Id: I43d6a8b49ecdb4981774fa0bb53b50159f730668

Change-Id: I290d4a4ca4eb386fe9d4ccf7a8e0111d5b4017b6

Change-Id: I580ee29560a4e7b22ea80a8654dab1e392f03935

prashantpogde · 2024-11-15T17:52:58Z

@sumitagrawl we did consider multiple links to the same destination bucket impact. It doesn't seem like it can create any problem because all the snapshot operations are still happening on the destination bucket. Let us know If you see any specific problem in allowing this.

sumitagrawl · 2024-11-15T17:54:30Z

-
+ // Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a
+ // permission check, since linked bucket permissions and source bucket permissions could be different.
+ ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink(Pair.of(volumeName,


respective caller already have acl check, do this acl check additionally while getting snapshot db reference is required? It may be redundant, as used while getting metadaReader.
IMO, should be done at entry point, not in some common methods.

sumitagrawl · 2024-11-15T17:57:06Z

@sumitagrawl we did consider multiple links to the same destination bucket impact. It doesn't seem like it can create any problem because all the snapshot operations are still happening on the destination bucket. Let us know If you see any specific problem in allowing this.

As such no problem in implementation, may be caution to be added to avoid creation of multiple snapshots unknowingly, for optimization in snapshot creation.

Change-Id: Ibaf927b11d9f617ac738921f675a0e6d3631b71f

hemantk-12 · 2024-11-15T19:35:56Z

+ .setVersioning(false)
+ .setSourceVolume(vol)
+ .setSourceBucket(sourceBucketName);
+ return createBucket(client, vol, bb.build(), linkedBucketName);


Shouldn't we need to call createBucket with true flag? Currently, it is calling createBucket which defaults to not linked bucket.

createLinkedBucket is called by createBucket when the flag is true.

When the sourceBucket is present it creates a linked bucket

hemantk-12 · 2024-11-15T19:56:21Z

 try {
+ // Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a
+ // permission check, since linked bucket permissions and source bucket permissions could be different.
+ ResolvedBucket resolvedBucket = resolveBucketLink(Pair.of(volumeName, bucketName), false);


nit: you can call resolveBucketLink which is default to false.

hemantk-12 · 2024-11-15T20:03:23Z

+ // Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a
+ // permission check, since linked bucket permissions and source bucket permissions could be different.
+ ResolvedBucket resolvedBucket = resolveBucketLink(Pair.of(volumeName, bucketName), false);
+ auditMap = buildAuditMap(resolvedBucket.realVolume());


We don't keep audit logs for read operations. It might been copied-pasted from the write API. If agree, we can create a clean jira.

yup we can create a followup jira

Change-Id: Ia080b9985766f9d6f9c4f45250db2d976334d943

Change-Id: I5262aeaf17498582fc6c46d041a8a27f99dc73b8

swamirishi · 2024-11-16T01:18:55Z

@hemantk-12 if you are good with the change we can merge this.

hemantk-12

LGTM+1

hemantk-12 · 2024-11-16T01:22:46Z

Thanks @swamirishi for the patch, @prashantpogde @sumitagrawl @sadanand48, and @adoroszlai for the review.

smengcl · 2024-11-16T04:51:53Z

+ // Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a
+ // permission check, since linked bucket permissions and source bucket permissions could be different.


I find this comment misleading.
When accessing a linked bucket, the ACL should be checked along the way. Not just on the (final) destination bucket.
That means, if:
C (linked) -> B (linked) -> A regular bucket
The user has to at least have READ permission on all of C, B and A buckets to be able to pass the ACL check.
ANY lack of permission along the chain should result in "operation denied".
At least that is how the current Ozone ACL design imposes checks on all other operations.

HDDS-11705. Snapshot diff fails on linked buckets
62a2c1c
Change-Id: I821e19b67e405d9bdffc49ad5fe8f483f224a6dd

swamirishi requested a review from hemantk-12 November 14, 2024 17:44

HDDS-11705. Snapshot operations should on linked buckets should work …
2a3c55a
…on actual bucket Change-Id: I23b167deca04f5d6d8156c011300f005351892fc

swamirishi changed the title ~~HDDS-11705. Snapshot diff fails on linked buckets~~HDDS-11705. Snapshot operations should on linked buckets should work on actual underlying bucketNov 15, 2024

HDDS-11705. Revert fixes made on snapshot flows
77f69a2
Change-Id: I2138306c1a3077ed4b77aeb1d2366c7acb8d2d66

swamirishi requested review from aswinshakil, prashantpogde, sadanand48 and smengcl November 15, 2024 00:19

hemantk-12 reviewed Nov 15, 2024
View reviewed changes

Comment threadhadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Comment threadhadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java Outdated

HDDS-11705. Address review comments
df52e73
Change-Id: I12e98b5a4f5896f1871f8b019fd6929f37e39f4a

hemantk-12 reviewed Nov 15, 2024
View reviewed changes

Comment threadhadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java Outdated

hemantk-12 reviewed Nov 15, 2024
View reviewed changes

hemantk-12 changed the title ~~HDDS-11705. Snapshot operations should on linked buckets should work on actual underlying bucket~~HDDS-11705. Snapshot operations on linked buckets should work on actual underlying bucketNov 15, 2024

HDDS-11705. Add test cases
3115158
Change-Id: Ie9fec98f741c875362e904ad48b00bf9c1f286b7

HDDS-11705. Address review comments
516f0a3
Change-Id: I73ca6a308e1aa4f2fa6d4de224881803b078ef25

sumitagrawl reviewed Nov 15, 2024
View reviewed changes

Comment thread...nager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java
Comment threadhadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java Outdated

swamirishi added 2 commits November 14, 2024 21:20

HDDS-11705. Fix tests
7830c2b
Change-Id: Ib85be9bdb30ec1ea7ad055ad6cbd696ff886bcff

HDDS-11705. Address review comments and move resolve check outside to…
a286afc
… perform check only external requests Change-Id: If23f82aa5681588ac32bedb8def12d43c7bd8a5a

sadanand48 reviewed Nov 15, 2024
View reviewed changes

HDDS-11705. Revert test case change
ce0fed0
Change-Id: I65418735f650ecf4f3e6901617f8b01e5702f146

adoroszlai added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Nov 15, 2024

swamirishi added 6 commits November 14, 2024 23:47

HDDS-11705. Fix bats
50bc68b
Change-Id: I9cf926bfd4cf79dfd3709622108220128bbf5751

HDDS-11705. Fix bats
d638499
Change-Id: I4e4bfc336e70a4d5f99cb02701e494d5ea64ec4b

HDDS-11705. fix bats
82154a6
Change-Id: I6fd76cdbc5bce189e0d12e2295e4119f9082a317

HDDS-11705. Add linked bucket info in snapshot info
d0d5e91
Change-Id: I43d6a8b49ecdb4981774fa0bb53b50159f730668

HDDS-11705. Fix checkstyle
029655d
Change-Id: I290d4a4ca4eb386fe9d4ccf7a8e0111d5b4017b6

HDDS-11705. Revert commented code
e5353c7
Change-Id: I580ee29560a4e7b22ea80a8654dab1e392f03935

sumitagrawl reviewed Nov 15, 2024
View reviewed changes

swamirishi marked this pull request as draft November 15, 2024 19:17

HDDS-11705. Fix test case
85daf58
Change-Id: Ibaf927b11d9f617ac738921f675a0e6d3631b71f

hemantk-12 reviewed Nov 15, 2024
View reviewed changes

swamirishi added 2 commits November 15, 2024 12:32

HDDS-11705. Address review comments
0bee4e3
Change-Id: Ia080b9985766f9d6f9c4f45250db2d976334d943

HDDS-11705. Fix build issue
a861e5a
Change-Id: I5262aeaf17498582fc6c46d041a8a27f99dc73b8

swamirishi marked this pull request as ready for review November 15, 2024 23:38

hemantk-12 approved these changes Nov 16, 2024
View reviewed changes

hemantk-12 merged commit f60ad61 into apache:masterNov 16, 2024

smengcl reviewed Nov 16, 2024
View reviewed changes

smengcl mentioned this pull request Nov 17, 2024
HDDS-11732. Skip ACL check on bucket resolution while reading from snapshot #7446
Merged

This was referenced Jun 3, 2026
HDDS-15473. TestOmSnapshotObjectStore should run with createLinkedBucket=false #10425
Merged
HDDS-15474. Drop redundant TestOmSnapshot linked-bucket subclasses #10432
Merged

		// Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a
		// permission check, since linked bucket permissions and source bucket permissions could be different.

Uh oh!

Conversation

swamirishi commented Nov 14, 2024• edited by hemantk-12 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hemantk-12 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prashantpogde commented Nov 15, 2024

Uh oh!

swamirishi commented Nov 15, 2024

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

prashantpogde commented Nov 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitagrawl commented Nov 15, 2024

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!

Uh oh!

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!

swamirishi commented Nov 16, 2024

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

hemantk-12 commented Nov 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

swamirishi commented Nov 14, 2024•
edited by hemantk-12
Loading

hemantk-12 left a comment •
edited
Loading