HPCC-33885 Ensure persistent components do not restart, but pick up logging config changes #19783

streeterd · 2025-04-24T07:41:59Z

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-04-24T07:42:20Z

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

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

Copilot

Pull Request Overview

This PR addresses HPCC-33885 by enhancing the logging configuration to allow persistent components to update their logging configuration on the fly without restarting. Key changes include:

Adding a new pure virtual method queryFormatType() to log handler classes in jlog.ipp.
Introducing queryMaxLinesToKeep() and setMaxLinesToKeep() in the ILogMsgHandler interface in jlog.hpp and their concrete implementations.
Updating the containerized logging setup in jlog.cpp to support on‑the‑fly configuration updates and dynamic postmortem log line adjustments.

Reviewed Changes

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

File	Description
system/jlib/jlog.ipp	Adds queryFormatType() method to log handler classes and adjusts maxLinesToKeep.
system/jlib/jlog.hpp	Extends ILogMsgHandler interface with new max-lines methods and updates function signature.
system/jlib/jlog.cpp	Refactors setupContainerizedLogMsgHandler to support live updates and adjusts postmortem handler logic.

Files not reviewed (1)

helm/hpcc/templates/_helpers.tpl: Language not supported

Copilot · 2025-04-24T11:25:17Z

system/jlib/jlog.cpp

 {
+    // Alow the first config update hook call to update the config as if it was not


Typo in comment: 'Alow' should be corrected to 'Allow'.

Suggested change

// Alow the first config update hook call to update the config as if it was not

// Allow the first config update hook call to update the config as if it was not

jakesmith

@streeterd - haven't looked beyond the unintended release of a change to plugins/CMakeLists.txt ..

Please self-review in github before tagging, to weed out issues like this.

streeterd · 2025-04-24T12:26:10Z

@streeterd - haven't looked beyond the unintended release of a change to plugins/CMakeLists.txt ..

Please self-review in github before tagging, to weed out issues like this.

Removed in 6522db7

jakesmith

@streeterd - discussed offline. I think this needs to be simplified with setupContainerizedLogging split into 2 functions, with a configureHandlers() or similarly named, called from the setup function and installed as a hook.

Such that configureHandlers only changes properties of the already created handlers.
Should still complain if format detection spotted.

A postmortem maxlines change will need to be coded carefully.
In fact, it may be best to do that as a follow on PR - smaller PR's are better PR's.

streeterd · 2025-04-28T13:48:11Z

helm/hpcc/values.schema.json

@@ -1142,7 +1142,7 @@
          "description": "Number of log entries to drop from the log queue when it is full. Disabled if 0, or if queueLength is 0",
          "default": 0
        },
