HPCC-30570 Use default data plane if no destGroup req in FileSpray.Copy #18013

wangkx · 2023-11-09T16:23:48Z

Use default data plane for remote/foreign copy if no destGroup req.

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-11-09T16:33:57Z

https://track.hpccsystems.com/browse/HPCC-30570
Jira updated

jakesmith · 2023-11-15T11:16:06Z

esp/services/ws_fs/ws_fsService.cpp

        {
-            getNodeGroupFromLFN(context, srcname, destNodeGroup);
+            if(isContainerized() && !isEmptyString(srcDali))


If dali specified performing a copy in containerized...
That should normally be forbidden, it is at least very unusual (implies containerized has been configured with allowForeign=true).

I don't think that was what @cloLN was testing when he hit the problem.
As you can see from the JIRA srcDali was blank (as you'd expect),
destGroup was also blank, and it's that that caused the problem.

I think the issue is that the file was ~remote, and getNodeGroupFromLFN isn't using wsdfs::lookup.
(anywhere that might lookup files from a remote source should now be using wsdfs::lookup)

@wangkx - can you check 'lookupLogicalName' in esp/smc/SMCLib/LogicFileWrapper.cpp, change to wsdfs::lookup and test ?

@jakesmith I am adding the code to call the wsdfs::lookup if the srcname isRemote. I got the error "Remote storage 'test_remote_storage_name' not found". It is because, I think, I haven't set the remote storage 'test_remote_storage_name'. To test/debug, I am going to test the remote copy between HPCCs on my local Ubuntu VM and on my local Docker Desktop. Is it possible? How should I configure my environment.xml and my values.yaml to test the remote copy?

if the srcname isRemote.

you should not need the test. wsdfs::lookup, will fallback to legacy DFS if needed.

I think, I haven't set the remote storage 'test_remote_storage_name'.

you need to setup a definition on the bare-metal side.
See: https://track.hpccsystems.com/browse/HPCC-27688 (also, looking at the 160x setup may be helpful).

copy between HPCCs on my local Ubuntu VM and on my local Docker Desktop. Is it possible?

Yes, you can hit a local k8s hpcc running in DD.

Reach out to me on Teams if you need guidance.

I copied the setup from 160x (will need your guidance on how to hit the local k8s later). I still got the same "Remote storage 'name' not found" error. In #16123, I saw the changes on several xsl files (no esp related?). Not sure that ESP can reach out the setup.

@jakesmith when I tried to remote copy a file from my local DD to my local bare-metal VM, I got this error: 'Bare metal does not support remote file access to planes without hosts'. Is it possible to add the hosts to the DD planes?

jakesmith

@wangkx - please see comment.

jakesmith

@wangkx - see reply.

jakesmith · 2023-12-07T10:16:15Z

esp/smc/SMCLib/LogicFileWrapper.cpp

@@ -107,7 +108,7 @@ IDistributedFile* lookupLogicalName(IEspContext& context, const char* logicalNam
        userDesc->set(userID, context.queryPassword(), context.querySignature());
    }

-    Owned<IDistributedFile> df = queryDistributedFileDirectory().lookup(logicalName, userDesc, accessMode,
+    Owned<IDistributedFile> df = wsdfs::lookup(logicalName, userDesc, accessMode,


@wangkx - is this change directly related to the issue that's being fixed?
i.e. is this lookupLogicalName used in the context of a copy from a remote file ? (I'm not sure I see where from CFileSprayEx::onCopy)

May not be related to this issue. I changed it because of your comment in #18013 (comment), and because it is called by CFileSprayEx::onCopy through the getNodeGroupFromLFN(). If you want, I can change it back.

right, if it had to call getNodeGroupFromLFN .. then wouldn't work unless it was using wsdfs::, but as discussed it doesn't make sense for getNodeGroupFromLFN to be used in the context of a out of environment logical file, at least not for this purpose.

I think it better that this change be removed from this PR, but it probably does make sense as part of a separate change. Anything that might be used to lookup a remote file should use wsdfs, please open a separate JIRA.

jakesmith

@wangkx - see question.

wangkx · 2023-12-07T19:16:44Z

@jakesmith the changes related to the wsdfs::lookup call have been removed. I created https://track.hpccsystems.com/browse/HPCC-30985.
I also noticed that the getDefaultStoragePlane() does not return a plane for bare metal.

jakesmith · 2023-12-07T19:41:18Z

esp/services/ws_fs/ws_fsService.cpp

+            if(!isEmptyString(srcDali) || lfn.isForeign() || lfn.isRemote())
+            {
+                //makes no sense to get the srcname's current group, if a logical file is from a different environment.
+                getDefaultStoragePlane(destNodeGroup);


I also noticed that the getDefaultStoragePlane() does not return a plane for bare metal.

In that case, yes, there is only the concept of a default plane in containerized...
This should not call it in bare-metal.
But that means, in bare-metal if no destNodeGroup is supplied - it should fire an error - there is no sensible default.
We could consider allowing a default to be defined in bare-metal to be more consistent, for such times, but I'm not sure it's worth it.

jakesmith

@wangkx - see comment re. no default , should fire an error in bare-metal.

jakesmith

@wangkx - looks good. Please squash.

jakesmith · 2023-12-12T11:59:09Z

esp/services/ws_fs/ws_fsService.cpp

        const char* destNodeGroupReq = req.getDestGroup();
-        if(!destNodeGroupReq || !*destNodeGroupReq)
+        if(isEmptyString(destNodeGroupReq))


picky: there was probably a mix before, but worth making formatting consistent within a function you're modifying, e.g. missing space after if here and on new code on line 2700.

Use default data plane for remote/foreign copy if no destGroup req Signed-off-by: wangkx <kevin.wang@lexisnexis.com>

wangkx · 2023-12-12T14:51:11Z

Squashed. Rebased. Tested.
@ghalliday this PR is ready to be merged.

wangkx requested a review from jakesmith November 9, 2023 20:15

jakesmith reviewed Nov 15, 2023

View reviewed changes

jakesmith requested changes Nov 15, 2023

View reviewed changes

wangkx requested a review from jakesmith November 20, 2023 22:36

jakesmith reviewed Nov 22, 2023

View reviewed changes

wangkx requested a review from jakesmith December 5, 2023 22:20

jakesmith reviewed Dec 7, 2023

View reviewed changes

wangkx requested a review from jakesmith December 7, 2023 19:16

jakesmith reviewed Dec 7, 2023

View reviewed changes

wangkx requested a review from jakesmith December 11, 2023 14:00

jakesmith approved these changes Dec 12, 2023

View reviewed changes

HPCC-30570 Use default data plane if no destGroup req in FileSpray.Copy

251c140

Use default data plane for remote/foreign copy if no destGroup req Signed-off-by: wangkx <kevin.wang@lexisnexis.com>

wangkx force-pushed the h30570 branch from 6517150 to 251c140 Compare December 12, 2023 13:40

ghalliday merged commit caf7a15 into hpcc-systems:candidate-9.4.x Dec 14, 2023
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30570 Use default data plane if no destGroup req in FileSpray.Copy #18013

HPCC-30570 Use default data plane if no destGroup req in FileSpray.Copy #18013

wangkx commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023

jakesmith Nov 15, 2023

wangkx Nov 15, 2023 •

edited

Loading

jakesmith Nov 22, 2023

wangkx Nov 22, 2023

wangkx Dec 4, 2023

jakesmith left a comment

jakesmith left a comment

jakesmith Dec 7, 2023

wangkx Dec 7, 2023

jakesmith Dec 7, 2023

jakesmith left a comment

wangkx commented Dec 7, 2023

jakesmith Dec 7, 2023

jakesmith left a comment

jakesmith left a comment

jakesmith Dec 12, 2023

wangkx Dec 12, 2023

wangkx commented Dec 12, 2023

HPCC-30570 Use default data plane if no destGroup req in FileSpray.Copy #18013

HPCC-30570 Use default data plane if no destGroup req in FileSpray.Copy #18013

Conversation

wangkx commented Nov 9, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 9, 2023

Choose a reason for hiding this comment

wangkx Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

wangkx commented Dec 7, 2023

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx commented Dec 12, 2023

wangkx commented Nov 9, 2023 •

edited

Loading

wangkx Nov 15, 2023 •

edited

Loading