HPCC-30368 Add packagemap support for remoteStorage property #19148

asselitx · 2024-09-24T22:04:37Z

Similar to how the daliip property is supported in the packagemap file, add support for a remoteStorage property that mirrors the --remote-storage CLI option.

Apply the same scoping rules as for daliip- values defined deeper in the hierarchy take precedence, and anything defined in the file takes precedence over a CLI option.
Disallow use of both daliip and remoteStorage in the same element.

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:

Manually ran seven tests on constellation of three clusters:

A local k8s roxiecluster that is the target of packagemap and query deployments, configured to copy files locally.
A second local k8s thorcluster configured as a remote storage location for the first.
The Alpha30 used as a daliip source.

Each test verified that the nonlocal files were copied to local storage:

Package File <SuperFile> remoteStorage overrides CLI daliip. This is the case needed by the requester.
Package File <Package> remoteStorage overrides <PackageMaps> remoteStorage
Package File <SuperFile> remoteStorage overrides <PackageMaps> remoteStorage
Package File <SuperFile> daliip overrides CLI remote-storage, but there is another <SuperFile> in the package file that correctly resolves with the CLI remote-storage value.
CLI daliip
CLI remote-storage
Package File <SuperFile remoteStorage> overrides <PackageMap daliip>

Similar to how the daliip property is supported in the packagemap file, add support for a remoteStorage property that mirrors the `--remote-storage` CLI option. - Apply the same scoping rules as for daliip- values defined deeper in the hierarchy take precedence, and anything defined in the file takes precedence over a CLI option. - Disallow use of both daliip and remoteStorage in the same element. Signed-off-by: Terrence Asselin <[email protected]>

asselitx · 2024-09-24T22:08:43Z

Before moving out of draft, there are a few tasks to complete:

Test more combinations of options for using the daliip and remoteStorage together and at different levels of the packagemap file.
Ensure I'm targeting the right branch and rebase if required.
Create Jira for documentation change

github-actions · 2024-09-24T22:09:08Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-30368

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

asselitx · 2024-09-24T22:14:25Z

@afishbeck My biggest questions are:

Is the method of storing the 'effective' remoteStorage name in the ReferencedFile object sound, or is there a cleaner approach I didn't see?
Is it reasonable to keep a separate function (splitDerivedDfsLocationOrRemote) for the purposes of processing the packagemap files (as opposed to where splitDerivedDfsLocation is used elsewhere)? It seemed that adding more features to the existing function would be over-complicating something that I would already have a hard time explaining what it did.

afishbeckln · 2024-09-24T22:36:05Z

common/pkgfiles/referencedfilelist.cpp

+            else
+                remoteStorage.set(remoteLocation);
+            file.resolveLocalOrRemote(locations, srcCluster, user, remoteStorage, remotePrefix, checkLocalFirst, expandSuperFiles ? &subfiles : NULL, trackSubFiles, resolveLFNForeign);
+            remoteStorage.clear();


Does the ability to clear this here mean that remoteStorage is a class member variable that is just being used here for temporary storage? Is that necessary?

I will re-confirm that this is the case, but I recall that resolveLocalOrRemote uses the class member variable for its work. But then I was concerned about side-effects since it wasn't previously set at this location. Perhaps it isn't necessary and should be a local temp.

This wasn't correct and is adjusted in the next commit.

afishbeckln · 2024-09-24T22:55:30Z

common/pkgfiles/referencedfilelist.cpp

