HPCC-33710 Add k8s engine Sasha access support #19649

jakesmith · 2025-03-20T12:18:42Z

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:

Copilot

Pull Request Overview

This PR adds engine Sasha access support by introducing a new label "accessSasha" into the Helm templates used for Thor and EclAgent deployments.

Added a static "accessSasha: "yes"" label in metadata sections in both thor.yaml and eclagent.yaml.
Introduced conditional logic for "accessSasha" in spec sections based on child process settings in thor.yaml and eclagent.yaml.

Reviewed Changes

Copilot reviewed 2 out of 7 changed files in this pull request and generated 2 comments.

File	Description
helm/hpcc/templates/thor.yaml	Added "accessSasha" label in metadata and dynamic label in spec to control access based on child process configuration
helm/hpcc/templates/eclagent.yaml	Added "accessSasha" label in metadata and dynamic label in spec for conditional access

Files not reviewed (5)

common/thorhelper/enginecontext.hpp: Language not supported
ecl/eclagent/eclagent.ipp: Language not supported
plugins/workunitservices/workunitservices.cpp: Language not supported
roxie/ccd/ccdcontext.cpp: Language not supported
thorlcr/graph/thgraphslave.cpp: Language not supported

Comments suppressed due to low confidence (1)

helm/hpcc/templates/thor.yaml:221

This static assignment of 'accessSasha: "yes"' in another metadata block may lead to inconsistencies with the dynamic label set later. Align this with the conditional logic used in the spec if applicable.

accessSasha: "yes"

Copilot · 2025-03-20T12:19:35Z

helm/hpcc/templates/thor.yaml

@@ -87,6 +87,7 @@ data:
        metadata:
          labels:
            accessDali: "yes"
+            accessSasha: "yes"


The static assignment of 'accessSasha: "yes"' in the metadata block is inconsistent with the dynamic conditional assignment used in the spec. Consider using a similar conditional expression to ensure consistent behavior.

Suggested change

accessSasha: "yes"

accessSasha: {{ if .Values.accessSasha }} "yes" {{ else }} "no" {{ end }}

Copilot · 2025-03-20T12:19:36Z

helm/hpcc/templates/eclagent.yaml

@@ -61,6 +61,7 @@ data:
          labels:
            {{- include "hpcc.addStandardLabels" (dict "root" $ "component" $apptype "name" "eclagent" "instance" $appJobName "instanceOf" (printf "%s-job" .me.name)) | indent 12 }}
            accessDali: "yes"
+            accessSasha: "yes"


The static 'accessSasha: "yes"' label in the metadata section contrasts with the dynamic assignment later in the file. Consider using a consistent approach (static or conditional) throughout to avoid configuration mismatches.

Suggested change

accessSasha: "yes"

accessSasha: {{ .me.accessSasha | default "yes" | quote }}

github-actions · 2025-03-20T12:21:00Z

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

Jirabot Action Result:
Assigning user: [email protected]
Workflow Transition To: Merge Pending
Updated PR

Signed-off-by: Jake Smith <[email protected]>

jakesmith · 2025-03-20T15:36:08Z

common/thorhelper/enginecontext.hpp

@@ -43,6 +43,7 @@ interface IEngineContext
    virtual StringBuffer &getQueryId(StringBuffer &result, bool isShared) const = 0;
    virtual void onTermination(QueryTermCallback callback, const char *key, bool isShared) const = 0;
    virtual void getManifestFiles(const char *type, StringArray &files) const = 0;
+    virtual bool allowSashaAccess() const = 0;


@ghalliday - any concerns re. adding this to IEngineContext, and/or does it need to be and end like this?

I think there are no problems modifying this interface.

ghalliday

A question and minor comment

ghalliday · 2025-03-21T16:42:44Z

common/thorhelper/enginecontext.hpp

@@ -43,6 +43,7 @@ interface IEngineContext
    virtual StringBuffer &getQueryId(StringBuffer &result, bool isShared) const = 0;
    virtual void onTermination(QueryTermCallback callback, const char *key, bool isShared) const = 0;
    virtual void getManifestFiles(const char *type, StringArray &files) const = 0;
+    virtual bool allowSashaAccess() const = 0;


I think there are no problems modifying this interface.

ghalliday · 2025-03-21T16:44:13Z

initfiles/componentfiles/configschema/xsd/thor.xsd

@@ -94,8 +94,8 @@
                                          hpcc:tooltip="Verify that a read file's CRC matches the published meta data CRC"/>
                            <xs:attribute name="crcWriteEnabled" type="xs:boolean" hpcc:displayName="CRC Write Enabled" hpcc:presetValue="true"
                                          hpcc:tooltip="Calculate and publish a CRC per published disk output file"/>
