Repository navigation
gprecoverseg: fix the defects the Behave suite exposes - #2183
Open
MisterRaindrop wants to merge 4 commits into
Open
MisterRaindrop wants to merge 4 commits into
MisterRaindrop wants to merge 4 commits into
Conversation
Defects the Behave suite exposes in gprecoverseg:
* gprecoverseg: point a recovered segment's conf at its own port
pg_basebackup and pg_rewind both copy the source segment's
postgresql.conf, port setting included, so a mirror recovered by
gprecoverseg came back configured for its primary's port. On a
multi-host cluster the wrong port is merely wrong; on a single host it
collides with the primary that is still running there.
Rewrite the port before starting the segment, on both the full and the
incremental path, using the existing ModifyConfSetting command -- its
perl expression is anchored at the start of the line, so it touches the
port setting and nothing else. Greenplum does the same in the same two
places. The gpsegrecovery unit tests mock the rewrite, assert it, and
expect the line it logs; before, they ran perl on a postgresql.conf
that does not exist.
Fixes the eight gprecoverseg Behave scenarios that assert
"port value in postgresql.conf of <host> is incorrect".
* gprecoverseg: keep the progress files a recovery writes
Each recovering segment writes pg_basebackup or pg_rewind output to
gpAdminLogs/<tool>.<timestamp>.dbid<N>.out, and gprecoverseg deleted that
file as soon as the segment came back successfully. Deleting it on
success is backwards: those files are what gpstate reads to report
recovery progress, and what an operator reads afterwards to see what a
recovery actually did.
Remove stale progress files for the segments about to be recovered
before starting, and leave the run's own files in place, the way
Greenplum does. Clearing beforehand is also what makes the file a
reliable record of the current run rather than an append of every run
that came before.
The stale files are matched by name ("*dbid<N>.out"), so the remove
command becomes Greenplum's "find <log dir> -name <pattern> -delete".
The old "rm -f" form would have quoted the pattern and run it in the
remote user's home directory, removing nothing. Its unit tests now
assert the single recovery call and the find commands.
Fixes the Behave scenarios asserting gpAdminLogs holds "pg_basebackup*"
and "pg_rewind*" files after a recovery.
* gprecoverseg: reject -F combined with -r or -p
A full resynchronisation rebuilds a mirror from its primary in place.
That is not what -r asks for -- rebalance returns segments to their
preferred roles -- nor what -p asks for, which is to rebuild onto a
different host. gprecoverseg accepted both combinations and silently
did something other than what was asked.
Reject them, with the messages Greenplum uses.
* gprecoverseg: report the failure and the progress files it leaves
Two log lines Greenplum emits around recover_mirrors() were dropped:
- the [ERROR] 'gprecoverseg failed. Please check the output for more
details.' that follows a failed recovery. Without it a failed run
ends on the per-segment errors alone, and callers that scrape the
output -- gpmovemirrors among them -- cannot tell a failure from a
partial success.
- the notice that the pg_basebackup/pg_rewind progress files this run
wrote are left behind and a later gprecoverseg may remove them.
Asserted by gpmovemirrors.feature:136 and gprecoverseg.feature:58.
* gprecoverseg: reject -o combined with -i or -r
-o writes out a sample recovery configuration describing what would be
recovered and recovers nothing. -i names a configuration to recover from
and -r rebalances back to preferred roles, so either one together with -o
is a contradiction: the user asked for a file and for a recovery at the
same time, and gprecoverseg quietly honours one of them.
Upstream rejects both combinations; the checks live in a method whose
other validations are for options this tree does not have, so take just
these two and put them beside the "Only one of -i, -p, and -r" check they
belong with. They run before the cluster is contacted, so the error is
immediate.
gprecoverseg.feature has been asserting this and failing.
* gprecoverseg: say why a segment failed to recover, not just where
When a recovery fails, the summary names the segment and the log file and
stops there:
Failed to recover the following segments. You must run gprecoverseg -F
for all incremental failures
hostname: sdw1; port: 7000; logfile: ~/gpAdminLogs/pg_rewind...out;
recoverytype: incremental
so the one thing the reader wants -- what went wrong -- is the one thing
they have to go and find. Upstream ends that line with "; error: {}" and
fills it by reading the log back, and the printer for setup-recovery
failures in this same file already does the equivalent. Only this one
lost it.
Bring over get_error_from_logfile, which takes the last ERROR, PANIC or
FATAL line from the segment's log. It deliberately skips the line
pg_rewind writes about itself after a failure, which is never the reason
for it; upstream skips PostgreSQL 12's wording, and this tree needs its
own, because pg_rewind.c has since been split into
pg_log_error("postgres single-user mode in target cluster failed");
pg_log_error_detail("Command was: %s", ...);
With the self-referential line out of the way what is left is the
backend's own complaint, which is what a reader needs: for a mirror whose
pg_wal was removed, 'required WAL directory "pg_wal" does not exist'.
The behave step that checks this summary matches up to "recoverytype: X"
with a regex search, so the added field does not disturb it.
Split out of apache#1639 (originally a3bfe47).
gpstate -e decides a recovery is in progress by asking whether
$COORDINATOR_DATA_DIRECTORY/gprecoverseg.lock exists. gprecoverseg
removes that directory when it exits normally, but a process killed with
SIGKILL never gets the chance, so from then on every gpstate -e reports
Segments in recovery
for a recovery that is not happening, and exits 1 while doing so.
Upstream asks a different question: it reads the PID out of the lock
directory and checks whether that process is still alive. Bring over
is_gprecoverseg_running and use it, with the two unit tests that cover
the pid-is-alive and no-pidfile cases.
Found by gprecoverseg.feature:735 "SIGKILL on gprecoverseg should not
display progress in gpstate -e" on a five-node cluster, where it was the
first of 38 failures -- the other 37 were scenarios that inherited the
half-recovered cluster it left behind.
The get_coordinatordatadir import existed only for the directory check
and is now unused, as it is upstream.
Split out of apache#1639 (originally 879b563).
There must be one writer per target segment. If a pg_basebackup or a pg_rewind is already streaming into a segment and gprecoverseg starts a second one, the two races leave the segment inconsistent. Greenplum guards against this by asking gp_stat_replication which segments are already sourcing a pg_basebackup, and pg_stat_activity whether a pg_rewind is connected, then skipping those and saying so. Cloudberry carried over neither check, so a second recovery would just start. Port both. gpmovemirrors needs no change of its own: it moves mirrors by shelling out to gprecoverseg, which is why its three scenarios assert on what gprecoverseg printed. This lets seven scenarios run instead of being skipped -- four in gprecoverseg.feature and three in gpmovemirrors.feature, all of the form "gives warning if pg_basebackup already running for ... segments". cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4, gpMgmt/bin/gppylib/programs/clsRecoverSegment_triples.py. One deliberate departure from upstream: Greenplum skips a third case, a source segment already in backup mode, which only arises from its differential recovery. Cloudberry has no differential recovery, so that arm and its pg_is_in_backup() dependency are left out. Split out of apache#1639 (originally 7056e02).
A segment has both a hostname and an address, and they are not always
the same -- the address is whichever interface the segment listens on,
and a host with several interfaces has several of them. gprecoverseg's
-i config file only understood "address|port|datadir", so on such a
cluster it had to guess the hostname from the address, and a recovery
to a new host could register the segment under the wrong one.
Greenplum lets the line carry the hostname explicitly, as
"hostname|address|port|datadir", and when it does:
- the hostname is cross-checked against the address, by resolving
both and requiring a common IP, so a typo is caught before anything
is recovered rather than after;
- the failed segment is matched on hostname as well, so a line
naming the wrong host no longer silently recovers the right one;
- the error naming an unfound segment quotes all four fields.
Port that. Both groups on a line must use the same number of fields,
which is what the "expected equal parts" error says.
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/programs/clsRecoverSegment_triples.py
gpMgmt/bin/gppylib/utils.py
This lets nine scenarios run instead of being skipped, the
"populates hostname and address from the config file correctly" group
and the two that assert the cross-check warning.
Two deliberate departures from upstream. Greenplum also accepts a fifth
field holding a per-line recovery type (I/D/F), which exists for its
differential recovery; Cloudberry has no differential recovery and no
recovery-type field, so only the 3- and 4-part forms are accepted and
the error message says so. And validateHostnameAddress() calls
socket.gethostbyaddr() rather than importing the bare name, because
gppylib/utils.py already imports the socket module.
One further departure is a fix rather than a trim. Upstream resolves
both names with gethostbyaddr() and requires their address lists to
intersect. gethostbyaddr() answers for a single address family, so on a
dual-stack host "localhost" comes back as ::1 while an address naming
that same machine comes back as 127.0.0.1; the lists do not intersect
and the check reports two different hosts. The consequence is not a
failure but a silence -- the segment is skipped and gprecoverseg still
exits 0, having recovered nothing. Resolve through getaddrinfo() instead
and take every address of each name, so the comparison no longer depends
on which family the resolver happens to answer with first. Observed on
the box this was tested on:
localhost -> 127.0.0.1, ::1
cdw -> 127.0.0.1
which upstream's check calls a mismatch.
Greenplum also reworked how an unreachable failover host is detected
while it was here, probing the hosts in parallel through a new
_get_unreachable_failover_hosts() and threading paralleldegree and
outputConfigFile down for it. That is a separate concern from reading a
hostname out of the config file, and Cloudberry already has its own
unreachable handling that the suite exercises, so this keeps that and
leaves the rework alone.
The -o sample config file still writes three fields, as upstream does.
Split out of apache#1639 (originally 0aa2666).
This was referenced Oct 24, 2026
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the gprecoverseg defects that the gpMgmt Behave suite (#2170) exposes, and brings back the two Greenplum guards whose absence makes gprecoverseg and gpstate misreport recoveries. Four commits; each commit message gives the full reasoning.
postgresql.confbefore it starts. pg_basebackup and pg_rewind copy the primary's.-Fcombined with-ror-p, and-ocombined with-ior-r, are rejected.gpstate -etreated a leftovergprecoverseg.lockas a recovery in progress, so after a SIGKILL it reported "Segments in recovery" forever. It now checks whether the PID in the lock is alive, as Greenplum does.gp_stat_replicationandpg_stat_activityis ported.-irecovery config file. Greenplum'shostname|address|port|datadirform is ported, with the hostname cross-checked against the address. Unlike upstream, the check resolves throughgetaddrinfo(), because upstream'sgethostbyaddr()comparison callslocalhostand127.0.0.1two different hosts on a dual-stack box.Split out of #1639 (originally a3bfe47, 879b563, 7056e02 and 0aa2666, by @tuhaihe).
Changes from #1639, found by running the unit tests, which #1639 did not run:
remove_existing_progress_files()came over from Greenplum, but Greenplum's matching change to_get_remove_cmd()did not. The pattern went intorm -f '*dbid5.out', quoted, and ran in the remote user's home directory, so nothing was removed. The remove command is now Greenplum'sfind <log dir> -name <pattern> -delete. I checked this on a cluster: I put an oldpg_basebackup.20000101_000000.dbid5.outingpAdminLogs, failed that mirror, and rangprecoverseg -aF. With CI: add gpMgmt Behave test #1639's code both the old file and the new one were there afterwards; with this PR only the new one is.test_unit_buildMirrorSegmentstests still asserted the old delete-on-success call. They now assert the single recovery call, and a test forremove_existing_progress_files()is added.test_unit_gpsegrecoverytests failed on the line the port rewrite logs. The rewrite was not mocked, so those tests and the workflow tests ran a realperl -ion apostgresql.confthat does not exist; perl only warns about that, so nothing caught it. The tests now mockModifyConfSetting, assert the port it writes, and expect the log line.test_is_gprecoverseg_running_succeedspassed a bareMagicMocktoos.path.join, which themockversion inrequirements-dev.txtcannot turn into a path. It now gives the mock a path and asserts the PID file it opens.Why one PR for four fixes: #1639 carried 19 fixes. Opening one PR per fix would leave a long queue of tiny PRs for reviewers, so the remaining ones are grouped by area into five PRs (this one, #2178, #2180, #2182 (gpcheckcat) and #2184 (gprecoverseg Greenplum ports), which builds on this one). The commits are kept separate on purpose, so please merge this with "Rebase and merge" rather than squash.
Test Plan
Rocky 9 build image, built from #2170's tree plus #2177, #2178 and #2180 (which the demo cluster and gpstop need), with a demo cluster created the way the Behave workflow creates it.
@needs_fixtags that point at these commits, including the ones on the scenarios that commits 3 and 4 make runnable.Run A is without this PR; run B is with it installed into
$GPHOME.gpmovemirrors.feature(--tags ~@concourse_cluster --tags ~@extended)gpaddmirrors.feature(--tags ~@concourse_cluster)gprecoverseg.feature(--tags ~@concourse_cluster --tags ~@extended --tags ~@differential --tags ~@not_implemented)Without this PR, most failures come from the port defect: a recovered mirror keeps its primary's port in
postgresql.conf.port value in postgresql.conf ... is incorrect.gprecoverseg.featureas a whole.all segments are not currently running).The scenarios skipped in B are the multi-host (
@concourse_cluster),@extendedand@differentialones, which CI skips too, plus the five that #2184 makes runnable.Unit tests (
python3 -m unittest, withPYTHONPATHset the waymake -C gpMgmt/bin unitdevelsets it):test_unit_buildMirrorSegments,test_unit_gpsegrecoveryandtest_unit_gppass. Across the wholegppylibsuite (690 tests without this PR, 693 with it), the same tests fail with and without this PR. One of them covers code this PR touches:test_gprecoverseg_with_mirrorlesserrors in both runs because its mockgpArrayhas nostandbyCoordinator. I have not looked into the rest.#2170 tags the scenarios these commits fix or make runnable
@needs_fix, each pointing at its #1639 commit. Whichever PR lands second removes those tags.Type of Change
Checklist