+                                    const char *defaultCluster, const char *baseCluster, const char *baseIP, const char *basePrefix,
+                                    const char *currentRemoteStorage, StringBuffer& effectiveRemoteStorage, const char *baseRemoteStorage)
+{
+    if (!isEmptyString(address) && !isEmptyString(currentRemoteStorage))


You're right that it shouldn't happen at the same level. I think you do support what Ananth wants which is to for example have the packagemap define a remoteStorage, but some packages inside it point to a daliIP?

I believe it should support that case, but I need to confirm with a test.

I've tested behavior where a CLI setting of either daliip or remote-storage is overridden by either daliip or remoteStorage set in the packagemap file. Also, those properties defined in the file have a precedence over each other- a property defined in an element will override the value defined in its ancestors.

afishbeckln · 2024-09-24T22:56:41Z

@afishbeck My biggest questions are:

Is the method of storing the 'effective' remoteStorage name in the ReferencedFile object sound, or is there a cleaner approach I didn't see?

Is it reasonable to keep a separate function (splitDerivedDfsLocationOrRemote) for the purposes of processing the packagemap files (as opposed to where splitDerivedDfsLocation is used elsewhere)? It seemed that adding more features to the existing function would be over-complicating something that I would already have a hard time explaining what it did.

Putting it in ReferencedFile is likely the least complicated option without doing some refactoring over all. If huge numbers of ReferencedFiles were being used you could find ways of reducing memory usage but I don't think that's a huge concern?
Yes I think the way you organized the change to the splitDerivedABC calls makes sense.

asselitx · 2024-09-25T16:14:01Z

@ghalliday Finalizing is still pending on running a few tests.

ghalliday

I didn't see any logic flaws, but a couple of instances where the variable names are confusing.

ghalliday · 2024-09-26T09:27:01Z

common/pkgfiles/referencedfilelist.cpp

 {
    StringBuffer ip;
    StringBuffer cluster;
    StringBuffer prefix;
-    splitDerivedDfsLocation(superFile.queryProp("@daliip"), cluster, ip, prefix, NULL, _cluster, _ip, _prefix);
+    StringBuffer remoteStorageName;


This is confusing having a variable remoteStorageName and _remoteStorageName. Can you rename one (or both) to clarify the difference.
Same applies to the next function.

ghalliday · 2024-09-26T10:25:14Z

common/pkgfiles/referencedfilelist.cpp

@@ -126,13 +126,38 @@ void splitDerivedDfsLocation(const char *address, StringBuffer &cluster, StringB
    prefix.append(basePrefix);
 }

+void splitDerivedDfsLocationOrRemote(const char *address, StringBuffer &cluster, StringBuffer &ip, StringBuffer &prefix,


I don't suggest it for this PR (or even as a priority), but this function has too many parameters, with little logical ordering - if it was refactored it would be good to think about which parameters should be grouped together, and possibly moved into a class.

* Rename variables to be more descriptive * When parsing the packagemap file, correctly propagate the resolution method (daliip vs remoteStorage) to the next level of parsing. Signed-off-by: Terrence Asselin <[email protected]>

ghalliday

I didn't see any problems.
@afishbeckln please take another look

afishbeckln

Looks good to me.

asselitx requested a review from afishbeck September 24, 2024 22:04

afishbeckln reviewed Sep 24, 2024

View reviewed changes

ghalliday removed the request for review from afishbeck September 25, 2024 15:52

asselitx requested a review from ghalliday September 25, 2024 16:12

asselitx marked this pull request as ready for review September 25, 2024 16:13

ghalliday reviewed Sep 26, 2024

View reviewed changes

HPCC-30368 Fixes after code review and final testing

c400b9b

* Rename variables to be more descriptive * When parsing the packagemap file, correctly propagate the resolution method (daliip vs remoteStorage) to the next level of parsing. Signed-off-by: Terrence Asselin <[email protected]>

asselitx requested review from ghalliday and afishbeckln October 10, 2024 00:02

ghalliday approved these changes Oct 10, 2024

View reviewed changes

afishbeckln approved these changes Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30368 Add packagemap support for remoteStorage property #19148

HPCC-30368 Add packagemap support for remoteStorage property #19148

asselitx commented Sep 24, 2024 •

edited

Loading

asselitx commented Sep 24, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024

asselitx commented Sep 24, 2024

afishbeckln Sep 24, 2024

asselitx Sep 25, 2024

asselitx Oct 8, 2024

afishbeckln Sep 24, 2024

asselitx Sep 25, 2024

asselitx Oct 8, 2024

afishbeckln commented Sep 24, 2024

asselitx commented Sep 25, 2024

ghalliday left a comment

ghalliday Sep 26, 2024

ghalliday Sep 26, 2024

ghalliday left a comment

afishbeckln left a comment

HPCC-30368 Add packagemap support for remoteStorage property #19148

Are you sure you want to change the base?

HPCC-30368 Add packagemap support for remoteStorage property #19148

Conversation

asselitx commented Sep 24, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

asselitx commented Sep 24, 2024 • edited Loading

github-actions bot commented Sep 24, 2024

asselitx commented Sep 24, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeckln commented Sep 24, 2024

asselitx commented Sep 25, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

afishbeckln left a comment

Choose a reason for hiding this comment

asselitx commented Sep 24, 2024 •

edited

Loading

asselitx commented Sep 24, 2024 •

edited

Loading