HPCC-30809 Add ESP WsSasha service and implement 3 methods #18065

wangkx · 2023-11-21T19:38:45Z

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-21T20:06:48Z

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

wangkx · 2023-11-21T21:15:26Z

@jakesmith I created this PR based on the comments on #18018.
There are 3 commits here. The 1st commit is a copy of c2ac378. The 2nd one contains the changes related to the SashaAccess settings for bare-metal environment. Those changes should be in the 1st commit. Inside the last commit, the GetVersion is implemented.

jakesmith

@wangkx - please see feedback.

It's probably worth a 1:1 chat about this before moving forward.

jakesmith · 2023-11-22T13:14:27Z

initfiles/componentfiles/configschema/xsd/buildset.xml

@@ -76,6 +76,7 @@
          <AuthenticateFeature description="Access to sign ECL code" path="CodeSignAccess" resource="CodeSignAccess" service="ws_codesign"/>
          <AuthenticateFeature description="Access to LogAccess service" path="WsLogAccess" resource="WsLogAccess" service="ws_logaccess"/>
          <AuthenticateFeature description="Access to DFS service" path="DfsAccess" resource="DfsAccess" service="ws_dfsservice"/>
+          <AuthenticateFeature description="Access to HPCC Archive" path="SashaAccess" resource="SashaAccess" service="ws_sasha"/>


I am not sure we should be adding this new service to bare-metal at all (or the ability to configure it via configmgr).
At least for now, bare-metal should continue to work as it has, and continue to be able to use the existing CLI, direct to sasha.
The main impetus behind this change is that in cloud, the sasha services cannot be directly reached anymore, and access to them must go via an esp service.

For now, let's ensure the bare-metal sasah CLI continues to work directly via a command line provided sasha 'server', but also (for cloud) the CLI should support being given a service=, which talks to the new esp service.

jakesmith · 2023-11-22T13:17:01Z

esp/smc/SMCLib/eclwatch_errorlist.hpp

@@ -40,6 +40,7 @@
 #define ECLWATCH_DFU_EX_ACCESS_DENIED           ECLWATCH_ERROR_START+14
 #define ECLWATCH_FILE_SPRAY_ACCESS_DENIED       ECLWATCH_ERROR_START+15
 #define ECLWATCH_FILE_DESPRAY_ACCESS_DENIED ECLWATCH_ERROR_START+16
+#define ECLWATCH_SASHA_ACCESS_DENIED ECLWATCH_ERROR_START+17


trivial: not only one, but would be better if these were consistently indented and aligned.

jakesmith · 2023-11-22T13:41:24Z

esp/services/ws_sasha/ws_sashaservice.cpp

+        context.ensureFeatureAccess(FEATURE_URL, SecAccess_Read, ECLWATCH_SASHA_ACCESS_DENIED, "WSSashaEx.GetVersion: Permission denied.");
+
+        StringBuffer results;
+        Owned<INode> node = createSashaNode(wuArchiverType);


future: this isn't too costly, but it is worth considering setting these up once and not having to walk through the configuration finding the service, and creating the INode each time. But ensuring they are updated on configuraton changes only, i.e. using a ConfigUpdateFunc (e.g. like the one in validateTargetName, in TpContainer.cpp)

Don't change now though.

esp/services/ws_sasha/ws_sashaservice.cpp

jakesmith · 2023-11-22T16:51:38Z

dali/sasha/saruncmd.cpp

+    outBuffer.appendf("Sasha server[%s]: Version %s", host.str(), id.str());
+}
+
+extern SARUNCMD_API void runSashaCommand(SashaCommandAction action, INode *node, StringBuffer &outBuffer)


I am not sure this will be wanted, I think it will be better to define the sasha commands and expose instead.
Either as exposed functions, or contained in a class, so that it can be reused with common settings.
e.g.:

