Repository navigation
Conversation
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
The reported symlink/real-path mismatch is fixed, but I reproduced two cases where the new fallback stops a process from a different installation. The details are in the inline comments.
I suggest retaining Edge's PID-file and installation-identity checks rather than copying the port-based lookup from stop-datanode.sh / stop-confignode.sh: changing the lookup port before stopping can make those scripts miss the old process. Start and stop should use consistent rules for IOTDB_HOME and configuration directories.
There is also a pre-existing lifecycle concern: the PID file is removed before stop_edge_process succeeds. Retaining it on shutdown failure and returning a nonzero status for genuine stop failures would avoid losing track of a running process. This can be handled separately if it is outside this fix.
Verification: I ran the unchanged base (1106d85) and PR (73a96dc) stop scripts on macOS against isolated minimal JVM processes named org.apache.iotdb.edge.EdgeNode, with process inspection and signals restricted to the test-owned processes. In both cross-installation cases, the base script leaves the unrelated process alive and the PR script terminates it. The ordinary symlink-to-real-path case behaves as intended after this change. These were script-level checks, not full IoTDB server tests.
| local home="${command_line#*-DIOTDB_HOME=}" | ||
| home="${home%% -D*}" | ||
| [ -n "$home" ] && [ "$home" != "$command_line" ] || return 1 | ||
| [ "$(resolve_home "$home")" = "${IOTDB_HOME_RESOLVED}" ] |
There was a problem hiding this comment.
[P1] Resolve relative process homes using the process's working directory
start-edge.sh accepts IOTDB_HOME=. and passes it unchanged to the JVM. If an instance was started this way in /srv/edge-a, running /srv/edge-b/sbin/stop-edge.sh from /srv/edge-b with IOTDB_HOME unset resolves the process's . to B. Consequently, find_edge_processes considers A a match and terminates it, even if B has no PID file or running instance. The base script rejects this process; the new fallback accepts it.
Please normalize the home to a physical absolute path at launch, or resolve relative process values against that process's working directory; do not resolve them against the stopping shell's working directory.
There was a problem hiding this comment.
You are right, and thank you for running it. I reproduced it before changing
anything: a synthetic process with cwd in edge-a and -DIOTDB_HOME=. on its
command line, stopped from edge-b -- the base script correctly declines it, my
fallback resolved the . against the stopping shell and terminated it.
I have taken the first of your two suggestions. start-edge.sh now normalises
IOTDB_HOME to a physical absolute path before exporting it, so the value handed
to the JVM identifies the installation on its own:
started through the symlink, before -DIOTDB_HOME=$ROOT/iotdb
started through the symlink, now -DIOTDB_HOME=$ROOT/apache-iotdb-2.0.11-SNAPSHOT-edge-bin
I kept an absolute-path guard in the stop-edge.sh fallback as well, rather than
relying only on the launcher. A process started by an earlier start-edge.sh
during an upgrade can still carry a relative value, and for those the fallback now
declines instead of resolving against our own directory -- the same outcome as the
base script. That leaves resolving against the process's working directory
unnecessary, which I was reluctant to do since it needs lsof or /proc and a
wrong answer there puts us back to stopping the wrong process.
Two things about the normalisation that I would rather state than leave to be
found. It changes the path string that appears in ps, in the console message and
in the logs -- on this machine /tmp becomes /private/tmp -- which is the point
of the change, but anything grepping for the old string is affected. And if the
cd -P cannot run, IOTDB_HOME keeps the value it had, which for the unset case
is still relative; that instance then falls to the literal comparison in
stop-edge.sh, which is today's behaviour. A fallback to a logical cd does not
help here: both still have to reach the directory, so it fails on the same inputs.
Both of your cases are in the regression set now, with the version you reviewed as
a control so they are shown to fail without the change.
| # Resolve symlinks and relative segments so that the same installation reached | ||
| # through a different path still compares equal. | ||
| resolve_home() { | ||
| (cd "$1" 2>/dev/null && pwd -P) || printf '%s' "$1" |
There was a problem hiding this comment.
[P2] Use physical cd semantics before resolving ..
pwd -P only resolves the directory reached by cd; plain cd has already collapsed .. logically. With /x/link pointing to /y/sub, a process home /x/link/../edge accesses /y/edge, but this helper returns /x/edge when that directory exists. Consequently, stopping /x/edge can terminate the /y/edge instance, while stopping /y/edge can miss it. This also reproduces with an absolute IOTDB_HOME, independently of the relative-path issue.
Use cd -P -- "$1" before pwd -P and cover a symlink followed by .. in the regression cases.
There was a problem hiding this comment.
Confirmed, and thank you -- this one reproduces as pure path arithmetic, without
needing a process. With x/link pointing at y/sub and a process home of
x/link/../edge, the physical target is y/edge, and the old helper returned
x/edge:
process home $ROOT/x/link/../edge
physically $ROOT/y/edge
old resolve_home $ROOT/x/edge <- wrong installation
cd -P version $ROOT/y/edge
Fixed in 739dae2 with cd -P -- "$1", and both of your cases are in the
regression set now:
scenario base previous now
-------------------------------------------------- ------ -------- ------
[P1] -DIOTDB_HOME=. in A, stopped from B keep stopped keep
[P2] home x/link/../edge, stopped from x/edge keep stopped keep
[P2] home x/link/../edge, stopped from y/edge - - stopped
symlink home, stopped from the real path keep stopped stopped
The "previous" column is the version you reviewed, so the two cases do fail
without the change rather than passing for an unrelated reason. The four
start/stop environment combinations from the PR description still behave the
same, checked against real Edge processes with 10710 confirmed listening before
each stop.
One limit of the fallback that I should state rather than leave for you to find:
it locates the end of the value by the next -D, which start-edge.sh always
emits after -DIOTDB_HOME. A hand-built command line that ends with
-DIOTDB_HOME=<symlink> is not matched by the fallback. The failure direction is
the safe one -- the extraction keeps the trailing arguments, the resolution fails,
and the process is simply not matched -- so that case behaves as it does today. I have noted it in a comment above the
fallback.
On the PID file being removed before stop_edge_process succeeds: I agree, and I
will open that separately as you suggested, since it changes the exit-code
contract and is worth reviewing on its own.
|
One more data point on the convention, from the Edge tools added in #18552 on the
IOTDB_HOME="${IOTDB_HOME:-$(cd "$(dirname "$0")"/../.. && pwd)}"
IOTDB_HOME="$(cd "$IOTDB_HOME" && pwd -P)"That is: honour an So of the four Edge shell entry points, three take the value they are given and |
25dbf74 to
ff1f1c0
Compare
|
@JackieTien97 A gentle ping on this one. Both cases from your review are handled, with details in the inline replies:
I rebased onto current master today and reran the regression cases against it. Master still leaves the symlink-started process running and the branch stops it. In both of your cases, the other installation's process is left alone. The CI runs from August expired before they were approved, so the new push will need an approval to run. |
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for the update and for the regression tables. I re-ran the scripts at ff1f1c0 against the base (491d9b3) with the same kind of script-level setup as last time: the real start-edge.sh and stop-edge.sh in fresh directories, a minimal JVM whose main class is org.apache.iotdb.edge.EdgeNode and which only sleeps, and process inspection limited to the test-owned processes.
The reported case is fixed, and both cases from my previous review behave correctly. I did find three cases where this version does worse than base (rows 2, 4 and 7 below), so the description's "it can recognise more processes than before, never fewer" no longer holds. Details are in the inline comments. In short:
- Rows 4 and 7 come from
stop-edge.shderivingIOTDB_HOMEdifferently fromstart-edge.sh. Using the same block in both scripts fixes them. - Row 2 is a side effect of resolving symlinks at launch, which was the first option I suggested last time. It needs a decision between documenting "stop before re-pointing the symlink" and recording an absolute path without resolving symlinks.
I also ran both directions on top of this branch, together with the two smaller fixes suggested inline. (a) is stop-edge.sh using the same physical normalisation as start-edge.sh; (b) is both scripts recording an absolute path without resolving symlinks, with the resolved-path comparison kept only in the stop fallback.
| # | Case | Expected | base | PR | (a) | (b) |
|---|---|---|---|---|---|---|
| 1 | #18545: started with IOTDB_HOME=<symlink>, stopped through the real path |
stopped | running | stopped | stopped | stopped |
| 2 | Started through iotdb -> v1, symlink re-pointed to v2, stopped through iotdb/sbin/ (IOTDB_HOME unset, or set to the symlink for both scripts) |
stopped | stopped | running | running | stopped |
| 3 | Previous P1: A runs a process with -DIOTDB_HOME=. from the previous launcher; stop from B with IOTDB_HOME unset |
A running | A running | A running | A running | A running |
| 4 | Same, but the stop in B runs with IOTDB_HOME=. |
A running | A running | A stopped | A running | A running |
| 5 | Previous P2: home x/link/../edge (x/link -> ../y/sub), stop from x/edge |
running | running | running | running | running |
| 6 | Same, stop from y/edge |
stopped | running | stopped | stopped | stopped |
| 7 | Started and stopped through x/link/../edge/sbin/, IOTDB_HOME unset |
stopped | stopped | running | stopped | stopped |
Every stop in this table exited 0, including the ones that left the process running.
Other points:
- Tests. Nothing in CI would catch a regression here.
IoTDBEdgeBasicITonly checks thatstop-edge.shexits 0, which it also does when no process matches, andstopProcessForciblyIfNeededthen kills the process anyway.test-edge-ops.pyreplacesstop-edge.shwith a stub. Could you assert in the IT that the PID fromedge.pidhas exited oncestop-edge.shreturns, and add the cases above totest-edge-ops.py? They do not need a JVM:stop-edge.shrecognises a process such aspython3 -c "import time; time.sleep(600)" -DIOTDB_HOME=<home> -DCONFIGNODE_HOME=<home> org.apache.iotdb.edge.EdgeNode -s. - AGENTS.md. The agent instructions added with the Edge distribution in #18538 (now in
AGENTS.md) say "Stop scripts must match both theEdgeNodemain class and the exactIOTDB_HOME". If the resolved-path fallback stays, please update that sentence in this PR so the rule and the code agree. - Inherited
IOTDB_HOME. Becausestop-edge.shnow honours the environment, anIOTDB_HOMEexported for another installation makes/opt/edge/sbin/stop-edge.shprint "No IoTDB Edge process from is running." and exit 0 while its own instance keeps running. A typo does the same. This is consistent withstart-edge.sh, but a warning when the resolved home is not the script's own installation, and an error when the directory does not exist, would make it visible. - Windows.
stop-edge.batstill setsIOTDB_HOMEunconditionally whilestart-edge.bathonours it, so the same problem remains there. Handling it separately is fine; please open an issue so it is tracked. - The PID-file lifecycle point from my previous review is still open. Thanks for planning a separate PR for it.
These were script-level checks on macOS, not full server tests, and I did not run the Windows scripts.
| # Normalise to a physical absolute path. This value is handed to the JVM as | ||
| # -DIOTDB_HOME and is what stop-edge.sh matches on, so it has to identify the | ||
| # installation on its own, independently of the path used to launch. | ||
| IOTDB_HOME_PHYSICAL="$(cd -P -- "${IOTDB_HOME}" 2>/dev/null && pwd -P)" | ||
| if [ -n "${IOTDB_HOME_PHYSICAL}" ]; then | ||
| IOTDB_HOME="${IOTDB_HOME_PHYSICAL}" | ||
| fi |
There was a problem hiding this comment.
[P1] Resolving symlinks at launch breaks stopping after a symlink is re-pointed
This makes the physical directory, not the launch path, the identity on the command line. With versioned directories behind a stable symlink, which is the layout in #18545, upgrading by re-pointing the symlink and then restarting through it no longer stops the old process:
# edge-v1 and edge-v2 are two unpacked Edge distributions
ln -s edge-v1 iotdb
iotdb/sbin/start-edge.sh # this PR: -DIOTDB_HOME=$PWD/edge-v1 base: -DIOTDB_HOME=$PWD/iotdb
ln -sfn edge-v2 iotdb
iotdb/sbin/stop-edge.sh # this PR: "No IoTDB Edge process from $PWD/iotdb is running.", exit 0edge.pid is in edge-v1, the literal comparison does not match, and the fallback compares edge-v1 with edge-v2. The next start then exits 0 because the ports are occupied, which is the #18545 symptom again. Setting IOTDB_HOME=$PWD/iotdb for both scripts gives the same result. Base records $PWD/iotdb and stops the process. Units generated by daemon-edge.sh pin the physical path, and systemd's default KillMode=control-group cleans up after ExecStop, so this mainly affects manual and non-systemd operation.
This was the first option I suggested last time, and I had not considered this flow. I see two ways forward:
-
Keep it, and document that the stop has to run before the symlink is re-pointed, or through the old physical path.
-
Record an absolute path without resolving symlinks, and keep the resolved-path comparison only in the
stop-edge.shfallback:if [ -z "${IOTDB_HOME}" ]; then IOTDB_HOME="$(CDPATH= cd -- "$(dirname "$0")"/.. && pwd)" fi case "${IOTDB_HOME}" in /*) ;; *) IOTDB_HOME="$(pwd)/${IOTDB_HOME}" ;; esac export IOTDB_HOME
with the same block in
stop-edge.sh. In my runs this keeps IoTDB Edge: stop-edge.sh does not stop its own process when IOTDB_HOME is set, and reports success #18545 and both cases from my previous review fixed, and also stops the process after the symlink is re-pointed. The trade-off is that the default stays logical, as in base, so launching throughx/link/../edge/sbin/still usesx/edge.
I lean towards 2, but either works as long as both scripts use the same block.
| # Normalise to a physical absolute path. This value is handed to the JVM as | ||
| # -DIOTDB_HOME and is what stop-edge.sh matches on, so it has to identify the | ||
| # installation on its own, independently of the path used to launch. | ||
| IOTDB_HOME_PHYSICAL="$(cd -P -- "${IOTDB_HOME}" 2>/dev/null && pwd -P)" |
There was a problem hiding this comment.
[P3] Guard this cd against CDPATH
If CDPATH is exported, cd looks relative names up in it first and prints the directory it chose, so the substitution captures two lines. With CDPATH=/elsewhere and IOTDB_HOME=iotdb, IOTDB_HOME_PHYSICAL is /elsewhere/iotdb twice, separated by a newline, and that becomes the home. With CDPATH=.:... it is ./iotdb followed by the absolute path. Base used a relative IOTDB_HOME from the environment as-is, so this is new for that case; the default branch already had the problem with a plain cd. CDPATH= cd -P -- ... here and in resolve_home, or unset CDPATH at the top of both scripts, avoids it.
| if [ -z "${IOTDB_HOME}" ]; then | ||
| IOTDB_HOME="$(cd "$(dirname "$0")"/.. && pwd)" | ||
| fi |
There was a problem hiding this comment.
[P1] Please derive IOTDB_HOME with the same code as start-edge.sh
The two scripts now compute the home differently, and both differences matter.
-
A relative
IOTDB_HOMEfrom the environment is used as-is in the literal comparison, which bypasses the absolute-path guard in the fallback. Suppose installation A runs a process started by the previousstart-edge.shwithIOTDB_HOME=., the case the guard is meant for, and B is not running:cd /srv/edge-b && IOTDB_HOME=. ./sbin/stop-edge.sh # IoTDB Edge process <PID of A's process> stopped.
B has no
edge.pid, so the script scans, and-DIOTDB_HOME=.matches A's command line. Base ignores the environment here and leaves A alone. -
The default still uses a logical
cd/pwd, whilestart-edge.shnow usescd -P/pwd -P. Withx/link -> ../y/sub, starting and stopping throughx/link/../edge/sbin/records.../y/edgeat start but computes.../x/edgehere. The stop printsNo IoTDB Edge process from .../x/edge is running.and exits 0, and ifx/edgehas its own instance, that one is stopped instead. This is last round's P2 again, on the script's own path. Base computesx/edgein both scripts and stops the process.
If the launcher keeps the physical normalisation, this block fixes both, and IOTDB_HOME_RESOLVED then equals IOTDB_HOME:
if [ -z "${IOTDB_HOME}" ]; then
IOTDB_HOME="$(dirname "$0")/.."
fi
IOTDB_HOME_PHYSICAL="$(CDPATH= cd -P -- "${IOTDB_HOME}" 2>/dev/null && pwd -P)"
if [ -z "${IOTDB_HOME_PHYSICAL}" ]; then
echo "Cannot resolve IOTDB_HOME ${IOTDB_HOME}."
exit 1
fi
IOTDB_HOME="${IOTDB_HOME_PHYSICAL}"If the launcher switches to option 2 from my start-edge.sh comment, use that block here instead. Either way, one function in conf/iotdb-common.sh called by both scripts would stop them from drifting apart again.
| # process is simply not matched -- never matched to the wrong installation. | ||
| local home="${command_line#*-DIOTDB_HOME=}" | ||
| home="${home%% -D*}" | ||
| [ -n "$home" ] && [ "$home" != "$command_line" ] || return 1 |
There was a problem hiding this comment.
[P3] This check cannot detect a missing -DIOTDB_HOME=
It runs after the %% -D* truncation on the previous line. When the command line has no -DIOTDB_HOME=, home is the command line cut at its first -D, which differs from command_line whenever any -D option is present. For /usr/bin/java -Dfoo=1 ... org.apache.iotdb.edge.EdgeNode, only the failing cd prevents a match, and a process whose argv[0] is the installation directory is matched without any -DIOTDB_HOME. That is unlikely in practice, but it contradicts the comment above. Checking for the option first makes the intent explicit:
case "$command_line" in
*-DIOTDB_HOME=*) ;;
*) return 1 ;;
esac
local home="${command_line#*-DIOTDB_HOME=}"
home="${home%% -D*}"
[ -n "$home" ] || return 1start-edge.sh honours an IOTDB_HOME from the environment and passes that value to the JVM as -DIOTDB_HOME=..., while stop-edge.sh recomputed IOTDB_HOME from its own location and matched the command line against that string. When the two differ -- for example a versioned install reached through a symlink -- the stop declined to act on its own process, exited 0 and removed the PID file, so a service manager saw a successful stop while the process kept running. stop-edge.sh now honours IOTDB_HOME the same way start-edge.sh does, and falls back to comparing resolved paths when the literal match does not hit. The original literal comparison is tried first, so any command line that matched before still matches.
Two problems in the previous version, both of which could stop a process belonging to a different installation. The helper used a plain cd before pwd -P. Because cd collapses ".." logically, a process home such as <symlink>/../edge resolved against the symlink's parent rather than its target, so stopping one installation could terminate another. It now uses cd -P. The fallback also resolved relative process homes, such as the "." that start-edge.sh accepts and passes through unchanged, against the working directory of the stopping shell rather than that of the started process. A process started with IOTDB_HOME=. in one directory could therefore be matched and stopped from an unrelated installation. The fallback now only considers absolute values and leaves relative ones to the literal comparison, which matches the previous behaviour for them.
Taking the first of the two options suggested in review. The value start-edge.sh hands to the JVM as -DIOTDB_HOME is what stop-edge.sh matches on, so it should identify the installation on its own rather than depend on the path used to launch. Normalising it at that point removes the relative and symlinked forms at the source instead of resolving them later. The absolute-path guard in stop-edge.sh is kept as well, so a process started by an earlier start-edge.sh, whose command line may still carry a relative value, is left alone rather than matched against the stopping shell's directory.
cd succeeds on an empty argument and returns the caller's working directory, so dropping the emptiness check would reopen the same class of mismatch as a relative value. Comment only.
start-edge.sh and stop-edge.sh computed IOTDB_HOME differently, so a stop could miss its own process or match another installation's. Both now call initEdgeHome from conf/iotdb-common.sh, which only makes the value absolute and does not resolve symlinks, so stopping through a symlink still works after it is re-pointed to a new version. The resolved-path comparison stays as a fallback in stop-edge.sh only. initEdgeHome fails when IOTDB_HOME cannot be entered and warns when it is not the installation the script belongs to. Its cd calls ignore CDPATH. The fallback now requires -DIOTDB_HOME= on the command line. test-edge-ops.py runs the real stop-edge.sh against stand-in processes for these cases, and IoTDBEdgeBasicIT checks that the process recorded in edge.pid has exited once stop-edge.sh returns. Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>
ff1f1c0 to
115d826
Compare
|
Thanks for re-running this and for the table. I went with option 2.
I reran your seven rows on macOS with the real
The base and ff1f1c0 columns match yours. In rows 3 and 4, A's process was started by the base Tests:
I opened #19005 for |
Description
Fixes #18545.
start-edge.shhonours anIOTDB_HOMEtaken from the environment and passes thatvalue to the JVM as
-DIOTDB_HOME=....stop-edge.shrecomputedIOTDB_HOMEfrom its own location and matched the process command line against that string,
so when the two differed it declined to act on its own process, exited 0, and
removed the PID file. A service manager saw a successful stop while the process
kept running; the following
ExecStartthen found the ports occupied and alsoexited 0, so a configuration change silently never took effect.
The change
start-edge.shandstop-edge.shnow deriveIOTDB_HOMEwith one sharedfunction,
initEdgeHomeinconf/iotdb-common.sh:IOTDB_HOMEfrom the environment is used by both scripts; otherwise it isthe parent of the script's directory;
not resolved, so an installation started through a symlink keeps that identity
after the symlink is re-pointed to another version;
when it is not the installation the script belongs to.
stop-edge.shfirst compares the recorded-DIOTDB_HOMEliterally, as before.If that does not match and the recorded value is an absolute path, it compares
the two after resolving symlinks. A process started with
IOTDB_HOME=<symlink>is then found when stopping through the real path, and the other way round.
Behavioural notes
stop-edge.shnow honoursIOTDB_HOMEfrom the environment. If it is exportedfor another installation, the stop looks for that installation's process and
prints a warning, where it previously stopped its own.
IOTDB_HOMEis made absolute before it reaches the JVM, sopsshows the absolute path in that case.
Verification
.github/scripts/test-edge-ops.pyhas a POSIX-onlyEdgeStopTestthat runs thereal
stop-edge.shagainst stand-in processes for the cases below, plus a processwithout
-DIOTDB_HOME,CDPATH, the warning and a missing directory.IoTDBEdgeBasicITchecks that the process fromedge.pidhas exited afterstop-edge.shreturns.The same cases with the real
start-edge.shandstop-edge.shand a JVM whosemain class is
org.apache.iotdb.edge.EdgeNodeand which only sleeps, on macOS;each cell is the state of the started process after the stop:
Row 2 was run with
IOTDB_HOMEunset and with it set to the symlink for bothscripts. The first version of this change was also run against the packaged Edge
archive with the restart sequence from #18545.
Not changed
stop-edge.bathas the same asymmetry on Windows; tracked in IoTDB Edge: stop-edge.bat ignores IOTDB_HOME, while start-edge.bat honours it #19005.