HPCC-30365 Add XRef Sasha service to K8s by jackdelv · Pull Request #19639 · hpcc-systems/HPCC-Platform

jackdelv · 2025-03-19T13:20:30Z

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 · 2025-03-19T13:20:48Z

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

Jirabot Action Result:
Workflow Transition To: Merge Pending
Additional PR: #19639

jakesmith

@jackdelv - I think changes look logical, but there are various problems with the changes to expandMask.
Could do with some unittests added to prove for various input it is creating the expected output.

jakesmith · 2025-03-20T09:23:29Z

+                }
+                if (isDirPerPart) {
+                    // MORE: Should maybe check this doesn't contain any subdirectories to make
+                    // sure it is really a dirPerPart directory. Is an all numbers subdirectory valid in ecl?


in practice yes, a scope must have a leading alpha char. So worth clarifying the comment.

Clarified comment.

jakesmith · 2025-03-20T09:28:19Z

+                        isDirPerPart = false;
+                }
+                if (isDirPerPart) {
+                    // MORE: Should maybe check this doesn't contain any subdirectories to make


can check pdir.dirs after the scanDirectory, should be empty.
Let's add a check and throw an exception if not.

Added check (pdir->dirs.ordinality()>0)

jakesmith · 2025-03-20T09:42:51Z

-            unsigned maxMb = serverConfig->getPropInt("DfuXRef/@memoryLimit", DEFAULT_MAXMEMORY);
+            unsigned maxMb;
+            if (isContainerized()) {
+                const char *resourcedMemory = getComponentConfigSP()->queryProp("resources/@memory");


now 'props' is saved, should used it instead of getComponentConfigSP() here.

Fixed to use new props member.

jakesmith · 2025-03-20T09:45:07Z

            char next = *(s++);
            if (next=='$') {
+                if (dirPerPart)
+                {


trivial/formatting: Allman vs K&R

Removed change.

jakesmith · 2025-03-20T09:57:01Z

+                {
+                    const char * start = buf.str();
+                    const char * slash = start + buf.length();
+                    while (slash > start && *slash != PATHSEPCHAR)


'slash' at the start is potentially beyond the end of the allocated buffer I think.

Reverted changed.

jakesmith · 2025-03-20T10:00:12Z

+                    while (slash > start && *slash != PATHSEPCHAR)
+                        slash--;
+                    buf.insert(slash-start, PATHSEPCHAR);
+                    buf.insert((slash+1)-start, p+1);


not safe, buf may have reallocated after 1st insert, meaning 'slash' points to a free'd pointer.

These insert also don't look right in that they're inserting "/", but the mask is typically something like "myfilename._$P$_of_10", so it's going to end up with e.g. "/5myfilename._5_of_10" afaics.

If it did find a slash, it would prob be okay, but there isn't necessarily a full path so there's no guarantee there's a slash.

Also, the mask can be "myfilename._$P$_of_$N$" (see 'N' handleing below)
This insert code is going to be hit twice in that case, and therefore insert the dir-per-part directory twice...

Reverted changes to expandMask in favor of storing prefix and scope directory paths in cFileDesc and building up full paths in getPartName instead.

jakesmith · 2025-03-20T10:28:18Z

+StringBuffer &expandMask(StringBuffer &buf, const char *mask, unsigned p, unsigned n, unsigned stripeNum, bool dirPerPart)
 {
+    if (stripeNum>0)
+        addPathSepChar(buf.append('d').append(stripeNum));


hm, how is this strip dir going to be in right place?

Removed changes to expandMask.

jakesmith

As discussed in our meeting, there's are some issues with the way the info. is being scanned and stored in the hierarchical structure built up during the physical file scan and used subsequently.
Currently too much information is being stored per cFileDesc - as before, the file path shouldn't be needed, only the deduced filename mask.
Lost files are probably being misreported (at least during initial phase), because the paths they walk do not match the physical representation on disk (due to striping and dir-per-part directories).

saxref should:

ensure runXRef is dealing with 1 plane a time (i.e. if multiple selected, process 1 at a time).
store plane details (IPT) so accessible to other xref tasks during scan.
during scanDirectories, if striped, detected when at stripe level, and 'skip'
detect dir-per-part directory and 'skip'. But, check after recursing the dir, that contains no subdirs.
Keep the filename mask only in cFileDesc
Add scope/lfn to listOrphans. Build up in same way as 'basedir' is now.
Think we can get rid of baseDir altogether.
during scanLogicalFiles, parse file paths fetched from parts (add helper func), to remove stripe and dir-part-part, so can marry the pathing with the cDirDesc tree.
NB: LATER- may be better to assume that the cDirDesc is a representation of scopes, and walk scopes of logical file (+part endpoints), instead of getting part directories.
In listOrphans(cFileDesc), deduce file path from lfn, partNum and plane. Add a utility func that deduces and uses stripe num and dir-per-part if relevant (based on plane details).

@jackdelv

jakesmith · 2025-03-21T10:53:47Z

-    if (mname.isEmpty())
+    // Assume that if prefix is passed in a match is required
+    if (prefix && prefix->isEmpty())
        throw makeStringExceptionV(-1, "Could not find matching prefix in plane definition for file %s", filename);


I think throwing an exception here, will mean 1 path that fails to match, will cause the whole xref process to fail?

True of any exception in parseFilename. NB: addFile only issues warnings.

addFile should likely have a try/catch and issue warnings in case of any exception in parseFilename.

There's something that doesn't make sense here in fact I think..

How would the files not match prefix?
And, why is it scanning planes every file being added, to determine which plane the prefix path is in?
Given, we are specifically xref'ing a given plane, and therefore start at the prefix path..
This relates to HPCC-33151.

Let's discuss.

Instead of matching the prefix for each file, the XRefManager stores the current storage plane.

jakesmith · 2025-03-21T11:10:53Z

-                mname.append((d+1)-name, name).append(cur-(tailSlash+1), tailSlash+1);
+                if (dirs)
+                    dirs->append((d+1)-name, name);
+                mname.append(cur-(tailSlash+1), tailSlash+1);


common with line 3881, could go outside if/else

Removed use of parseFileName from addfile. Now the striped and dir-per-part status are deduced from the directory structure while building up cDirDescs in scanDirectory and passed to recursive calls.

jakesmith · 2025-03-21T11:40:09Z


    StringBuffer &getPartName(StringBuffer &buf,unsigned p)
    {
+        // In baremetal, buf can be prepoulated with replicate directory


The old code was:

StringBuffer &getPartName(StringBuffer &buf,unsigned p) { StringBuffer mask; getName(mask); return expandMask(buf, mask, p, N); }

If buf is prepopulated, didn't that mean that as it was, it added the whole path again?

This feels like it's masking a problem, rather than the correct fix.

Something not quite right here..
the cFileDesc (and cDirDesc) contain the name only, not a path.

getPartName should be returning an expanded form of the name mask only, as it was before.

Changed cFileDesc to contain just the file name.

jackdelv · 2025-04-02T14:39:21Z

@jakesmith I made the changes to remove adding path and scope to cFileDesc and instead pass the plane information and striped and dir-per-part statuses down to cFIleDesc::create. This avoids having to match the prefix and lookup storage planes for every file.

It seems in scanLogicalFiles there is more wrong than just not being able to include stripe and dir-per-part numbers. It is trying to match the current directory with the root. In bare-metal the root prefix looks like /var/lib/HPCCSystems/hpcc-data/thor based on which cluster is currently running xref. This will always fail to find files in directories for other components e.g. it will always say files under /var/lib/HPCCSystems/hpcc-data/roxie are potential lost files if doing the search on thor. In containerized this doesn't happen since the prefix won't contain the component name at the tail and will instead be /var/lib/HPCCSystems/hpcc-data. In this case the missing stripeNum and dir-per-part nums do cause an issue.

Other than that, my tests for found/orphan files are working and everything is showing up correctly in results on bm and containerized versions.

AttilaVamos · 2025-04-02T19:26:43Z

@jackdelv There are some Unit test errors. See: https://github.com/hpcc-systems/HPCC-Platform/actions/runs/14222208906/job/39855851609?pr=19639
(I need to fix that test to report this kind of errors. I opened a JIRA to do that.)

jakesmith · 2025-04-03T11:50:57Z

@jackdelv There are some Unit test errors. See: https://github.com/hpcc-systems/HPCC-Platform/actions/runs/14222208906/job/39855851609?pr=19639 (I need to fix that test to report this kind of errors. I opened a JIRA to do that.)

@streeterd - hit similar problems when testing https://hpccsystems.atlassian.net/browse/HPCC-27051.
He has some pending changes to make to the k8s github action, which provide more insight when a deployment fails like this. See https://hpccsystems.atlassian.net/issues/HPCC-33739

jackdelv · 2025-04-03T13:26:34Z

@AttilaVamos @jakesmith I fixed the issue with the DFUXrefLibTests. I had made some changes to parseFileName and didn't notice the tests were failing. If the unittests are reporting errors, why is the github action being marked as "Succeeded" (I just saw your comment about opening a jira. Disregard) ? Also, it seems like the DFUAccessTests failures are occurring in other PR's. Should I ignore them?

jackdelv · 2025-04-03T13:30:05Z

@jakesmith I noticed another error is occurring that I have never seen before.

Waiting for deployment "sasha-xref" rollout to finish: 0 of 1 updated replicas are available...
error: timed out waiting for the condition

In the test: https://github.com/hpcc-systems/HPCC-Platform/actions/runs/14222208906/job/39856041969

This probably has something to do with how I instantiated the sasha xref service, but I'm not sure what I did wrong.

jakesmith

@jackdelv - please see feedback.

jakesmith · 2025-04-03T14:05:50Z

-                    f->getPartName(path,pn);
                    RemoteFilename rfn;
-                    rfn.setPath(grp->queryNode((pn+drv)%numnodes).endpoint(),path.str());
+                    rfn.setPath(grp->queryNode((pn+drv)%numnodes).endpoint(),getPhysicalPartName(path,storagePlane,lfn.get(), pn, f->N, f->isDirPerPart, f->isStriped).str());


quite a long hard to read line, would benefit from the getPhysicalPartName call begin on its own line.

But, can you check if constructPartFilename (dadfs.hpp) doesn't do what's required already?
I think it also covers the replicate case (in BM case only).

Moved getPhysicalPartName to its own line.

It looks like constructPartFilename does build up the correct physical part name, and all the information it needs is available in listOrphans. However, right before constructPartFilename returns the finished string, it lowercases the return buffer which causes checkOrphanPhysicalFile to not find the file.

I'm not sure why it does this, but I didn't want to change since it is probably relied on for other uses.

I'm not sure why it does this, but I didn't want to change since it is probably relied on for other uses.

I think it's because all logical files are caseless by definition.. e.g. a::b::c is same as A::B::C - physical files should be lower cased in DFS.

which causes checkOrphanPhysicalFile to not find the file.

it failed because there was a physical file with mixed case? (there shouldn't be)

Changed to use constructPartFilename and removed lower casing from the function.

jakesmith · 2025-04-03T14:26:28Z

+                    if (!scanDirectory(node,ep,path,drv,pdir,NULL,!isStriped,isStriped))
                        return false;
+                    if (!isStriped && pdir->dirs.ordinality()>0)
+                        throw makeStringExceptionV(-1, LOGPFX "Directory Per Part %s contains other subdirectories.", path.str());


need to be careful that 1 malformed file, doesn't cause all efforts to xref to be unrunnable.
Should report and skip, but not abort completely.

Changed to log an error rather than throw an exception.

See other comment, we need to revisit these rejections, they should appear in the orphaned report I think (needs JIRA to be done after this one).

Currently, it is calling OERRLOG to report that the dir per part contains sub dirs. Is there a better way to log what is happening?

jakesmith · 2025-04-03T14:27:44Z

-                    // sure it is really a dirPerPart directory. Is an all numbers subdirectory valid in ecl?
-                    if (!scanDirectory(node,ep,path,drv,pdir,NULL))
+                if (isSpecialDir) {
+                    // Files inside a special directory should be scanned with the current directory as the parent


can you expand on this. Explaining why structure is being built up without stripe and dir-per-part cDirDesc's

Added more comments.

See my comment.

jakesmith · 2025-04-03T15:05:12Z

-                    if (!scanDirectory(node,ep,path,drv,pdir,NULL))
+                if (isSpecialDir) {
+                    // Files inside a special directory should be scanned with the current directory as the parent
+                    if (!scanDirectory(node,ep,path,drv,pdir,NULL,!isStriped,isStriped))


I think isStriped shouldn't be a recursively passed property.
We know whether files should be in stripe directories or not from the outset (from the plane details).
scanDirectories should check that the 1st level is a stripe directory format, skip it like this if it is, report an error if not and ignore directory.

I'm not sure how to tell if there should be a stripe directory from the plane details.

I was trying to use numDevices, but in bare-metal I have a test file that does not have a stripeDir despite numDevices>1 being in the plane info.

Also, if there is something in the plane details saying there should be stripeDir, does this apply to all files in the plane i.e. could there be a plane where some files are striped on it and some aren't?

I was trying to use numDevices, but in bare-metal I have a test file that does not have a stripeDir despite numDevices>1 being in the plane info.

If a plane has numDevices and it is not a 'hostGroup' (!plane->hasProp("@HoStGroup")) then it is striped.
@HoStGroup planes have a list of planes, although supported in containerized, they are generally going to be BM planes mapped over a set of hosts.

Also, if there is something in the plane details saying there should be stripeDir, does this apply to all files in the plane i.e. could there be a plane where some files are striped on it and some aren't?

No, if a plane implies striped, all files must be. It's because the plane defined the storage, the lfn's only know they layout based on the plane.
You could imagine a different scenario where the lfn's stored more detail - that might imply that the underlying original plane could change, and the existing lfn's remained associated with the old info, but that is not the design. Plane essentially need to be thought of as immutable, if different storage configuration were needed, then a new plane would need defining, and e.g. set as default, and pre-existing files would either remain on the old plane, or would need migrating.

So:

isStriped should be deduced at the start of the scan (NB: dirPerPart could vary per file).

Should be remove as a member of cFileDesc

scanDirectory should assert that the top-level directory looks like a strip-directory, and reject the files if not.

NB: TBD (open a new sub JIRA place) - rejecting files both for this and other places, e.g. when "Directory Per Part %s contains other subdirectories.", should probably not short-circuit and ignore them, but add them, such that they are picked up as orphans, or misplaced.

Removed isStriped from cFileDesc and added a check to scanDirectories to log an error when expected stripe directories are not found in the toplevel.

jakesmith · 2025-04-03T15:06:18Z

+            StringBuffer path;
            if (drv)
-                setReplicateFilename(path,drv);
+                setReplicateFilename(addPathSepChar(path.append(basedir)),drv);


may be able to use constructPartFilename vs getPhysicalPartName and not special case here.

If constructPartFilename didn't return a lowercased string, then it could be used here to avoid the special case.

Why did lower casing causes issues? (see other comment).

Removed lower casing in constructPartFilename because it was lower casing the prefix along with the filename.

jakesmith

@jackdelv - changes look sensible. Please see follow up comments.

jakesmith · 2025-04-09T11:03:32Z

-                    if (!scanDirectory(node,ep,path,drv,pdir,NULL))
+                if (isSpecialDir) {
+                    // Files inside a special directory should be scanned with the current directory as the parent
+                    if (!scanDirectory(node,ep,path,drv,pdir,NULL,!isStriped,isStriped))


I was trying to use numDevices, but in bare-metal I have a test file that does not have a stripeDir despite numDevices>1 being in the plane info.

If a plane has numDevices and it is not a 'hostGroup' (!plane->hasProp("@HoStGroup")) then it is striped.
@HoStGroup planes have a list of planes, although supported in containerized, they are generally going to be BM planes mapped over a set of hosts.

Also, if there is something in the plane details saying there should be stripeDir, does this apply to all files in the plane i.e. could there be a plane where some files are striped on it and some aren't?

No, if a plane implies striped, all files must be. It's because the plane defined the storage, the lfn's only know they layout based on the plane.
You could imagine a different scenario where the lfn's stored more detail - that might imply that the underlying original plane could change, and the existing lfn's remained associated with the old info, but that is not the design. Plane essentially need to be thought of as immutable, if different storage configuration were needed, then a new plane would need defining, and e.g. set as default, and pre-existing files would either remain on the old plane, or would need migrating.

jakesmith · 2025-04-09T11:09:31Z

-                    if (!scanDirectory(node,ep,path,drv,pdir,NULL))
+                if (isSpecialDir) {
+                    // Files inside a special directory should be scanned with the current directory as the parent
+                    if (!scanDirectory(node,ep,path,drv,pdir,NULL,!isStriped,isStriped))


So:

isStriped should be deduced at the start of the scan (NB: dirPerPart could vary per file).

Should be remove as a member of cFileDesc

scanDirectory should assert that the top-level directory looks like a strip-directory, and reject the files if not.

NB: TBD (open a new sub JIRA place) - rejecting files both for this and other places, e.g. when "Directory Per Part %s contains other subdirectories.", should probably not short-circuit and ignore them, but add them, such that they are picked up as orphans, or misplaced.

jakesmith · 2025-04-09T11:24:53Z

-                    f->getPartName(path,pn);
                    RemoteFilename rfn;
-                    rfn.setPath(grp->queryNode((pn+drv)%numnodes).endpoint(),path.str());
+                    rfn.setPath(grp->queryNode((pn+drv)%numnodes).endpoint(),getPhysicalPartName(path,storagePlane,lfn.get(), pn, f->N, f->isDirPerPart, f->isStriped).str());


I'm not sure why it does this, but I didn't want to change since it is probably relied on for other uses.

I think it's because all logical files are caseless by definition.. e.g. a::b::c is same as A::B::C - physical files should be lower cased in DFS.

which causes checkOrphanPhysicalFile to not find the file.

it failed because there was a physical file with mixed case? (there shouldn't be)

jakesmith · 2025-04-09T11:25:53Z

+                    if (!scanDirectory(node,ep,path,drv,pdir,NULL,!isStriped,isStriped))
                        return false;
+                    if (!isStriped && pdir->dirs.ordinality()>0)
+                        throw makeStringExceptionV(-1, LOGPFX "Directory Per Part %s contains other subdirectories.", path.str());


See other comment, we need to revisit these rejections, they should appear in the orphaned report I think (needs JIRA to be done after this one).

jakesmith · 2025-04-09T11:26:19Z

-                    // sure it is really a dirPerPart directory. Is an all numbers subdirectory valid in ecl?
-                    if (!scanDirectory(node,ep,path,drv,pdir,NULL))
+                if (isSpecialDir) {
+                    // Files inside a special directory should be scanned with the current directory as the parent


See my comment.

jakesmith · 2025-04-09T11:27:18Z

+            StringBuffer path;
            if (drv)
-                setReplicateFilename(path,drv);
+                setReplicateFilename(addPathSepChar(path.append(basedir)),drv);


Why did lower casing causes issues? (see other comment).

jackdelv · 2025-04-09T20:04:31Z

@jakesmith Removing the call to StringBuffer::lowercase in constructPartFilename provided the expected physical filename, so I have removed getPhysicalPartName. I removed isStriped from the parameters of scanDirectory and the cFileDesc member since it can be deduced from the plane info.

I've taken note of the two JIRAs you have mentioned. Am I missing anything?

Revisit errors/exceptions being thrown to ensure XRef keeps running even if a file causes an error
Reject Files not under stripe directory if plane details show directories are striped and subdirectories under dir-per-part directories.

jakesmith

@jackdelv - please see comments.

jakesmith · 2025-04-10T10:12:35Z

                    if (!isdigit(*(dir++)))
                        isSpecialDir = false;
                }
+                // If numDevices>0, check that top-level directories are striped


should be >1 (see earlier comments)

jakesmith

@jackdelv - 1 issue re. "d" still I think + 2 trivial comments.

jakesmith · 2025-04-10T15:12:56Z

        if (plane)
+        {
            storagePlane.set(plane);
+            isPlaneStriped = !storagePlane->hasProp("@hostGroup") && (storagePlane->getPropInt("@numDevices", 1)>1);


trivial: no point in the default value in this context.

jakesmith · 2025-04-10T15:14:10Z

    unsigned numuniqnodes = 0;
    Owned<IUserDescriptor> udesc;
    Linked<IPropertyTree> storagePlane;
+    bool isPlaneStriped;


trivial: even though itialized in ctor, it's good practice for all members (for new code at least) to have member initializers. Helps if code is ever refactored etc., and compiler is usually smart enough not to initialize twice anyway.

jakesmith · 2025-04-10T15:26:09Z

    }

-    StringBuffer &getName(StringBuffer &buf)
+    bool getName(StringBuffer &buf)


trivial/not new: should be a const method really (as should get some of the members below)

jakesmith · 2025-04-10T15:29:01Z

I've taken note of the two JIRAs you have mentioned. Am I missing anything?

Revisit errors/exceptions being thrown to ensure XRef keeps running even if a file causes an error
Reject Files not under stripe directory if plane details show directories are striped and subdirectories under dir-per-part directories.

Not that I can recall. Please open sub-jira's under parent, if haven't already.

jackdelv · 2025-04-10T16:16:29Z

@jakesmith Fixed the trivial items. I added a check to the initializer for isDirStriped that dir[1] != '\0'. It seemed to be the best way to fix it, but is there a better way? Can the whole stripe/dirperpart check be refactored in a cleaner way?

jackdelv · 2025-04-10T16:44:26Z

@jakesmith Opened two JIRAs relating to our discussion:

https://hpccsystems.atlassian.net/browse/HPCC-33889
https://hpccsystems.atlassian.net/browse/HPCC-33890

jakesmith

@jakesmith Fixed the trivial items. I added a check to the initializer for isDirStriped that dir[1] != '\0'. It seemed to be the best way to fix it, but is there a better way? Can the whole stripe/dirperpart check be refactored in a cleaner way?

I think it works as is.
I'd probably have introduced a isRestDigits inline helper and code like:

                bool isDirStriped = false; // format 'd[0-9]+'
                bool isSpecialDir = false; // isDirStriped || dirPerPart format '[0-9]+'
                if (*dir == 'd')
                {
                    dir++;
                    isDirStriped = (*dir != '\0') && isRestDigits(dir);
                    isSpecialDir = isDirStriped;
                }
                else
                    isSpecialDir = isRestDigits(dir);

untested!

@jackdelv - looks good I think. Please squash, and I'll hopefully do a final review in the morning.

jakesmith

@jackdelv - a few final comments.

jakesmith · 2025-04-11T10:36:43Z

+    makePhysicalPartName(lname, partNo+1, max, fullname, 0, DFD_OSdefault, prefix, dirPerPart, stripeNum);
+
+    // revisit: constructPartFilename should be refactored not to deal with replicate directories, by pre-determining the alternate prefix if copy>0
+    // If copy>0 it could do calPartLocation, find the replicate plane, get it's prefix, and pass to makePhysicalPartName


trivial: typo calcPartLocation

jakesmith · 2025-04-11T11:40:08Z

    IPropertyTree * plane = storage->addPropTree("planes");
+
+    // Revisit: Ignore hthor planes when running XRef on storage planes
+    if (groupType == grp_hthor)


I'm 99% sure these group types only exist in BM/not created in containerized.
Comment should be updated.

jakesmith · 2025-04-11T12:28:55Z

 #endif
+
+// takes filename and creates mask filename with $P$ extension
+extern da_decl void parseFileName(const char *name,StringBuffer &mname,unsigned &num,unsigned &max,unsigned &stripeNum,unsigned &dirPerPart,bool &replicate)


it would be good to move this back into dfuxreflib.cpp where it was, as not shared anymore.

jakesmith · 2025-04-11T12:40:36Z

    for (unsigned m=0; m<n; m++) {
        RemoteFilename rfn;
-        constructPartFilename(group,m+1,n,NULL,partmask.str(),dir.str(),false,1,rfn);
+        constructPartFilename(group,m+1,1,n,0,0,false,"",dir.str(),partmask.str(),0,rfn);


shouldn't the 3d parameter be 0 ? (copy=0)

jakesmith · 2025-04-11T12:44:47Z

        ndone[0] = 0;
        ndone[1] = 0;
-        for (drv=0;drv<2;drv++) {
+        unsigned numDevices = storagePlane->hasProp("@HostGroup") ? 1 : storagePlane->getPropInt("@numDevices", 1);


should be @hostGroup

.. but would make sense to add numDevices as a member and set at same time as isPlaneStriped

jakesmith · 2025-04-11T13:03:14Z

        ClusterPartDiskMapSpec mspec;
        Owned<IGroup> grp = createIGroup("10.150.10.1-3");
        RemoteFilename rfn;
+        IStoragePlane *plane = getDataStoragePlane("mystorageplane", true);


leaks, but also not used - this line should be deleted.

jakesmith

@jackdelv - looks good. Please squash, and I'll give it one more pass hopefully.

jackdelv · 2025-04-15T13:10:34Z

@jakesmith Squashed.

jakesmith

@jackdelv - looks good.

As discussed, the dfuxreflib should likely be deprecated altogether. It is not the default or ever used in BM afaik (saxref is the standard setuip).
It would be good to assert that it is never used in k8s, and remove changes to it in this PR.

jakesmith · 2025-04-16T12:23:17Z

+        if (!plane)
+            throw makeStringExceptionV(0, "Plane definition \"%s\" is missing for File", grpname.str());
+        unsigned numDevices = plane->numDevices();
+        bool r = replicate?1:0;


should be unsigned r =

jackdelv · 2025-04-16T14:54:33Z

@jakesmith Removed the dfuXRefLib changes. Since the constructPartFilename function was updated and is used in saxref, I changed dfuXRefLib to use the now deprecated version. I also added an assert that containerized should be using the new saxref path.

jakesmith · 2025-04-16T15:13:14Z

+                {
+                    // MORE: Containerized should only be using saxref and dfuXRefLib should probably be removed
+                    assert(xRefNode->useSasha());
+                    xRefNode->setStatus("Submitted");


I think better to remove this line and the else. i.e. just the assert (should use assertex),

Good to catch/assert here, but what I meant/was referring to, was the code in saxref.cpp:

bool eclwatchProviderDefault = false; // NB: Default to start Xref using sasha service in containerized without having to change values.yaml // eclwatchProvider sets useSasha in call to setSubmittedOk if (isContainerized()) eclwatchProviderDefault = true; bool eclwatchprovider = props->getPropBool("@eclwatchProvider",eclwatchProviderDefault);

That should explicitly only support the eclwatch provide option in containerized, e.g.:

bool eclwatchprovider = true; if (!isContainerized()) // NB: containerized does not support xref any other way. { // eclwatchProvider sets useSasha in call to setSubmittedOk eclwatchprovider = props->getPropBool("@eclwatchProvider"); }

jakesmith

@jackdelv - a couple of comments.

I notice some whitespaces creeping in (at end of lines).
(more obvious when using git diff on the CLI)
I haven't used it personally, but there is a vscode option:

"files.trimTailingWhitespace" : true

which may be useful

jackdelv · 2025-04-16T16:00:10Z

@jakesmith Fixed the assert. I also turned on that setting to hopefully prevent that from happening in the future. Which files had trailing whitespaces? I know the dfuXRefLib diff shows whitespace changes still, but those are removals.

jakesmith · 2025-04-16T17:16:06Z

@jakesmith Fixed the assert. I also turned on that setting to hopefully prevent that from happening in the future. Which files had trailing whitespaces? I know the dfuXRefLib diff shows whitespace changes still, but those are removals.

hm, I don't see them now. Maybe I was looking at wrong diff.

jakesmith

@jackdelv - looks good. Please squash.

jakesmith · 2025-04-16T17:17:26Z

@jackdelv - now that candidate-9.12.x exists, this needs to be rebased onto it.

jackdelv · 2025-04-16T17:22:26Z

@jakesmith Squashed, and Rebased on 9.12.x. Back to you.

jakesmith

@jackdelv - looks good.

@ghalliday - please take a look.

ghalliday

@jackdelv looks good. One question, otherwise I am happy to merge. (One additional minor comment.)

ghalliday · 2025-04-17T14:18:41Z

-    static cFileDesc * create(CLargeMemoryAllocator &mem,const char *_name,unsigned n)
+    static cFileDesc * create(CLargeMemoryAllocator &mem,const char *_name,unsigned n,bool d,unsigned fnLen)
    {
        size32_t sl = strlen(_name); 


future: add an assert that fnLen <= 255

ghalliday · 2025-04-17T14:21:41Z

                                if (dir.length()&&isPathSepChar(dir.charAt(dir.length()-1)))
                                    dir.setLength(dir.length()-1);
-                                unsigned drv = getPathDrive(dir.str()); // should match c
+                                unsigned drv = isContainerized() ? getPathDrive(dir.str()) : 0; // should match c


Is this test around the correct way?

No, that is wrong. It should be isContainerized() ? 0 : getPathDrive(dir.str());

ghalliday

@jackdelv please squash

jackdelv · 2025-04-17T15:00:20Z

@ghalliday Squashed.

ghalliday · 2025-04-17T15:08:51Z

+        ForEachItemIn(i1, list) {
+            const char *planeName = list.item(i1);
+            Owned<IPropertyTree> plane = getStoragePlane(planeName);
+            if (isContainerized) {


isContainerized()?

jackdelv requested a review from jakesmith March 19, 2025 13:20

jakesmith requested changes Mar 20, 2025

View reviewed changes

jackdelv requested a review from jakesmith March 20, 2025 20:48

jakesmith requested changes Mar 21, 2025

View reviewed changes

jackdelv force-pushed the xrefContainerized branch from 71efafe to 1edf7ce Compare April 2, 2025 14:29

jackdelv requested a review from jakesmith April 2, 2025 14:39

jakesmith requested changes Apr 3, 2025

View reviewed changes

jackdelv requested a review from jakesmith April 3, 2025 21:19

jakesmith reviewed Apr 9, 2025

View reviewed changes

jackdelv requested a review from jakesmith April 9, 2025 20:04

jakesmith reviewed Apr 10, 2025

View reviewed changes

jackdelv requested a review from jakesmith April 10, 2025 14:51

jakesmith reviewed Apr 10, 2025

View reviewed changes

jackdelv requested a review from jakesmith April 10, 2025 16:16

jakesmith reviewed Apr 10, 2025

View reviewed changes

Comment thread dali/sasha/saxref.cpp

jackdelv force-pushed the xrefContainerized branch from f989f89 to d9e01f4 Compare April 10, 2025 18:31

jackdelv requested a review from jakesmith April 10, 2025 18:33

jakesmith reviewed Apr 11, 2025

View reviewed changes

jackdelv requested a review from jakesmith April 11, 2025 14:58

jakesmith reviewed Apr 15, 2025

View reviewed changes

jackdelv force-pushed the xrefContainerized branch from 8f9269d to d0ef745 Compare April 15, 2025 13:09

jackdelv requested a review from jakesmith April 15, 2025 13:10

jakesmith reviewed Apr 16, 2025

View reviewed changes

jackdelv requested a review from jakesmith April 16, 2025 14:54

jakesmith reviewed Apr 16, 2025

View reviewed changes

jackdelv requested a review from jakesmith April 16, 2025 16:14

jakesmith reviewed Apr 16, 2025

View reviewed changes

jackdelv force-pushed the xrefContainerized branch from 706fd44 to 7e55d41 Compare April 16, 2025 17:20

jackdelv changed the base branch from master to candidate-9.12.x April 16, 2025 17:21

jackdelv requested a review from jakesmith April 16, 2025 17:22

jakesmith approved these changes Apr 17, 2025

View reviewed changes

jakesmith requested a review from ghalliday April 17, 2025 09:23

ghalliday reviewed Apr 17, 2025

View reviewed changes

jackdelv requested a review from ghalliday April 17, 2025 14:54

ghalliday approved these changes Apr 17, 2025

View reviewed changes

jackdelv force-pushed the xrefContainerized branch from 457cef5 to c4c0177 Compare April 17, 2025 14:59

ghalliday requested changes Apr 17, 2025

View reviewed changes

HPCC-30365 Add XREF Sasha service to K8s

858cef8

jackdelv force-pushed the xrefContainerized branch from c4c0177 to 858cef8 Compare April 17, 2025 15:17

ghalliday approved these changes Apr 17, 2025

View reviewed changes

ghalliday merged commit 7695468 into hpcc-systems:candidate-9.12.x Apr 17, 2025
28 of 29 checks passed

Conversation

jackdelv commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

github-actions Bot commented Mar 19, 2025

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

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!

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!

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!

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!

jakesmith left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

jackdelv commented Apr 2, 2025

Uh oh!

AttilaVamos commented Apr 2, 2025

Uh oh!

jakesmith commented Apr 3, 2025

Uh oh!

jackdelv commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackdelv commented Apr 3, 2025

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jackdelv commented Mar 19, 2025 •

edited

Loading

jackdelv commented Apr 3, 2025 •

edited

Loading

jakesmith Apr 9, 2025 •

edited

Loading