class CSashaCmdExecutor { Linked<INode> node; unsigned defaultTimeoutMs; StringBuffer nodeText; const char *getNodeText() { if (0 == nodeText.length()) node->endpoint().getEndpointHostText(nodeText); return nodeText; } bool restoreWorkunit(const char *wuid, bool dfu, StringBuffer &serverResponse) { Owned<ISashaCommand> cmd = createSashaCommand(); cmd->setAction(SCA_RESTORE); if (dfu) cmd->setDFU(true); cmd->addId(wuid); if (!cmd->send(node, defaultTimeoutMs)) throw makeStringExceptionV(-1, "Could not connect to Sasha server on %s", getNodeText()); if (cmd->numIds()==0) { // nothing restored return false; } cmd->getId(0, serverResponse); return true; } bool archiveWorkunit(const char *wuid, bool dfu, bool deleteAfterArchiving, StringBuffer &serverResponse) { Owned<ISashaCommand> cmd = createSashaCommand(); cmd->setAction(SCA_ARCHIVE); if (dfu) cmd->setDFU(true); cmd->addId(wuid); if (!cmd->send(node, defaultTimeoutMs)) throw makeStringExceptionV(-1, "Could not connect to Sasha server on %s", getNodeText()); if (cmd->numIds()==0) { // nothing restored return false; } cmd->getId(0, serverResponse); return true; } public: CSashaCmdExecutor(INode *_node, unsigned _defaultTimeoutSecs=60) : node(_node) { defaultTimeoutMs = _defaultTimeoutSecs * 1000; } CSashaCmdExecutor(const SocketEndpoint &ep, unsigned _defaultTimeoutSecs=60) { node.setown(createINode(ep)); defaultTimeoutMs = _defaultTimeoutSecs * 1000; } CSashaCmdExecutor(const char *server, unsigned port=0, unsigned _defaultTimeoutSecs=60) { SocketEndpoint ep(server, port); node.setown(createINode(ep)); defaultTimeoutMs = _defaultTimeoutSecs * 1000; } StringBuffer &getVersion(StringBuffer &version) { Owned<ISashaCommand> cmd = createSashaCommand(); cmd->setAction(SCA_GETVERSION); if (!cmd->send(node, defaultTimeoutMs)) throw makeStringExceptionV(-1, "Could not connect to Sasha server on %s", getNodeText()); if (!cmd->getId(0, version)) throw makeStringExceptionV(-1, "Sasha server[%s]: Protocol error", getNodeText()); } bool restoreECLWorkUnit(const char *wuid, StringBuffer &serverResponse) { return restoreWorkunit(wuid, false, serverResponse); } bool restoreDFUWorkUnit(const char *wuid, StringBuffer &serverResponse) { return restoreWorkunit(wuid, true, serverResponse); } bool archiveECLWorkUnit(const char *wuid, StringBuffer &serverResponse) { return archiveWorkunit(wuid, false, true, serverResponse); } bool archiveDFUWorkUnit(const char *wuid, StringBuffer &serverResponse) { return archiveWorkunit(wuid, true, true, serverResponse); } bool backupECLWorkUnit(const char *wuid, StringBuffer &serverResponse) { return archiveWorkunit(wuid, true, false, serverResponse); } bool backupDFUWorkUnit(const char *wuid, StringBuffer &serverResponse) { return archiveWorkunit(wuid, true, false, serverResponse); } };

then the service code can use/call these methods directly.

jakesmith · 2023-11-22T16:53:01Z

dali/sasha/saruncmd.hpp

+    #define SARUNCMD_API DECL_IMPORT
+#endif
+
+extern SARUNCMD_API void runSashaCommand(SashaCommandAction action, INode *node, StringBuffer &outBuffer);


see above. I think it would be better to clearly define the commands and their parameters/results, and expose those.

Then the service can use them.
And the ECM can map to them.

jakesmith · 2023-11-22T17:09:53Z

dali/sasha/sasha.cpp

@@ -374,23 +375,11 @@ ISashaCommand *createCommand(unsigned argc, char* argv[], SocketEndpoint &server
    return cmd.getClear();
 }

-bool getVersion(INode *node)
+void getVersion(INode *node)


see other comments,
It would be better if each command implementation was contained in saruncmd.

The interaction with a directly accessible sasha 'server' should continue to work as it does now for the time being.
But there should also be a way to interact with the new esp service via the CLI, e.g. by specifying "service=" instead of "server="

The implementation of this CLI to the service will necessarily have to look quite different, using the new saruncmd's.
It's a good reason to add just a few command for now, in order to get the structure right.
Ultimately, "createCommand" may need refactoring, since it's doing serveral things:

parsing the command line

creating an ISashaCommand as a repository for the parsed command (which also implements server<>client send/receipt)

interactive behaviour (via 'confirm').

Continuing to use it to parse the command line into a ISashaCommand is probably fine/pragmatic, which can then be used to decide what/how to run selected saruncmd's after that when talking to the esp service.

wangkx · 2023-12-04T20:57:15Z

@jakesmith please review

jakesmith

@wangkx - looks good in general, please see feedback.

jakesmith · 2023-12-07T10:19:08Z

dali/sasha/saruncmd.cpp

+#include "sacmd.hpp"
+#include "saruncmd.hpp"
+
+class CSashaCmdExecutor : implements ISashaCmdExecutor, public CInterface


trivial: CInterfaceOf is more efficient/saves a vmt.
and you then don't need the IMPLEMENT_IINTERFACE below either.

dali/sasha/saruncmd.cpp

jakesmith · 2023-12-07T10:47:49Z

dali/sasha/saruncmd.cpp

+
+class CSashaCmdExecutor : implements ISashaCmdExecutor, public CInterface
+{
+    Linked<INode> node;


trivial: see other comment re. single ctor

if a single ctor that creates a INode, it would be more logical if this was a Owned.

jakesmith · 2023-12-07T10:50:17Z

dali/sasha/saruncmd.cpp

+    Linked<INode> node;
+    unsigned defaultTimeoutMs = 60;
+    StringBuffer nodeText;
+    const char *getNodeText()


minor: as this is a class, it would probably be better to make this a const, and fetch the text in the ctor.

jakesmith · 2023-12-07T11:34:31Z

dali/sasha/saruncmd.cpp

+            throw makeStringExceptionV(-1, "Sasha server[%s]: Protocol error", getNodeText());
+        return version;
+    }
+    bool restoreECLWorkUnit(const char *wuid, StringBuffer &serverResponse)


maybe worth revisiting in future.
Ideally, this command interface, wouldn't see arbitrary response messages coming directly back to the caller.
The server would instead ensure a consistent response of success/failure, a failure would be thrown as an error, and/or if there was auxiliary info to be related, it would be separate.

Perhaps for now, to clear up the interface, consider storing the response in a member, e.g. lastServerMessage,
removing from the interface, and allowing the caller to query it if it wants it.

jakesmith · 2023-12-07T11:44:37Z

esp/services/ws_sasha/ws_sashaservice.cpp

+        Owned<ISashaCmdExecutor> executor = createSashaCommandExecutor(wuType == CWUTypes_ECL ? wuArchiverType : dfuwuArchiverType);
+
+        StringBuffer results;
+        bool sashaCmdReturn = false;


is sashaCmdSuccess more intuitive?

jakesmith · 2023-12-07T11:51:30Z

esp/services/ws_sasha/ws_sashaservice.hpp

+    void setResults(const char* wuid, const char* action, StringBuffer& results);
+
+public:
+    IMPLEMENT_IINTERFACE;


unnecessary

jakesmith · 2023-12-07T12:06:14Z

esp/scm/ws_sasha.ecm

+    ESPmethod [auth_feature("SashaAccess:FULL")] ArchiveWU(ArchiveWURequest, ResultResponse); //archive ECL WUs or DFU WUs
+    ESPmethod [auth_feature("SashaAccess:FULL")] RestoreWU(RestoreWURequest, ResultResponse); //restore ECL WUs or DFU WUs
+    //The ArchiveWU deletes the workunit from Dali whereas BackupWU leave it in place.
+    ESPmethod [auth_feature("SashaAccess:FULL")] BackupWU(BackupWURequest, ResultResponse);


from the cli, it makes sense for archive and backup to be separate.
From the service point of view, the only difference is the whether it deletes as well as archiving, I think it would be clearer if this was one service method, with an optional 'deleteOnSuccess' option.

jakesmith · 2023-12-07T12:08:07Z

esp/services/ws_sasha/ws_sashaservice.cpp

+        context.ensureFeatureAccess(FEATURE_URL, SecAccess_Read, ECLWATCH_SASHA_ACCESS_DENIED, "WSSashaEx.GetVersion: Permission denied.");
+
+        StringBuffer results;
+        Owned<ISashaCmdExecutor> executor = createSashaCommandExecutor(wuArchiverType); 


Perhaps instead of recreating per command, there should be a dfu + ecl instances of the command executor, as members?

jakesmith · 2023-12-07T12:18:33Z

initfiles/componentfiles/configxml/@temp/esp_service_WsSMC.xsl

@@ -137,6 +137,10 @@ This is required by its binding with ESP service '<xsl:value-of select="$espServ
            <xsl:with-param name="bindingNode" select="$bindingNode"/>
            <xsl:with-param name="authNode" select="$authNode"/>
        </xsl:apply-templates>
+        <xsl:apply-templates select="." mode="ws_sasha">


I'd rather not add any bare-metal changes at this point.
This should be primarily for cloud, and test there.
I think you'll need to add WsSasha to eclwatch/applications.yaml - but you may need to check with @afishbeck

Will remove this code block.
The WsSasha is already in the eclwatch/applications.yaml.

ok, please test in a k8s setup

jakesmith

@wangkx - looks close, please see minor comments.

jakesmith · 2023-12-14T10:27:48Z

dali/sasha/saruncmd.cpp

