HPCC-32724 Allow storage planes to be defined in bare-metal systems #19234

jackdelv · 2024-10-24T20:27:33Z

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 · 2024-10-24T20:27:55Z

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

Jirabot Action Result:
Assigning user: jack.delvecchio@lexisnexisrisk.com
Workflow Transition To: Merge Pending
Updated PR

ghalliday

Changes look reasonable. Need to separate out the different types of changes though.

ghalliday · 2024-10-25T11:23:29Z

dali/base/dafdesc.cpp

-        //printYAML(storage);
+    // Add groups defined in environment.xml
+    Owned<IPropertyTree> env = getHPCCEnvironment();
+    Owned<IPropertyTreeIterator> storagePlanes = env->getElements("Software/Globals/StoragePlaneList/StoragePlane[@name='*']");


minor:I would use "Storage" (or "StoragePlanes" rather than "StoragePlaneList"

Also, no need to have a qualifier on the name:

Owned<IPropertyTreeIterator> storagePlanes = env->getElements("Software/Globals/StoragePlanes/StoragePlane");

I am not sure about the benefits of accessing via the environment, or by copying the data to the component configuration file and accessing from there.
@jakesmith opinions?

The standard approach is to generate into the component config file (in this case daliconf.xml).
We do this already for other components (dfuserver, esp.. ), see e.g. initfiles/componentfiles/configxml/dfuserver.xsl. At the moment it does this for RemoteStorage only in those places.

should be generated in the same way into those configs too, and Dali (which doesn't need RemoteStorage) should generate only.

One caveat to that is that the above code only creates planes for existing groups in Dali (that correspond to engine groups in BM) if "planes" doesn't exist, but it will do after generating it into the config..

I suggest changing the createStoragePlane calls, so that in this context it adds a flag to the planes being added, to indicate they are based on groups (e.g. @fromGroup=true), and then change the start of this process to:

if (createPlanesFromGroups) { // delete all [@fromGroup=true] planes // existing code that constructs groups and adds planes for groups ... }

NB: this routine can be recalled if/when environment.xml changes.

Copied storage/planes definitions into component config files which allowed a group to be created for my storage plane.

I changed createStoragePlane to add the flag fromGroup to the planes created from it. Then at the beginning of the process changed it to remove all storage planes previously created by createStoragePlane by removing any with fromGroup=true. I also had to add a check at the beginning of createStoragePlanes for a storage plane that is already defined (this would be coming from the component config file) otherwise it would create a duplicate planes.

ghalliday · 2024-10-25T11:24:59Z

testing/regress/environment.xml.in

@@ -20,6 +20,20 @@
        storageWrites="0.0038"/>
 </Hardware>
 <Software>
+  <Globals noCommon="true"
+           maxConnections="-1">
+           <StoragePlane name="storagePlane"


Doesn't contain StoragePlaneList

Also, I suspect these should not be checked in as part of the PR.

Removed initfiles changes from PR.

ghalliday · 2024-10-25T11:25:16Z

common/remote/hooks/azure/azurefile.cpp

@@ -459,12 +459,15 @@ AzureFile::AzureFile(const char *_azureFileName) : fullName(_azureFileName)

        StringBuffer planeName(slash-filename, filename);
        Owned<IPropertyTree> plane = getStoragePlane(planeName);
+        if (!plane)


Should be in a separate PR.

Moved to HPCC-32723 on my local repo.

ghalliday · 2024-10-25T11:32:33Z

common/remote/hooks/azure/azurefile.cpp

        const char * api = plane->queryProp("storageapi/@type");
        if (!api)
            throw makeStringExceptionV(99, "No storage api defined for plane %s", planeName.str());

        constexpr size_t lenPrefix = strlen(azureBlobPrefix);
-        if ((strncmp(api, azureBlobPrefix, lenPrefix-1) != 0) || api[lenPrefix-1] != ':')
+        if ((strncmp(api, azureBlobPrefix, lenPrefix-1) != 0))


I think the extra test should have been

api[lenPrefix-1] == 0

so that it doesn't match azureblobv2

possibly better would be to define a string without the trailing colon and use strieq.

Changed to use strieq.

jackdelv · 2024-10-25T14:12:50Z

@ghalliday I have removed changes unrelated to adding support for storage planes in bare-metal. The working code and the commits based on your comments can be found on branch HPCC-32723 in my forked repo.

ghalliday · 2024-11-04T09:37:26Z

@jackdelv please convert to normal PR
@jakesmith can you foresee any problems with this?

jakesmith

@jackdelv - please see comments

jakesmith

@jackdelv - looks good. Please address 1 trivial comment and squash.

jakesmith · 2024-11-13T18:33:50Z

dali/base/dafdesc.cpp

    {
+        // Remove old planes created from groups
+        while (storage->removeProp("planes[@fromGroup=\"1\"]"));


trivial: for consistency match with true vs 1, and can use single quotes for readability:
while (storage->removeProp("planes[@fromGroup='true']"));

jackdelv · 2024-11-14T17:20:20Z

@jakesmith Squashed. I'm looking into why some of the smoketests are failing.

- Copy any storage plane definitions from environment.xml Globals into component config files - Change createStoragePlane to exit if the storage plane already exists (this is after any previous storage planes created with createStoragePlane are removed) - Add fromGroup=true flag to storage planes created from groups Signed-off-by: Jack Del Vecchio <jack.delvecchio@lexisnexisrisk.com>

ghalliday · 2024-11-21T12:33:03Z

@jackdelv tests are still failing. Did Jakes suggestion of changing from ='1' to ='true' break it?

ghalliday

tests currently failing

jackdelv · 2024-11-21T13:41:51Z

@ghalliday It seems that may have been the issue. I will wait for the tests to run to be sure since I'm still confused why it was happening. When I run this code for connecting to my azure blob storage it seemed to be working on roxie. It was hitting an unimplemented error rather than the missing host group exception regardless of if '1' or 'true' is passed in.

UNIMPLEMENTED feature [AzureFile::copyTo] in function copyTo() at azurefile.cpp(246)

in := DATASET('~plane::jackdelvstorageplanetest::taxi-blob::taxi_tripdata.csv', layout, CSV(HEADING(1)));
out := CHOOSEN(in,1000);
OUTPUT(out);
OUTPUT(out,,'~plane::jackdelvstorageplanetest::taxi-blob::taxi_tripdata_copy.csv',CSV, PLANE('jackdelvstorageplanetest'), OVERWRITE);
in2 := DATASET('~plane::jackdelvstorageplanetest::taxi-blob::taxi_tripdata_copy.csv', layout, CSV);
OUTPUT(in2,,'~mytaxi::taxi_tripdata_copy.csv',CSV, PLANE('jackdelvstorageplanetest'), OVERWRITE);

I tested with testing/regress/payload.ecl and was getting the same host group error with 'true'. Changing to '1' seemed to fix the error.

EXCEPTION: No entry found for hostGroup: 'mythor' (in Index Read 2)

Although, when the test suite reported the error, it was looking for myroxie, but I could only ever reproduce with the mythor host group.

jackdelv requested a review from ghalliday October 24, 2024 20:27

ghalliday reviewed Oct 25, 2024

View reviewed changes

jackdelv force-pushed the issue32724 branch from f53dc71 to 9ab6ad9 Compare October 25, 2024 14:09

jackdelv requested a review from ghalliday October 25, 2024 14:14

ghalliday approved these changes Nov 4, 2024

View reviewed changes

ghalliday requested a review from jakesmith November 4, 2024 09:37

jackdelv marked this pull request as ready for review November 4, 2024 12:08

jakesmith reviewed Nov 5, 2024

View reviewed changes

jackdelv requested a review from jakesmith November 12, 2024 21:14

jakesmith approved these changes Nov 13, 2024

View reviewed changes

jackdelv force-pushed the issue32724 branch from 264a352 to ed5341a Compare November 14, 2024 15:45

jackdelv force-pushed the issue32724 branch from ed5341a to 30fc1fe Compare November 15, 2024 13:19

ghalliday requested changes Nov 21, 2024

View reviewed changes

Change from 'true' to '1'

f4f5426

jackdelv requested a review from ghalliday November 21, 2024 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32724 Allow storage planes to be defined in bare-metal systems #19234

HPCC-32724 Allow storage planes to be defined in bare-metal systems #19234

jackdelv commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024

ghalliday left a comment

ghalliday Oct 25, 2024

jakesmith Nov 5, 2024

jackdelv Nov 12, 2024

ghalliday Oct 25, 2024

jackdelv Oct 25, 2024

ghalliday Oct 25, 2024

jackdelv Oct 25, 2024

ghalliday Oct 25, 2024

jackdelv Oct 25, 2024

jackdelv commented Oct 25, 2024

ghalliday commented Nov 4, 2024

jakesmith left a comment

jakesmith left a comment

jakesmith Nov 13, 2024

jackdelv commented Nov 14, 2024

ghalliday commented Nov 21, 2024

ghalliday left a comment

jackdelv commented Nov 21, 2024 •

edited

Loading

HPCC-32724 Allow storage planes to be defined in bare-metal systems #19234

Are you sure you want to change the base?

HPCC-32724 Allow storage planes to be defined in bare-metal systems #19234

Conversation

jackdelv commented Oct 24, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Oct 24, 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

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

Choose a reason for hiding this comment

jackdelv commented Oct 25, 2024

ghalliday commented Nov 4, 2024

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

jackdelv commented Nov 14, 2024

ghalliday commented Nov 21, 2024

ghalliday left a comment

Choose a reason for hiding this comment

jackdelv commented Nov 21, 2024 • edited Loading

jackdelv commented Oct 24, 2024 •

edited

Loading

jackdelv commented Nov 21, 2024 •

edited

Loading