Repository navigation
HDDS-13066. Return a clear bucket-link error when the linked source does not exist - #11368
rajeshkumarchandolu wants to merge 3 commits into
Conversation
sarvekshayr
left a comment
sarvekshayr
left a comment
There was a problem hiding this comment.
Thanks @rajeshkumarchandolu for working on this. Please find the below inline comments.
| OMException omEx = assertThrows(OMException.class, | ||
| () -> omSpy.resolveBucketLink(Pair.of(targetVolume, "dangling-link"))); | ||
| assertEquals(ResultCodes.BUCKET_NOT_FOUND, omEx.getResult()); | ||
| assertTrue(omEx.getMessage().contains("Cannot follow bucket link")); |
There was a problem hiding this comment.
Please use assertThat instead of assertTrue, see HDDS-9951.
| assertTrue(omEx.getMessage().contains("Cannot follow bucket link")); | |
| assertThat(omEx.getMessage()).contains("Cannot follow bucket link"); |
| OMException omEx = assertThrows(OMException.class, | ||
| () -> omSpy.listKeys(targetVolume, "dangling-link-list", null, null, 100)); | ||
| assertEquals(ResultCodes.BUCKET_NOT_FOUND, omEx.getResult()); | ||
| assertTrue(omEx.getMessage().contains("Cannot follow bucket link")); |
There was a problem hiding this comment.
Use assertThat instead of assertTrue here as well.
| ${missing} = Generate Random String 5 [NUMBERS] | ||
| ${missingVol} = Set Variable ${missing}-missing-source | ||
| Execute ozone sh bucket link ${missingVol}/any-bucket ${target}/dangling-missing-vol |
There was a problem hiding this comment.
Lets simplify this -
| ${missing} = Generate Random String 5 [NUMBERS] | |
| ${missingVol} = Set Variable ${missing}-missing-source | |
| Execute ozone sh bucket link ${missingVol}/any-bucket ${target}/dangling-missing-vol | |
| Execute ozone sh bucket link no-such-volume/no-such-bucket ${target}/dangling-missing-vol | |
There was a problem hiding this comment.
@sarvekshayr The Jira isn’t about link create failing when the source is missing — dangling links are allowed and create succeeds today. The issue is after the link exists: operations that follow the link (here key list on the target link bucket) could return VOLUME_NOT_FOUND when the source volume never existed, which is misleading.
This test is meant to mirror that: create the link to a non-existent source, then run key list on the link and check the remapped error/message. A hardcoded no-such-volume/no-such-bucket is fine with me if we keep that second step; I used a random volume name mainly to avoid name clashes in the suite. Happy to simplify the first line if you prefer.
| throw new OMException( | ||
| "Cannot follow bucket link: linked source bucket does not exist", | ||
| BUCKET_NOT_FOUND); |
There was a problem hiding this comment.
Include source details to improve error message.
| throw new OMException( | |
| "Cannot follow bucket link: linked source bucket does not exist", | |
| BUCKET_NOT_FOUND); | |
| throw new OMException( | |
| String.format("Cannot follow bucket link: linked source %s/%s does not exist", | |
| volumeName, bucketName), | |
| e, BUCKET_NOT_FOUND); |
There was a problem hiding this comment.
@sarvekshayr Thanks for the suggestion.
At this point in resolveBucketLink, volumeName and bucketName are the current hop that failed, not necessarily the bucket the user called or the final source. With chained links, putting those names in the message can look like a direct get against the wrong volume/bucket and confuse operators.
We kept the message generic on purpose (Cannot follow bucket link: linked source bucket does not exist): the failure should read as “could not follow the link,” not as a specific missing object name. Enriching the message with the original link target or true source would mean carrying extra context through the recursion (or more bookkeeping in the catch path) for little gain here—the common case is simply a dangling link, and the generic text is enough for that.
So I’d prefer to stay with the generic message for this Jira rather than add more state just for error-string formatting.
There was a problem hiding this comment.
Thanks @rajeshkumarchandolu . few minor comments below.
| @@ -94,6 +94,14 @@ Link to non-existent bucket | |||
| ${result} = Execute And Ignore Error ozone sh key list ${target}/dangling-link | |||
| Should Contain ${result} BUCKET_NOT_FOUND | |||
There was a problem hiding this comment.
nit: can we check the error message "Cannot follow bucket link" here also
| verify(bucketManager).getBucketInfo(eq(targetVolume), eq("dangling-link-list")); | ||
| verify(bucketManager).getBucketInfo(eq(missingSourceVolume), eq("any-bucket")); | ||
| } | ||
|
|
There was a problem hiding this comment.
can we add a unit test where the underlying failure is to throw BUCKET_NOT_FOUND instead of VOLUME_NOT_FOUND. The catch remaps both codes when visited is non-empty, unit tests only cover the volume-not-found path today.
3 participants
What changes were proposed in this pull request?
When a client strictly follows a bucket link (for example
ozone sh key liston a link bucket) and the linked source volume or bucket does not exist, Ozone previously often returnedVOLUME_NOT_FOUND, which is misleading because the link bucket itself may exist on a valid target volume. This change remaps that failure to a generic bucket-link message so operators see that the problem is a broken link target, not a missing volume on a direct lookup.The implementation adds a single catch in private
OzoneManager.resolveBucketLink: after at least one link hop has been recorded invisited, if resolution fails withVOLUME_NOT_FOUNDorBUCKET_NOT_FOUND, OM throwsBUCKET_NOT_FOUNDwith messageCannot follow bucket link: linked source bucket does not exist. Direct lookups (no link hop) and dangling-link create/metadata paths are unchanged. No newResultCodesor protoStatusvalues are introduced.Coverage: two unit tests in
TestBucketManagerImpl(resolveBucketLinkandlistKeys), and a new case inlinks.robotfor a link to a non-existent source volume.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13066
How was this patch tested?
mvn -pl :ozone-manager test -Dtest=TestBucketManagerImpl#testResolveBucketLinkMissingSourceVolume,TestBucketManagerImpl#testListKeysOnLinkWithMissingSourceVolume -DfailIfNoTests=false -DskipShade -DskipRecon -DskipDocsmvn clean install -DskipTests -Pdist -DskipShade -DskipRecon -DskipDocs, startedhadoop-ozone/dist/target/ozone-*-SNAPSHOT/compose/ozonewithOZONE_REPLICATION_FACTOR=3 ./run.sh -d, waited for safemode exit, then../test-single.sh om basic/links.robot— 19 tests, 19 passed (including Link to non-existent source volume).https://github.com/rajeshkumarchandolu/ozone/actions/runs/37463312443.