HPCC-34746 Allow cost definitions for agent, manager and workers in Thor's yaml by shamser · Pull Request #20683 · hpcc-systems/HPCC-Platform

shamser · 2025-11-26T12:19:43Z

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-11-26T12:20:08Z

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

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

shamser · 2025-11-26T12:24:11Z

Documentation jira: https://hpccsystems.atlassian.net/browse/HPCC-35425

Copilot

Pull request overview

This pull request implements granular cost configuration for Thor subcomponents (manager, worker, and eclagent), allowing each component to have its own cost definition with automatic inheritance from global cost settings when not explicitly specified.

Key Changes:

Added per-subcomponent cost configuration support in Thor's helm templates with inheritance fallback to global.cost.perCpu
Updated C++ cost calculation functions to query subcomponent-specific cost values
Enhanced getResourcedCpus() to handle both containerized and bare-metal deployments consistently

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`helm/hpcc/values.yaml`	Added example configuration comments for manager, worker, and eclagent cost definitions
`helm/hpcc/values.schema.json`	Defined `thorSubComponent` schema with cost properties and marked top-level thor.cost as deprecated
`helm/hpcc/templates/thor.yaml`	Implemented `setThorComponentCost` helper and cost inheritance logic for Thor subcomponents
`helm/hpcc/templates/_helpers.tpl`	Updated `generateGlobalConfigMap` to accept optional `excludePerCpuCost` parameter for selective cost exclusion
`helm/hpcc/templates/_warnings.tpl`	Added validation warning for inconsistent Thor subcomponent cost configurations
`helm/hpcc/templates/{roxie,esp,dali,etc}.yaml`	Updated all template calls to `generateGlobalConfigMap` with new dict-based signature
`common/workunit/workunit.cpp`	Enhanced cost calculation functions to support subcomponent-specific cost lookups
`system/jlib/jutil.cpp`	Updated `getResourcedCpus()` to handle bare-metal deployments using affinity CPUs
`testing/helm/tests/thor-*.yaml`	Added test cases for explicit costs, inheritance, and conflict scenarios

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

helm/hpcc/values.schema.json

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

helm/hpcc/templates/_warnings.tpl

helm/hpcc/templates/thor.yaml

helm/hpcc/values.schema.json

helm/hpcc/templates/thor.yaml

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

common/workunit/workunit.cpp

helm/hpcc/templates/thor.yaml

common/workunit/workunit.cpp

testing/helm/tests/thor-subcomponent-inherit.yaml

ghalliday

@shamser some questions.
Also, the jira does not have a conclusion - a summary of the changes and how to configure them - or a link to the documentation jira.

ghalliday · 2025-12-02T09:49:42Z

common/workunit/workunit.cpp

+        VStringBuffer subComponentSection("%s/cost/@perCpu", subComponentName);
+        double costCpuHour = getComponentConfigSP()->getPropReal(subComponentSection.str(), -1.0);
+        if (costCpuHour < 0.0)
+            return 0.0;


I have a similar question to copilot. This now means that the manager will default to 0 cost, when previously it would have been the default cost - that looks like a backward compatibility problem. Should it only use that cost if that property exists, otherwise use the default?

Is the section now always filled in?

What is the situation for bare-metal?

The backward compatibility is handled by the Thor templates as suggested here: #20263 (comment) .

Again, the thor templates will ensure that the cost section is always created.

In bare-metal, the global perCPU rate will continue to be used which I believe is reasonable and make sense for bare-metal.

@shamser can you clarify this comment. "In bare-metal...", the getThorManagerRate() and getThorWorkerRate() don't seem to have any containerised specific logic.

@shamser can you clarify this comment. "In bare-metal...", the getThorManagerRate() and getThorWorkerRate() don't seem to have any containerised specific logic.

In a bare‑metal environment, there is no resource section that specifies the number of CPUs. Additionally, different components can be deployed to different node pools, meaning they may run on different VM types with varying CPU costs. On bare metal, CPU costs are less likely to vary, so using a single per‑CPU cost is generally sufficient. @ghalliday

