Skip to content

Conversation

Copy link

Copilot AI commented Oct 14, 2025

  • Analyze the problem: Thor manager and workers load config without monitoring (monitor=false)
  • Understand current flow: Manager serializes entire config to workers at RegisterSelf time
  • Identify issue: Workers would lose manager settings if config auto-reloads
  • Create separate IPropertyTree for manager-specific settings to send to workers
  • Modify master to serialize additional settings separately (containerized only)
  • Modify slave to receive and store additional settings separately (containerized only)
  • Install config update hooks in both master and worker to re-merge settings on config refresh
  • Enable config monitoring (monitor=true) in containerized Thor for both manager and workers
  • Implementation complete - optimized based on code review feedback

Code Review Changes (latest commit)

1. Optimized additional settings - Only include settings truly SET by manager:

  • Removed: @channelsPerWorker, logging/@thorworkerdetail, @name, @nodeGroup (already in config)
  • Removed: managerMemory (not used by workers)
  • Kept: @masterBuildTag, @masterTotalMem, @thorPath, @query_so_dir, @dllsToSlaves, @thorTempDirectory, workerMemory

2. Replaced #ifdef with isContainerized() - More flexible runtime check

3. Moved in-method statics to file scope - Better code organization:

  • Master: managerAdditionalSettings, managerAdditionalSettingsCrit, managerConfigHook
  • Worker: workerStoredManagerSettings, workerManagerSettingsCrit, workerConfigHook
Original prompt

Code context: thorlcr, Containerized only.
In both Thormanager and workers, both load the deployed configuratoin (both without monitoring at the moment).

Spotting 'soft' changes that Thor could use without restarting.
In other components monitoring is used, and the confiig is auto reloaded.
Thor could do the same, and it makes as much sense for it to do so as it does elsewhere.
There is an issue that these "soft" changes (e.g. logging level) could change mid-job, but that is also true in every other component.

The complication for Thor here, is that it can't just allow the config to auto reload via loadConfiguration monitor=true, because then the workers would lose settings that the manager had serialized to them (at RegisterSelf time).
However, it doesn't need the whole of the manager config serialized each time (it does in Bare Metal but not in Containerized). In containerized, the workers already have the identical config to load from, but they need the additional settings from the manager.
The manager is currently using the config (globals) as a conduit for additional settings, but this is for slave registration time/at startup only.
So what it really should be doing (in containerized) is sending these addtional setting in a mergable form separetly. Then the worker can keep them, merge them in, and both Thor manager and Thor worker can install a call back to merge these settings in every time the config is refreshed.

Work on reorganizing these additional settings into a separate IPT, that is merged into globals, and that is serialized to the workers a RegisterSelf time, and re-merged when the manager or worker configs are monitored and refreshed.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 4 commits October 14, 2025 12:35
Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
Copilot AI changed the title [WIP] Add soft configuration reload for Thormanager and workers Enable config monitoring for Thor in containerized deployments Oct 14, 2025
Copilot AI requested a review from jakesmith October 14, 2025 12:47
Copy link
Owner

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - structure looks broadly okay, but:

  1. you should be able to tell that a lot of the settings that you have added to managerAdditionalSettings, e.g. channelsPerWorker, logging/@thorworkerdetail, are coming direct from the configuration, without being overridden, and therefore there is no point in storing and sending them.
    Also some settings are used by workers like "managerMemory"
  2. prefer isContainerized() in code rather than #ifdef _CONTAINERIZED
  3. prefer out of method static's than in-method static objects.

Copy link
Author

Copilot AI commented Oct 14, 2025

@copilot - structure looks broadly okay, but:

  1. you should be able to tell that a lot of the settings that you have added to managerAdditionalSettings, e.g. channelsPerWorker, logging/@thorworkerdeta...

Addressed all three points in commit 6c97653:

  1. Removed settings that come directly from config without being overridden (@channelsPerWorker, logging/@thorworkerdetail, @name, @nodeGroup). Also removed managerMemory which workers don't need.

  2. Replaced all #ifdef _CONTAINERIZED with isContainerized() runtime checks.

  3. Moved all in-method static objects to file scope (both manager and worker).

…), move statics

Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants