Skip to content

mirror: add ECF_MIRROR_DISABLE to disable local mirror polling - #349

Closed
morianemo wants to merge 1 commit into
ecmwf:developfrom
morianemo:feature/mirror-kill-switch-step1-clean
Closed

morianemo wants to merge 1 commit into
ecmwf:developfrom
morianemo:feature/mirror-kill-switch-step1-clean

Conversation

@morianemo

Copy link
Copy Markdown
Collaborator

Summary

  • add ECF_MIRROR_DISABLE support to disable local mirror polling
  • add environment constants for mirror kill switches
  • add parser test coverage for disabled mirror behavior

Files

  • libs/core/src/ecflow/core/Environment.hpp
  • libs/node/src/ecflow/node/MirrorAttr.cpp
  • libs/node/test/parser/TestMirrorAttr.cpp

Notes

  • clean PR: excludes anki/, ecflow-2025.md, and .gitignore changes

@marcosbento

Copy link
Copy Markdown
Collaborator

Hi @morianemo,

Thank you for this contribution, and for the interest in giving operators more control over the mirror mechanism.
Being able to switch behaviour off without touching the suite definition is a reasonable thing to want.

Having looked at the implementation, there are a few aspects that make it difficult to merge as it stands:

  • The guard in MirrorAttr::mirror() does not actually stop mirroring. The polling controller is also started by MirrorAttr::reset(), which Node::begin() (and requeue) call unconditionally, so after --begin the background thread keeps polling the remote host. Since mirror() returns early, get_notifications() is never called and the notification vector grows for the lifetime of the server. A correct kill switch needs to act on the controller lifecycle (reset(), reload(), stop_controller()), not only on the polling step.
  • The variable is tested for presence, not value: findParentVariableValue returns true for an empty value as well, so ECF_MIRROR_DISABLE "", "0" or "no" all disable mirroring, contrary to the in-code comment ("defined and non-empty") and to what a user toggling the value would expect.
  • The added test passes without the feature: --remote_host invalid-host --polling 1 resolves fine, so start_controller() succeeds and connection failures only arrive asynchronously, which the test never drains. REMOTE_ERROR is therefore never set, with or without ECF_MIRROR_DISABLE. It is also a runtime test placed in the parser test suite.
  • ECF_MIRROR_DISABLE_QUERIES is declared but not used anywhere, and there is no documentation or release-note entry for a new user-facing ECF_MIRROR_* variable.

More importantly, before going further with any implementation, it would be valuable to step back and discuss the intended behaviour. A node with a mirror attribute is set to UNKNOWN at begin and is never free to run; with the switch set, it simply stays UNKNOWN forever, with no state, no flag and no reason reported. It is not clear whether a "disabled" mirror is meant to freeze the node, make it behave as a regular task, or only pause the remote polling temporarily. Note also that --alter delete mirror / --alter add mirror already allow switching mirroring off and on explicitly, and the -step1 suffix in the branch name suggests further pieces are planned, so seeing the whole design would help a lot.

With that in mind, I am going to close this PR for now. Please do not read this as a rejection of the idea: if you would like to pursue it, opening an issue describing the operational scenario (what needs to be disabled, when, and what the mirrored nodes should do in the meantime) would be a great starting point for the discussion, and from there a design that fits the mirror lifecycle can be worked out.

Thanks again for the effort and for reaching out with the proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants