Issue1196 - Add default.inc entry point in configuration parsing

[andreleblanc11]

This PR adds

• An entry point to parse <component>/default.inc prior to parsing the configuration file.

  • This enables <component>/default.inc to override default.conf and;
  • This enables the configuration file to override what is in default.conf + <component>/default.inc

• I opted with default.inc instead of default.conf because

  1. Any .conf entry gets parsed and treated as a configuration file and;
  2. Adding component parsing in def default_config seemed like more work. default.inc does the same job and doesn't need refactoring of the code to adapt.

• Add unit tests that test out this new entry point. Unit tests were manually ran on my VM + GitHub actions. Both passed.

• Update documentation to reflect changes. Remove <component>.conf entry in documentation as I tested it and it doesn't work. I believe @reidsunderland confirmed this is a v2 specific feature.

[github-actions]

Test Results

379 tests   378 ✅  1m 38s ⏱️
  1 suites    1 💤
  1 files      0 ❌

Results for commit eb57988.

♻️ This comment has been updated with latest results.

[andreleblanc11]

Lots of flow tests are failing.. so that's a bad sign

[andreleblanc11]

The flow tests are failing due to the Sarra C package being missing.. for some test cases. Seems unrelated to this PR.

I ran the static flow test on a U22.04 VM and that passed without problems.

[robjarawan]

Went through the diff and traced the config through both sr_GlobalState and the worker startup path. I think this needs a few changes before it merges, here we goo:

default.inc only gets loaded by sr_GlobalState

The new include is parsed in sr_GlobalState._read_configs(), so sr3 show and declare see it. The process that actually starts a component builds its options through config.one_config() in instance.py, and that path doesn't load <component>/default.inc.

I reproduced this with retry_ttl 1h in subscribe/default.inc: the management-side options showed 3600, but the worker-side options left retry_ttl unset. Basically the config can look right in show and then run with different settings. Could we load this through the same path used by one_config() so management and runtime can't diverge? This one needs fixing before merge.

poll/default.inc loses the component context

The new parse_file(default.inc) call doesn't pass c. That changes how component-specific aliases are resolved. For example, destination in poll/default.inc became sendTo in my test; parsing the same file with "poll" sets pollUrl, which is the expected poll option. broker has the same kind of component-dependent mapping.

Can you pass the component into this parse call and add a test using a component-specific option?

Tests

The current tests prove _read_configs() sees the file, but they don't exercise the worker one_config() path where the first issue shows up. Can you add a runtime-side test as well?

They also write predictable /tmp/<component> paths and remove them afterward. That can race in parallel test runs or overwrite somebody's existing path. tmp_path would keep the test isolated.

Also, watch appears twice in the parameter list, while cpost and cpump aren't covered. If those are intentionally outside the feature, it would help to make that scope explicit; otherwise they should be included.

French docs

The French page still lists <component>.conf in the lookup order even though the paragraph below talks about <component>/default.inc. Can you update that list so both descriptions agree?

Flow tests

The current failed runs show /usr/bin/sr3_cpump installed and reporting a version before the failures. The MQTT failure is in the flakey-broker counts on Ubuntu 22/24, and the AMQP failure is the partitioned-flow check on Ubuntu 24. I don't think the missing-Sarra-C explanation matches those logs.

Can you rerun those flows or compare them against the same runs on development so we know whether they're pre-existing or caused by this branch?

Minor cleanup

git diff --check reports trailing whitespace in the new test, and the new test functions need the usual blank-line cleanup. This user-facing change also needs a CHANGES.rst entry.

[andreleblanc11]

Thanks for that. I changed my entries so that the sarracenia instance as well as the sr command line invocation use both the same method.

From a runtime perspective, the component/default.inc entries now seem to be properly applied at runtime.

For a poll. The retry_ttl entry gets correctly invoked from the config, while the rest of the settings get invoked from the default.inc file.

leblanca@edcm-dirt22-3:~/.config/sr3$ cat poll/default.inc
fileEvents -rmdir,delete
acceptSizeWrong True
logReject False
retry_ttl 10d
leblanca@edcm-dirt22-3:~/.config/sr3$ cat poll/sftp_f62.conf
# created by: sr3 convert poll/sftp_f63.conf
#
# component of the flow test use for QA in development.
#

post_topicPrefix v03.post
post_broker ${MQP}://tsource@${FLOWBROKER}/
post_exchangeSuffix poll

# pick an ip we never have.
vip 127.0.0.99

scheduled_interval 10

# logReject On
fileAgeMax 1000w
housekeeping 60

retry_ttl 1h

permCopy off
pollUrl sftp://${SFTPUSER}@localhost/
path ${TESTDOCROOT}/sent_by_tsource2send

debug on
set sarracenia.moth.amqp.AMQP.logLevel debug
set sarracenia.moth.mqtt.MQTT.logLevel debug


reject .*\.tmp$