-                            <xs:attribute name="slaveDaliClient" type="xs:boolean" hpcc:displayName="Slave Dali Client" hpcc:presetValue="false"
-                                          hpcc:tooltip="Set to true if thor slaves require dali connectivity"/>
+                            <xs:attribute name="allowDaliAccess" type="xs:boolean" hpcc:displayName="Worker Dali Client" hpcc:presetValue="false"


Should there be similar entries for the new sasha options?

it's "documentation" only, all Debug options will be copied regardless by thor.xsl. But guess should add (even though don't want to encourage).

ghalliday · 2025-03-21T16:49:19Z

thorlcr/slave/slavmain.cpp

-                        if (!getExpertOptBool("slaveDaliClient") && workUnitInfo->getPropBool("Debug/slavedaliclient", false))
+                        // slaveDaliClient option deprecated, but maintained for compatibility
+                        if ((!getExpertOptBool("allowDaliAccess") && workUnitInfo->getPropBool("Debug/allowdaliaccess", false)) ||
+                            (!getExpertOptBool("slaveDaliClient") && workUnitInfo->getPropBool("Debug/slavedaliclient", false)))


I doubt it will cause a problem in practice, but this condition isn't quite right. It should be

if ((!getExpertOptBool("allowDaliAccess") && !getExpertOptBool("slaveDaliClient") && (workUnitInfo->getPropBool("Debug/allowdaliaccess", false)) || (workUnitInfo->getPropBool("Debug/slavedaliclient", false)))

similar for the other tests.

needs to be this actually I think (brackets enclosing the || terms):

if (!getExpertOptBool("allowDaliAccess") && !getExpertOptBool("slaveDaliClient") && (workUnitInfo->getPropBool("Debug/allowdaliaccess", false) || workUnitInfo->getPropBool("Debug/slavedaliclient", false)))

Signed-off-by: Jake Smith <[email protected]>

ghalliday

Please squash

jakesmith requested a review from Copilot March 20, 2025 12:18

jakesmith force-pushed the HPCC-33710-allowSashaAccess branch from bb02c5e to 7ab193a Compare March 20, 2025 12:19

jakesmith changed the title ~~HPCC-33710 Add engine Sasha access support~~ HPCC-33710 Add k8s engine Sasha access support Mar 20, 2025

Copilot AI reviewed Mar 20, 2025

View reviewed changes

HPCC-33710 Add k8s engine Sasha access support

edd85d5

Signed-off-by: Jake Smith <[email protected]>

jakesmith force-pushed the HPCC-33710-allowSashaAccess branch from 7ab193a to edd85d5 Compare March 20, 2025 15:34

jakesmith requested a review from ghalliday March 20, 2025 15:35

jakesmith commented Mar 20, 2025

View reviewed changes

ghalliday reviewed Mar 21, 2025

View reviewed changes

jakesmith added 2 commits March 21, 2025 17:23

HPCC-33710 Add allowSashaAccess to thor.xsd

5b3d50a

Signed-off-by: Jake Smith <[email protected]>

HPCC-33710 fix expressions on expert conditions

090e053

Signed-off-by: Jake Smith <[email protected]>

jakesmith force-pushed the HPCC-33710-allowSashaAccess branch from 71875f1 to 090e053 Compare March 21, 2025 17:23

jakesmith requested a review from ghalliday March 21, 2025 17:23

ghalliday approved these changes Mar 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-33710 Add k8s engine Sasha access support #19649

HPCC-33710 Add k8s engine Sasha access support #19649

jakesmith commented Mar 20, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI Mar 20, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Mar 20, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

github-actions bot commented Mar 20, 2025

jakesmith Mar 20, 2025

ghalliday Mar 21, 2025

ghalliday left a comment

ghalliday Mar 21, 2025

ghalliday Mar 21, 2025

jakesmith Mar 21, 2025

ghalliday Mar 21, 2025

jakesmith Mar 21, 2025

ghalliday left a comment

	accessSasha: "yes"
	accessSasha: {{ if .Values.accessSasha }} "yes" {{ else }} "no" {{ end }}

	accessSasha: "yes"
	accessSasha: {{ .me.accessSasha \| default "yes" \| quote }}

HPCC-33710 Add k8s engine Sasha access support #19649

Are you sure you want to change the base?

HPCC-33710 Add k8s engine Sasha access support #19649

Conversation

jakesmith commented Mar 20, 2025 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Mar 20, 2025

Choose a reason for hiding this comment

Copilot AI Mar 20, 2025

Choose a reason for hiding this comment

github-actions bot commented Mar 20, 2025

jakesmith Mar 20, 2025

Choose a reason for hiding this comment

ghalliday Mar 21, 2025

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday Mar 21, 2025

Choose a reason for hiding this comment

ghalliday Mar 21, 2025

Choose a reason for hiding this comment

jakesmith Mar 21, 2025

Choose a reason for hiding this comment

ghalliday Mar 21, 2025

Choose a reason for hiding this comment

jakesmith Mar 21, 2025

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

jakesmith commented Mar 20, 2025 •

edited

Loading