-
Notifications
You must be signed in to change notification settings - Fork 0
Implement logical Thor cluster feature for simplified multi-instance configuration #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: jakesmith <[email protected]>
…configuration Co-authored-by: jakesmith <[email protected]>
jakesmith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - see comments
| instanceTemplate: "{name}-az{instance}" | ||
| numWorkers: 2 | ||
| maxJobs: 6 | ||
| maxGraphs: 3 No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - I do not see what purpose this example servces over the one in comprehensive-thor-cluster.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right. I've removed the redundant logical-thor-cluster.yaml test file since comprehensive-thor-cluster.yaml provides better coverage with more configuration options and comments explaining the use case. (ac87eea)
| instanceTemplate: "{name}-node{instance}" | ||
| maxJobs: 8 | ||
| maxGraphs: 4 | ||
| # Generates: thor-batch-node1, thor-batch-node2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - apart from 'thor-batch' vs 'thor-prod' what is this "Custom Instance Naming" demonstrating? It doesn't seem to explore anything new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've updated the "Custom Instance Naming" example to demonstrate a more meaningful pattern using "thor-region-{instance}-cluster" which better shows how the template can be used for specific naming conventions needed for external systems or monitoring tools integration. (ac87eea)
helm/hpcc/templates/_helpers.tpl
Outdated
| {{- $maxGraphsPerInstance := div $thorConfig.maxGraphs $instances -}} | ||
| {{- if or (ne (mul $maxJobsPerInstance $instances | int) ($thorConfig.maxJobs | int)) (ne (mul $maxGraphsPerInstance $instances | int) ($thorConfig.maxGraphs | int)) -}} | ||
| {{- $_ := fail (printf "Thor cluster '%s': maxJobs (%d) and maxGraphs (%d) must be evenly divisible by instances (%d)" $thorConfig.name $thorConfig.maxJobs $thorConfig.maxGraphs $instances) -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - it is unnecessary to valid the divisibility here in _helpers.tpl, given it will be validated in thor.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I've removed the duplicate validation logic from _helpers.tpl since it's already handled in thor.yaml. The validation now occurs only once in the template processing flow. (ac87eea)
…iminate duplicate validation Co-authored-by: jakesmith <[email protected]>
jakesmith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - retarget this PR to the candidate-9.14.x branch
I cannot directly retarget the PR branch through code changes - that requires updating the base branch in the GitHub PR interface. The
The code changes themselves are compatible with the candidate branch structure. Would you like me to help verify compatibility or make any adjustments for the target branch? |
Summary
Implemented a logical Thor cluster feature that allows defining a single Thor configuration that expands into multiple physical instances.
Recent Changes (addressing code review):
logical-thor-cluster.yamlwas duplicate ofcomprehensive-thor-cluster.yaml_helpers.tplsince it already exists inthor.yamlKey Features:
{name}and{instance}placeholdersExample Usage:
This generates
thorcluster-az1,thorcluster-az2,thorcluster-az3with divided limits, all listening to queuethorcluster.Testing:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.