        return version;
    }
-    bool restoreECLWorkUnit(const char *wuid, StringBuffer &serverResponse)
+    virtual StringBuffer &getLastServerMessage() const override


minor: more conventional to either return a const char * and for method to be queryXXX,
or provide a StringBuffer reference to copy into and return it (e.g. like getVersion).
And would be good if getLastServerMessage + getVersion followed same pattern.

jakesmith · 2023-12-14T12:15:07Z

esp/services/ws_sasha/ws_sashaservice.cpp

+        bool sashaCmdSuccess = false;
+        if (req.getDeleteOnSuccess())
+        {
+            if ((wuType == CWUTypes_ECL))


trivial: unnecessary extra ( ) (also on line 108)

jakesmith · 2023-12-14T12:21:02Z

esp/services/ws_sasha/ws_sashaservice.cpp

+            else
+                sashaCmdSuccess = executor->backupDFUWorkUnit(isEmptyString(wuid) ? "*" : wuid);
+        }
+        setResults(sashaCmdSuccess ? executor->getLastServerMessage().str() : nullptr, wuid, "archived", resp);


not sure setResults + the server message semantics are very clear.
Perhaps break it down to setSuccess and setFailure method so it is explicit when/what it is setting in the response and why.

jakesmith

@wangkx - looks good. Please squash.

wangkx · 2023-12-14T21:50:53Z

@jakesmith squashed.

jakesmith

@wangkx - looks good.

ghalliday · 2023-12-15T11:26:40Z

@jakesmith Just to check, is 9.4 the correct target for this, or would master be better?
For 9.4: It is unlikely to break anything, and ? is needed sooner rather than later
For 9.6: It is new functionality.

jakesmith · 2023-12-18T10:06:49Z

@jakesmith Just to check, is 9.4 the correct target for this, or would master be better? For 9.4: It is unlikely to break anything, and ? is needed sooner rather than later For 9.6: It is new functionality.

It is new functionality and unlikely to break anything, but I think master would be more appropriate.
@wangkx - please retarget.

jakesmith

@wangkx - please retarget to master.

1. Add ws_sasha framework 2. Add GetVersion/Archive/Restore Revise based on review: 1. Remove 2 constructors. 2. Merge BackupWU into ArchiveWU with an optional 'deleteOnSuccess' option. 3. Store the sasha server response into the lastServerMessage and allow a caller to query it. 4. Store the ISashaCmdExecutors as members to avoid recreating. 5. Roll back the changes in esp_service_WsSMC.xsl. 6. Change the getLastServerMessage() to match with the pattern of the getVersion(). 7. Remove extra (). 8. Divide the setResults() to setSuccess and setFailure. Signed-off-by: wangkx <[email protected]>

wangkx · 2023-12-18T14:37:27Z

@ghalliday @jakesmith this PR has been re-targeted to master and tested.

BTW: I noticed that the build and smoke tests is not started. I saw the same thing for my PR #18137. Is it because those PRs have already been approved?

github-actions · 2023-12-18T16:15:48Z

https://track.hpccsystems.com/browse/HPCC-30809
This pull request is already registered

jakesmith · 2023-12-18T16:16:36Z

BTW: I noticed that the build and smoke tests is not started. I saw the same thing for my PR #18137. Is it because those PRs have already been approved?

Don't the fact that it's approved is relevant, not sure why they weren't running.
I closed the PR and reopened it - and now they are.

wangkx · 2023-12-18T16:20:49Z

Thanks for reopen. I did the same for #18137.

ghalliday · 2023-12-19T10:00:53Z

I think the tests often fail to run if you change the target branch.

wangkx requested a review from jakesmith November 21, 2023 21:13

jakesmith requested changes Nov 22, 2023

View reviewed changes

wangkx force-pushed the h30809v01 branch from dd9fbb3 to 11d6ca6 Compare December 4, 2023 18:53

wangkx requested a review from jakesmith December 4, 2023 20:57

wangkx changed the title ~~HPCC-30809 Add ESP WsSasha service and implement GetVersion~~ HPCC-30809 Add ESP WsSasha service and implement 4 methods Dec 4, 2023

jakesmith reviewed Dec 7, 2023

View reviewed changes

wangkx force-pushed the h30809v01 branch from 11d6ca6 to 0814710 Compare December 9, 2023 00:13

wangkx requested a review from jakesmith December 9, 2023 01:29

jakesmith reviewed Dec 14, 2023

View reviewed changes

wangkx requested a review from jakesmith December 14, 2023 15:36

jakesmith reviewed Dec 14, 2023

View reviewed changes

wangkx force-pushed the h30809v01 branch 2 times, most recently from eb27162 to 78e568e Compare December 14, 2023 19:18

wangkx changed the title ~~HPCC-30809 Add ESP WsSasha service and implement 4 methods~~ HPCC-30809 Add ESP WsSasha service and implement 3 methods Dec 14, 2023

wangkx requested a review from jakesmith December 14, 2023 21:48

jakesmith approved these changes Dec 15, 2023

View reviewed changes

jakesmith self-requested a review December 18, 2023 10:06

jakesmith reviewed Dec 18, 2023

View reviewed changes

wangkx force-pushed the h30809v01 branch from 78e568e to 27f36d9 Compare December 18, 2023 14:16

wangkx changed the base branch from candidate-9.4.x to master December 18, 2023 14:17

jakesmith closed this Dec 18, 2023

jakesmith reopened this Dec 18, 2023

ghalliday merged commit c1acb93 into hpcc-systems:master Dec 19, 2023
47 checks passed

HPCC-30809 Add ESP WsSasha service and implement 3 methods #18065

HPCC-30809 Add ESP WsSasha service and implement 3 methods #18065

Conversation

wangkx commented Nov 21, 2023

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 21, 2023

wangkx commented Nov 21, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx commented Dec 4, 2023

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

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

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 14, 2023

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday commented Dec 15, 2023

jakesmith commented Dec 18, 2023

jakesmith left a comment

Choose a reason for hiding this comment

wangkx commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

jakesmith commented Dec 18, 2023

wangkx commented Dec 18, 2023

ghalliday commented Dec 19, 2023