leblanca@edcm-dirt22-3:~/.config/sr3$ sr3 start poll/sftp_f62

leblanca@edcm-dirt22-3:~/.cache/sr3/log$ grep -E "(logReject|acceptSizeWrong|retry_ttl|fileEvents|mirror)" poll_sftp_f62_01.log
 'acceptSizeWrong': True,
 'fileEvents': {'mkdir', 'link', 'create', 'modify'},
 'logReject': True,
 'masks': ['reject .*\\.tmp$ into None with mirror:True filename:None'],
 'mirror': True,
 'retry_ttl': 3600.0,

At the same time, Im running a sarra. The default.inc sarra settings aren't overriding the poll options.

leblanca@edcm-dirt22-3:~/.config/sr3$ cat sarra/default.inc
acceptSizeWrong True
mirror False
retry_ttl 1d

leblanca@edcm-dirt22-3:~/.config/sr3$ cat sarra/download_f20.conf
#
#sr3 inflight defaults to None, v2 defaulted to .tmp when post_broker set.
inflight .tmp

statehost on

broker ${MQP}://tfeed@${FLOWBROKER}/
exchange xsarra
topicPrefix v03.post

housekeeping 60


debug
batch 20
# I want the times the files are written, rather than source times.
# MG this does not work with wget... by def. if available local file gets remote times
timeCopy off

set sarracenia.moth.amqp.AMQP.logLevel debug
set sarracenia.moth.mqtt.MQTT.logLevel debug

messageDebugDump on

nodupe_ttl 600

logReject
logEvents +after_accept

accelThreshold 100
#plugin accel_wget

# upstream is posting every file twice.  2nd copy should be rejected.
logReject

# WARNING: DO NOT CHANGE.
# for the rejections to work, need to use a single instance.
instances 1

strip /path/to/something/


# If you put this back in, all the posts will fail.  This is new desired behaviour,
# as per issue #294
#on_post post_long_flow.py
directory ${TESTDOCROOT}

post_broker ${MQP}://tfeed@${FLOWBROKER}/
post_exchange xflow_public
post_baseDir ${TESTDOCROOT}
post_baseUrl http://localhost:8001/

leblanca@edcm-dirt22-3:~/.cache/sr3$ grep -E "(logReject|acceptSizeWrong|retry_ttl|fileEvents|mirror)" edcm-dirt22-3/log/sarra_download_f20_01.log
 'acceptSizeWrong': True,
 'fileEvents': {'rmdir', 'delete', 'mkdir', 'link', 'create', 'modify'},
 'logReject': True,
 'mirror': False,
 'retry_ttl': 86400.0,

[robjarawan]

Thanks, the direct runtime setting and component-specific parsing issues from the first pass are fixed. The French lookup order is corrected and git diff --check is clean.

I reran the focused tests: 16 passed. The full unit suite also passed with 375 passed and 1 skipped.

I found three remaining points in the inline comments. CHANGES.rst is also still missing, and the new test block still has duplicated imports and two missing blank lines.

The full AMQP and MQTT flow workflows haven’t rerun against the current head because those workflows don’t trigger on synchronized commits. The base AMQP workflow also failed, but in different test cases, so it doesn’t establish that the branch failures were pre-existing. I’d still rerun them or provide a matching baseline before approval.

[andreleblanc11]

The github actions that have already failed are either,

• The MQTT tests. Which are known to fail sometimes. Should not be related to this change.
• restart_server check. Is known to fail with 35/37 tests passing. This is a known issue that needs to be addressed, but should be unrelated to these changes.

[robjarawan]

Thanks, I went through the latest updates. So what I did was check the diff, your runtime evidence, and the CI state.

The blockers from my first pass look fixed. Basically you now load default.inc through one shared helper from both one_config() and _read_configs(), so management and runtime cannot diverge the way my retry_ttl repro showed. Passing the component into parse_file fixes the destination/broker alias problem too. French docs agree now and diff --check is clean. Nested includes plus tests are a good addition.

I am holding approval on the items from my second review since they are still open. The tests only cover _read_configs(), so the worker one_config() path still has no test. They also use fixed /tmp paths instead of tmp_path, and the nested test uses a bare mkdir, so a leftover dir or a parallel run breaks them. CHANGES.rst is still missing, plus the duplicated imports and blank lines.

One new thing I found while reading the helper. addComponentDefaultInc uses the global get_user_config_dir() instead of the state's user_config_dir. In _read_configs everything else works under self.user_config_dir, so with an overridden config dir the default.inc comes from the wrong tree while the .conf comes from the right one. Basically the same divergence class as the first finding, one level up. Passing the base dir in or resolving relative to cwd would fix it cheaply.

On the flows, the maintenance runs are green on the PR, which is good. I still want a matching AMQP/MQTT baseline on the current head before approval, since the base failed in different cases and that does not prove pre-existing.

Good progress. Once the one_config test, tmp_path, CHANGES.rst, and the config-dir point are in, I will re-review.