If I am reading the code correctly, if getThorManagerRate() is called in bare-metal it will always return 0, because getCostCpuHour() will return 0.
I don't see how this returns non-zero thor costs for bare-metakl (but I haven't tested to verify).

I see the issue. I'll re-introduce the drop-through to use the legacy cost values. (It was removed following a code review comment suggesting it wasn't required for containerized. I'll add back legacy support as it's the cleanest way to address this.)

ghalliday · 2025-12-02T09:50:52Z

common/workunit/workunit.cpp

 }

-static double getCostCpuHour()
+static double getCostCpuHour(const char * subComponentName = nullptr)


You could remove the default parameter, so you explicitly pass null for the default.

ghalliday · 2025-12-02T09:57:26Z

system/jlib/jutil.cpp

+    }
+    else
+    {
+        return (double) getAffinityCpus();


minor: better to allow resource configuration in containerized, and only return this if no resourceTree. i.e. aim to keep containerized and bare-metal as similar as possible.

In a containerized environment, the Thor manager calculates the job cost. If we use getAffinityCpus(), the Thor worker, agent, and manager costs will all be derived from the number of CPUs assigned to the manager container. This results in Thor job costs that are almost always incorrect, and it can be very difficult to diagnose why the values are so inaccurate.
Given that, it seems more reasonable for Thor costs to default to zero in a misconfigured containerized setup, rather than producing cost values that are consistently and obviously wrong.

helm/hpcc/templates/_warnings.tpl

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

jakesmith

@shamser - will pass back to you for now until GH's comments are looked at.

shamser · 2026-02-12T14:35:36Z

ummary of the changes and how to configure them - or a link to the documentation jira.

I have add a description of the changes. The documentation jira is in the parent jira: https://hpccsystems.atlassian.net/browse/HPCC-35425.

ghalliday

@shamser one question/clarification

ghalliday · 2026-02-13T14:13:18Z

common/workunit/workunit.cpp

+        VStringBuffer subComponentSection("%s/cost/@perCpu", subComponentName);
+        double costCpuHour = getComponentConfigSP()->getPropReal(subComponentSection.str(), -1.0);
+        if (costCpuHour < 0.0)
+            return 0.0;


@shamser can you clarify this comment. "In bare-metal...", the getThorManagerRate() and getThorWorkerRate() don't seem to have any containerised specific logic.

shamser · 2026-02-16T16:30:40Z

@ghalliday In a bare‑metal environment, there is no resource section that specifies the number of CPUs. Additionally, different components can be deployed to different node pools, meaning they may run on different VM types with varying CPU costs. On bare metal, CPU costs are less likely to vary, so using a single per‑CPU cost is generally sufficient.

…hor's yaml - Added cost definitions for agent, manager and workers in Thor's yaml - The new component level cost definition overrides existing thor cost definition - Warn if the thor level cost definition is not consistent with the component level cost definition - Warn if thor level cost definition is being used - Updated the values.yaml with the new cost definitions (commented out) REDBOOK NOTE: - Breaking change: thor.cost has been depreciated and is no longer supported. Users must migrate to subcomponent-level cost definitions. Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

helm/hpcc/templates/_helpers.tpl

Copilot · 2026-02-20T10:15:55Z

helm/hpcc/templates/thor.yaml

+{{- $eclAgentCostConfig := dict -}}
+{{- $_ := include "setThorComponentCost" (dict "root" .root "me" .me "subComponent" "eclagent" "cost" $eclAgentCostConfig) -}}
+{{- $eclAgentSpecifications := pick .me "eclagent" | default (dict "eclagent" (dict "cost" $eclAgentCostConfig.cost)) -}}
+{{- $eclAgentScope := dict "name" .eclAgentName "type" $eclAgentType "useChildProcesses" .eclAgentUseChildProcesses "replicas" .eclAgentReplicas "maxActive" .me.maxJobs | merge (pick .me "auxQueues" "expert" "keepJobs" "logging" "tracing" "schedulingTimeoutSecs") ((hasKey .me "holdAgent") | ternary (dict "hold" .me.holdAgent) dict) $eclAgentSpecifications.eclagent }}


The inheritance helper populates $eclAgentCostConfig.cost, but if .me.eclagent exists (even without a cost key), pick .me \"eclagent\" is non-empty and the default(...) won’t run—so the computed/inherited cost never gets merged in. Similar logic applies to manager/worker. Consider always merging the computed cost into the picked subcomponent map (e.g., deepCopy the picked map, set/merge cost when missing, then wrap back into {eclagent: ...}) so inheritance works whether the subcomponent section exists or not.

@copilot open a new pull request to apply changes based on this feedback

helm/hpcc/templates/thor.yaml

Copilot · 2026-02-20T10:15:56Z

common/workunit/workunit.cpp

+static double getCostCpuHour(const char * subComponentName)
 {
+    if (!isEmptyString(subComponentName))
+    {
+        VStringBuffer subComponentSection("%s/cost/@perCpu", subComponentName);
+        double costCpuHour = getComponentConfigSP()->getPropReal(subComponentSection.str(), -1.0);
+        if (costCpuHour < 0.0)
+            return 0.0;
+        return costCpuHour;
+    }


When subComponentName is provided but no subcomponent perCpu is configured, this returns 0.0 rather than falling back to the legacy/default sources (top-level cost/@perCpu or global cost). That’s a behavior change that will silently zero out Thor rates when users rely on deprecated thor.cost or only configure global cost. A safer approach is: if the subcomponent value is missing/negative, fall back to the same logic used for the nullptr case.

Copilot · 2026-02-20T10:15:56Z

testing/helm/tests/thor-subcomponent-inconsistent-cost.yaml

@@ -0,0 +1,27 @@
+# Test inheritance from global.cost to subcomponents without explicit override


Corrected spelling of 'inconsistant' to 'inconsistent' (also consider renaming the file to match).

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-20T10:15:56Z

helm/hpcc/values.schema.json

+    "thorSubComponent": {
+      "description": "Thor sub-component with resource and cost configuration",
+      "type": "object",
+      "properties": {
+        "cost": {
+          "$ref": "#/definitions/componentCost"
+        }
+      },
+      "additionalProperties": false
+    },


The schema description says the sub-component supports 'resource and cost configuration', but the schema only allows cost (and disallows all other keys via additionalProperties: false). Either update the description to match (cost-only), or add the intended resource-related properties if they’re meant to be supported.

The schema will be modified in subsequent PR to include resources. When that is done, the new "resource" property will be included in thorSubComponent's definition

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

… not available Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

jakesmith

@shamser - please see comments/questions

jakesmith · 2026-02-20T16:59:17Z

helm/hpcc/templates/_helpers.tpl

 cost:
-{{ toYaml .Values.global.cost | indent 2 }}
+{{- if $excludePerCpuCost }}
+{{ toYaml (omit $root.Values.global.cost "perCpu") | indent 2 }}


minor: we try to indent the template scopes for readibiltiy and chomp leading whitespace.

jakesmith · 2026-02-20T17:20:32Z

helm/hpcc/templates/thor.yaml

    global:
-{{ include "hpcc.generateGlobalConfigMap" .root| indent 6 }}
+{{/* Thor uses sub-component costs (manager, worker, eclagent), so exclude global perCpu */}}
+{{ include "hpcc.generateGlobalConfigMap" (dict "root" .root "excludePerCpuCost" true) | indent 6 }}


Shouldn't be excluded everywhere that defines it's own cost.perCpu too?
and/or what's the difference (I may be missing something), if Thor per sub component costs are missing, then it would (without excludePerCpuCost=true) drop through to global?
but how is that different really from e.g. eclagent having no cost.perCpu and using global?

This PR relates to Thor. I don't want to expand the scope of this PR to other components cost configuration.

github-actions · 2026-02-26T19:02:30Z

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

Jirabot Action Result:
Workflow Transition To: Merge Pending

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

Copilot AI review requested due to automatic review settings November 26, 2025 12:19

shamser mentioned this pull request Nov 26, 2025

HPCC-34746 Allow cost definitions for agent, manager and workers in Thor's yaml #20263

Closed

39 tasks

Copilot started reviewing on behalf of shamser November 26, 2025 12:22 View session

shamser force-pushed the issue34746 branch from 3ae01ea to 5b6a19a Compare November 26, 2025 12:22

Copilot finished reviewing on behalf of shamser November 26, 2025 12:24

Copilot AI reviewed Nov 26, 2025

View reviewed changes

helm/hpcc/values.schema.json Outdated Show resolved Hide resolved

shamser force-pushed the issue34746 branch from 5b6a19a to c387757 Compare November 26, 2025 16:35

shamser requested a review from Copilot November 26, 2025 16:35

Copilot started reviewing on behalf of shamser November 26, 2025 16:48 View session

Copilot AI reviewed Nov 26, 2025

View reviewed changes

Copilot finished reviewing on behalf of shamser November 26, 2025 16:53

shamser requested review from Copilot, ghalliday and jakesmith November 26, 2025 16:56

Copilot started reviewing on behalf of shamser November 26, 2025 17:05 View session

Copilot finished reviewing on behalf of shamser November 26, 2025 17:08

Copilot AI reviewed Nov 26, 2025

View reviewed changes

helm/hpcc/templates/_warnings.tpl Outdated Show resolved Hide resolved

helm/hpcc/templates/thor.yaml Show resolved Hide resolved

helm/hpcc/values.schema.json Outdated Show resolved Hide resolved

helm/hpcc/templates/thor.yaml Outdated Show resolved Hide resolved

shamser force-pushed the issue34746 branch 2 times, most recently from c742cdc to 67540ec Compare November 27, 2025 10:11

shamser requested a review from Copilot November 27, 2025 10:11

Copilot started reviewing on behalf of shamser November 27, 2025 10:12 View session

Copilot finished reviewing on behalf of shamser November 27, 2025 10:16

Copilot AI reviewed Nov 27, 2025

View reviewed changes

common/workunit/workunit.cpp Outdated Show resolved Hide resolved

helm/hpcc/templates/thor.yaml Show resolved Hide resolved

common/workunit/workunit.cpp Outdated Show resolved Hide resolved

testing/helm/tests/thor-subcomponent-inherit.yaml Outdated Show resolved Hide resolved

shamser force-pushed the issue34746 branch from 67540ec to fc77e4d Compare November 27, 2025 10:49

ghalliday requested a review from Copilot December 2, 2025 09:46

ghalliday requested changes Dec 2, 2025

View reviewed changes

Copilot AI reviewed Dec 2, 2025

View reviewed changes

Copilot started reviewing on behalf of ghalliday December 2, 2025 11:48 View session

Copilot finished reviewing on behalf of ghalliday December 2, 2025 11:51

jakesmith reviewed Dec 18, 2025

View reviewed changes

shamser requested a review from ghalliday February 12, 2026 15:13

ghalliday reviewed Feb 13, 2026

View reviewed changes

shamser requested a review from ghalliday February 16, 2026 16:30

shamser force-pushed the issue34746 branch from 740ff9b to 2853d73 Compare February 20, 2026 10:08

shamser changed the base branch from candidate-9.14.x to candidate-10.0.x February 20, 2026 10:08

shamser requested a review from Copilot February 20, 2026 10:09

Copilot AI reviewed Feb 20, 2026

View reviewed changes

Use resources if available for non-containerized

516de1a

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

shamser requested a review from jakesmith February 20, 2026 10:25

Apply suggestions from code review

49d99a7

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot started reviewing on behalf of shamser February 20, 2026 11:06 View session

shamser added 2 commits February 20, 2026 12:39

Fall back to global/component cost values if subcomponent cost values…

e277c35

… not available Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

Fix spelling for test yaml file name

7ead639

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

jakesmith reviewed Feb 20, 2026

View reviewed changes

ghalliday closed this Feb 26, 2026

ghalliday reopened this Feb 26, 2026

Change following review

5972825

Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexisrisk.com>

		@@ -0,0 +1,27 @@
		# Test inheritance from global.cost to subcomponents without explicit override

Conversation

shamser commented Nov 26, 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 Nov 26, 2025

Uh oh!

shamser commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamser Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

shamser commented Feb 12, 2026

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamser commented Feb 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

shamser commented Nov 26, 2025 •

edited

Loading

shamser commented Nov 26, 2025 •

edited

Loading

shamser Feb 12, 2026 •

edited

Loading