-        "dataFormat": {
+        "format": {


Correct the logging format property name to format.

streeterd · 2025-04-29T08:13:53Z

The failure of the K8s Regression Suite is being resolved under PR: hpcc-systems/hpcc4j#815

jakesmith

@streeterd - tagging you back for now, after pushing commit.

jakesmith · 2025-04-30T12:27:49Z

system/jlib/jlog.ipp

@@ -438,6 +438,7 @@ public:
    int                       flush() { CriticalBlock block(crit); return fflush(handle); }
    bool                      getLogName(StringBuffer &name) const { return false; }
    offset_t                  getLogPosition(StringBuffer &name) const { return 0; }
+    virtual LogHandlerFormat  queryFormatType() const = 0;


virtual is good, but it jars that other existing virtuals methods are not decorated like this. In cases like this, it's worth updating the other methods, rather than sacrifice the new code for consistency.

also, though not strictly needed for this change, I think it would be better to add to interface too (ILogMsgHandler), as it stands out as an anomaly otherwise.

And, given that, these should really all have 'override's on their definitions too

@jakesmith Fixed in 2991719

jakesmith

@streeterd - please see comments.

system/jlib/jdebug.cpp

jakesmith · 2025-05-01T10:12:08Z

system/jlib/jlog.cpp

+        else if (streq(newFormatString, "table"))
+            newFormat = LOGFORMAT_table;
+        else
+            LOG(MCoperatorWarning, "JLog: Invalid log format configuration detected '%s'!", newFormatString);


OWARNLOG more concise

Fixed in 28782b7

jakesmith · 2025-05-01T10:23:40Z

system/jlib/jlog.cpp

@@ -2473,6 +2517,7 @@ void setupContainerizedLogMsgHandler()
            queryLogMsgManager()->addMonitor(thePostMortemHandler, getCategoryLogMsgFilter(MSGAUD_all, MSGCLS_all, TopDetail));
        }


there should be call after this line to updateStdErrLogHandler ..
as it stands, nothing is using the config properties at the outset..until there's an update.

Fixed in 9ad9d66

jakesmith · 2025-05-01T10:23:55Z

system/jlib/jlog.hpp

@@ -912,6 +914,7 @@ extern jlib_decl ILogMsgHandler * queryStderrLogMsgHandler();
 extern jlib_decl ILogMsgHandler * queryPostMortemLogMsgHandler();
 extern jlib_decl bool copyPostMortemLogging(const char *target, bool clear);
 extern jlib_decl void setupContainerizedLogMsgHandler();
+bool configureHandlers(bool rejectOnTheFlyChanges);


missing extern jlib_decl

but actually this is old, should be removed.

Fixed in 9ad9d66

jakesmith · 2025-05-01T10:25:03Z

system/jlib/jlog.ipp

+    bool                      needsPrep() const override { return false; }
+    void                      prep() override {}
+    void                      addToPTree(IPropertyTree * tree) const override;
+    virtual LogHandlerFormat  queryFormatType() const override { return LOGFORMAT_json; };


please decorate other methods with 'virtual' too to be consistent.

Fixed in 9ad9d66

jakesmith · 2025-05-01T10:33:34Z

system/jlib/jlog.ipp

+    int                       flush() override { return 0; }
+    bool                      getLogName(StringBuffer &name) const override { name.append(filename); return true; }
+    offset_t                  getLogPosition(StringBuffer &name) const override { CriticalBlock block(crit); name.append(filename); return fstr->tell(); }
+    LogHandlerFormat          queryFormatType() const override { return LOGFORMAT_undefined; };


please decorate methods with 'virtual' as well.

Fixed in 9ad9d66

jakesmith · 2025-05-01T10:35:32Z

system/jlib/jlog.ipp

+    int                       flush() override { return 0; }
+    bool                      getLogName(StringBuffer &name) const override { return false; }
+    offset_t                  getLogPosition(StringBuffer &logFileName) const override { return 0; }
+    LogHandlerFormat          queryFormatType() const override { return LOGFORMAT_undefined; };


please decorate methods with 'virtual' as well.

Fixed in 9ad9d66

jakesmith · 2025-05-01T10:35:40Z

system/mp/mplog.ipp

+    void                      prep() override {}
+    void                      addToPTree(IPropertyTree * tree) const override;
+    unsigned                  queryMessageFields() const override { return MSGFIELD_all; }
+    void                      setMessageFields(unsigned _fields = MSGFIELD_all) override {}


please decorate methods with 'virtual' as well.

Fixed in 9ad9d66

jakesmith · 2025-05-01T10:35:46Z

system/mp/mplog.ipp

-    offset_t                  getLogPosition(StringBuffer &name) const { return 0; }
+    bool                      getLogName(StringBuffer &name) const override { return false; }
+    offset_t                  getLogPosition(StringBuffer &name) const override { return 0; }
+    LogHandlerFormat          queryFormatType() const override { return LOGFORMAT_undefined; };


please decorate methods with 'virtual' as well.

Fixed in 9ad9d66

jakesmith · 2025-05-01T10:40:05Z

system/jlib/jlog.cpp

-void setupContainerizedLogMsgHandler()
+// returns LOGFORMAT_undefined if format has not changed
+// NB: returns LOGFORMAT_table if no format specified (i.e. this is the default)
+LogHandlerFormat hasLogFormatChanged(const IPropertyTree *logConfig)


hasXX doesn't really make sense anymore, I suggest renaming to 'getConfigHandlerFormat'

Fixed in 9ad9d66

jakesmith

@streeterd - 1 trivial issue remaining (see comment).

jakesmith · 2025-05-01T12:24:34Z

system/jlib/jlog.ipp

+    virtual int               flush() override { CriticalBlock block(crit); return fflush(handle); }
+    virtual bool              getLogName(StringBuffer &name) const override { CriticalBlock block(crit); name.append(filename); return true; }
+    virtual offset_t          getLogPosition(StringBuffer &name) const override { CriticalBlock block(crit); fflush(handle); name.append(filename); return ftell(handle); }
+    virtual LogHandlerFormat  queryFormatType() const override { return LOGFORMAT_undefined; };


I missed one.. RollingFileLogMsgHandler is implicitly LOGFORMAT_table (see its handleMessage impl).

Fixed in e647cc9

jakesmith

@streeterd - a couple of minor comments.

jakesmith · 2025-05-12T08:28:21Z

system/jlib/jlog.cpp

+    if (newFormat != LOGFORMAT_undefined)
+    {
+        OWARNLOG("JLog: Ignoring log format configuration change on the fly, as it is not supported in a containerized environment");
+        return;


the message implies only the format change will be ignored, but the return means all changes will be ignored.
Should either say [all] logging configure update ignore (not just format), or continue to call updateStdErrLogHandler.

I think probably should just ignore format and continue to call udpateStdErrLogHandler.

I meant still issue the warning that ignoring format change, but continue...

Added code back in to warn if log format changes and carry on in 80f0769

jakesmith · 2025-05-12T08:30:00Z

system/jlib/jlog.cpp

+        switch (newFormat)
+        {
+            case LOGFORMAT_xml:
+                theStderrHandler = new HandleLogMsgHandlerXML(stderr, MSGFIELD_STANDARD);


is there a separate existing JIRA to fix the pre-existing leak here?
(Can you link it to this JIRA if so?)

Fixed in 9bf2dbe
New JIRA https://hpccsystems.atlassian.net/browse/HPCC-34101

jakesmith · 2025-05-12T10:03:52Z

system/jlib/jlog.cpp

@@ -2462,7 +2506,7 @@ void setupContainerizedLogMsgHandler()
            UseSysLogForOperatorMessages();

        unsigned postMortemLines = logConfig->getPropInt(capturePostMortemAtt, 0);
-        if (postMortemLines)
+        if (postMortemLines && !thePostMortemHandler)


adding && !thePostMortemHandler suggests, that it's valid to already be set.
If it were already set, then postMortemLines from config would be ignored.

I think best to remove this new check for now, but open a JIRA to support postMortemLines changing, and support it without recreating the handler, i.e. adding a method that sets it the existing handler, and manages increase/decrease/rollover carefully.

Fixed in 9bf2dbe
New JIRA https://hpccsystems.atlassian.net/browse/HPCC-34101

jakesmith

@streeterd - 1 comment re. removal of format change warning

jakesmith · 2025-05-15T15:03:38Z

system/jlib/jlog.cpp

@@ -2442,6 +2442,12 @@ static void loggingSetupUpdate(const IPropertyTree *oldComponentConfiguration, c
        return;
    }

+    LogHandlerFormat newFormat = getConfigHandlerFormat(logConfig);
+    if (newFormat != LOGFORMAT_undefined)
+    {


trivial/style/don't change: normally we avoid { } for single line.

jakesmith

@streeterd - looks good.

Please squash.

streeterd · 2025-05-15T15:35:00Z

@streeterd - looks good.

Please squash.

@jakesmith squashed

jakesmith · 2025-05-16T14:50:47Z

@ghalliday - please sanity check.

ghalliday

Looks good. Only a few trivial comments, not worth changing.

ghalliday · 2025-05-21T10:45:23Z

dali/base/sysinfologger.cpp

+    virtual LogHandlerFormat queryFormatType() const override
+    {
+         return LOGFORMAT_undefined;
+    };


trivial: extraneous semicolon

ghalliday · 2025-05-21T10:46:48Z

system/jlib/jdebug.cpp

+    virtual int flush() override { return 0; }
+    virtual bool getLogName(StringBuffer &name __attribute__((unused))) const override { return false; }
+    virtual offset_t getLogPosition(StringBuffer &logFileName __attribute__((unused))) const override { return 0; };
+    virtual LogHandlerFormat queryFormatType() const override { return LOGFORMAT_undefined; };


similar extra ;, and on line above.

ghalliday · 2025-05-21T10:54:58Z

system/jlib/jlog.hpp

@@ -691,6 +691,7 @@ interface jlib_decl ILogMsgFilter : public IInterface

 typedef enum
 {
+    LOGFORMAT_undefined,


I think this ok, but I had to double check none of these values were serialized - because this changes all the values.

ghalliday · 2025-05-21T10:57:23Z

@streeterd needs rebasing before it can be merged.

…ut restarts Implement ability to change to table log format. Add ability for on-the-fly logging configuration changes. Validate on-the-fly logging configuration changes to prevent log format change. Signed-off-by: Dave Streeter <[email protected]>

streeterd · 2025-05-21T12:16:25Z

@streeterd needs rebasing before it can be merged.

@ghalliday rebased on candidate-9.12.x

github-actions · 2025-05-22T15:57:26Z

Jirabot Action Result:
Added fix version: 9.12.6
Workflow Transition: 'Resolve issue'

streeterd self-assigned this Apr 24, 2025

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch 2 times, most recently from 54c06d8 to a3e6458 Compare April 24, 2025 07:49

streeterd changed the base branch from master to candidate-9.12.x April 24, 2025 07:49

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch 4 times, most recently from e9fa1b7 to dbea252 Compare April 24, 2025 11:23

streeterd requested a review from Copilot April 24, 2025 11:24

streeterd marked this pull request as ready for review April 24, 2025 11:24

Copilot AI reviewed Apr 24, 2025

View reviewed changes

streeterd requested a review from jakesmith April 24, 2025 11:33

jakesmith requested changes Apr 24, 2025

View reviewed changes

streeterd requested a review from jakesmith April 24, 2025 12:26

jakesmith reviewed Apr 24, 2025

View reviewed changes

streeterd marked this pull request as draft April 25, 2025 15:16

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch from 5dc4dc7 to ca893c0 Compare April 28, 2025 09:26

streeterd commented Apr 28, 2025

View reviewed changes

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch 2 times, most recently from 1479d7c to 2452fe9 Compare April 28, 2025 14:23

streeterd marked this pull request as ready for review April 28, 2025 14:29

streeterd requested a review from jakesmith April 28, 2025 14:29

jakesmith reviewed Apr 30, 2025

View reviewed changes

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch from c98a810 to 2991719 Compare May 1, 2025 08:37

streeterd requested a review from jakesmith May 1, 2025 08:37

jakesmith requested changes May 1, 2025

View reviewed changes

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch from d66e5a6 to 9ad9d66 Compare May 1, 2025 11:53

streeterd requested a review from jakesmith May 1, 2025 11:54

jakesmith reviewed May 1, 2025

View reviewed changes

streeterd requested a review from jakesmith May 8, 2025 08:02

jakesmith reviewed May 12, 2025

View reviewed changes

streeterd requested a review from jakesmith May 14, 2025 08:09

jakesmith reviewed May 15, 2025

View reviewed changes

streeterd requested a review from jakesmith May 15, 2025 13:58

jakesmith reviewed May 15, 2025

View reviewed changes

jakesmith approved these changes May 15, 2025

View reviewed changes

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch from 80f0769 to 24201ce Compare May 15, 2025 15:34

jakesmith requested a review from ghalliday May 16, 2025 14:50

ghalliday approved these changes May 21, 2025

View reviewed changes

streeterd force-pushed the HPCC-33885_Log_cfg_chgs_dont_restart_persist_pods2 branch from 24201ce to 4de8fc8 Compare May 21, 2025 12:15

ghalliday merged commit 0c8a2ae into hpcc-systems:candidate-9.12.x May 22, 2025
54 of 55 checks passed

		{
		// Alow the first config update hook call to update the config as if it was not

	// Alow the first config update hook call to update the config as if it was not
	// Allow the first config update hook call to update the config as if it was not

		@@ -2473,6 +2517,7 @@ void setupContainerizedLogMsgHandler()
		queryLogMsgManager()->addMonitor(thePostMortemHandler, getCategoryLogMsgFilter(MSGAUD_all, MSGCLS_all, TopDetail));
		}

HPCC-33885 Ensure persistent components do not restart, but pick up logging config changes #19783

HPCC-33885 Ensure persistent components do not restart, but pick up logging config changes #19783

Uh oh!

Conversation

streeterd commented Apr 24, 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 Apr 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

streeterd commented Apr 24, 2025

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

streeterd commented Apr 29, 2025

Uh oh!

jakesmith left a comment

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!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

streeterd commented Apr 24, 2025 •

edited

Loading

jakesmith May 15, 2025 •

edited

Loading