Repository navigation
Conversation
tuhaihe
marked this pull request as draft
March 25, 2026 08:15
tuhaihe
force-pushed
the
add-behave-test-ci
branch
3 times, most recently
from
June 24, 2026 08:00
0e824bd to
c0fd832
Compare
tuhaihe
force-pushed
the
add-behave-test-ci
branch
10 times, most recently
from
September 12, 2026 01:39
56b073c to
0d5e452
Compare
Member
Author
|
Hi @leborchuk the behave test workflow can run successfully, but need more review on the changes. FYI. |
Contributor
|
We could temporary disable failing tests and fix them in the upcoming PRs |
Contributor
|
Also I think what if we will launch it only if management code was changed? And in a case of special tag. For the numerous of commits they not needed |
Member
Author
Sounds good. I am working with Claude to test further in a real multi-node cluster to verify changes. If any updates, will let you know. |
tuhaihe
force-pushed
the
add-behave-test-ci
branch
2 times, most recently
from
October 12, 2026 03:37
361a2e3 to
1c49e4c
Compare
tuhaihe
force-pushed
the
add-behave-test-ci
branch
from
October 19, 2026 05:52
1c49e4c to
4416f22
Compare
tuhaihe
marked this pull request as ready for review
October 19, 2026 05:57
Member
Author
Already one in the latest code. FYI. |
Member
Author
|
Now this PR can be reviewed. These tests were also run on a real cloudberry cluster with 5 nodes, and not just on gpdemo. But most changes have gone beyond my knowledge (they're done by Claude code and DeepSeek). If someone else can take over, that would be best. cc @leborchuk @Alena0704 |
Run the gpMgmt Behave suite on Rocky Linux 9, one job per management command so that a failure in one feature file does not mask or pollute the others. The workflow builds an RPM once and fans out to a per-command matrix, each entry creating its own demo cluster. It is kept separate from the main build/installcheck workflow so that Behave-specific matrix expansion and result parsing can evolve without disturbing the primary CI path. A run costs about 190 minutes across 21 jobs, so a pull request only triggers it when it touches gpAux, gpMgmt, src/bin, this workflow, or system_views.sql, where gp_stat_replication is defined. The push trigger is left unfiltered on purpose: src/backend is too broad to put in the filter, but the utilities sit on top of it, so running after merge is what keeps a regression there from going unnoticed. 'edited' is not among the pull_request types, since it fires on a title or body change. Six of the 23 suites under test/behave/mgmt_utils are not in the matrix. gpcheckperf, gprecoverseg_newhost and gpssh_exkeys are @concourse_cluster throughout, so against a single-host demo cluster the tag filter selects nothing and the job reports success without running a scenario; a green check that tests nothing is worse than no check. cross_subnet needs the coordinator and segments on different subnets. gpexpand and gppkg are the two that could plausibly be added later. The core dump check treats a SIGABRT core as expected only when gpstop logged that it sent the signal. gpstop escalates to SIGABRT when a postmaster will not shut down, which "gpstop succeeds when pg_ctl command fails" exercises deliberately by injecting a sleep into checkpoint, so that core belongs to a scenario that passed. Without that log line a SIGABRT core still fails the job.
The Behave suite had drifted from the one it was forked from: scenarios added upstream for gprecoverseg, gpmovemirrors, gpaddmirrors, gpexpand, gpcheckcat, gpstop and minirepro were missing, several existing ones had diverged, and a number of step definitions carried only one of the @given/@when/@then decorators Greenplum registers, so features using "And" could not match them and Behave reported the steps as undefined even though the implementation was right there. Bring the feature files and their step definitions back in line with github.com/greenplum-db/gpdb-archive, keeping the Cloudberry-specific adjustments where the two products genuinely differ. Differential recovery is the one place they differ outright: Cloudberry has no "gprecoverseg --differential", so those scenarios cannot pass here. Tag them @Differential and filter them out of the gprecoverseg matrix entry, splitting the Examples tables that mix differential with full and incremental so the other variants keep running. The scenarios stay in place, aligned with Greenplum, ready to be enabled with the feature.
Cloudberry does not implement every feature the Greenplum suite covers,
and some scenarios encode a cluster topology we do not run. Tag or drop
them so the matrix selects only what this product can actually satisfy.
Each exclusion and its reason:
Note that the @not_implemented tags added here are lifted again later in
this same series: the commits that follow port the missing behaviour from
Greenplum, and by the end of the branch nothing carries that tag. They are
added here so that this commit -- the one that puts Behave into CI -- lands
with a job that is green, rather than one that is red until the last commit
in the series.
* test/behave: filter the scenarios Cloudberry does not support
Four scenarios in the matrix test behaviour the product does not have.
They are kept in the tree, aligned with Greenplum, so they can be
enabled with the feature -- but tagged and filtered out, because a job
that ships permanently red teaches everyone to ignore it.
@not_implemented, for behaviour that was never built:
- "should drop existing slot on full recovery" and the full variant of
"should not try to drop slot if slot does not exist" expect
gprecoverseg to check and drop internal_wal_replication_slot during a
full recovery. Cloudberry instead runs pg_basebackup without
--create-slot and retries with it if anything fails, so it never
prints what these scenarios assert.
- "recovers segment for valid max-rate options" needs gprecoverseg
--max-rate, which Cloudberry does not have.
- the two "config file contains hostname" scenarios need gprecoverseg's
-i file to accept a leading hostname. Greenplum's _parseConfigFile()
takes 3, 4 or 5 parts and cross-checks hostname against address;
Cloudberry's takes 3, so the run stops at "expected 3 parts on failed
segment group, obtained 4".
@known_failure, a separate tag for behaviour that exists and is broken:
- "gpinitsystem should print FQDN in pg_hba.conf for standby when
HBA_HOSTNAMES=1" never gets a standby. CREATE_STANDBY_QD runs
gpinitstandby -s localhost --hba-hostnames and it gives up in under a
second against a coordinator that is up and answering queries,
leaving no standby data directory while gpinitsystem still exits 0.
* test/behave: un-skip the HBA_HOSTNAMES=1 standby scenario
The scenario was tagged @known_failure because gpinitsystem left it with
no standby and nothing but a warning. The cause is the bare -f fixed in
the previous commit: with -I, gpinitsystem never reaches the line that
sets CLUSTER_BOOT_MODE=DEMO -- that assignment sits inside the
'no input config' branch -- so the scenario took the PRODUCTION path and
gpinitstandby died on 'option requires 1 argument'.
Verified on a five-node cluster, on the scenario's own shape:
gpinitsystem -a -I <config> -s scdw -h <hostfile>
-> rc 0, 'Successfully completed standby coordinator initialization',
standby streaming in gp_stat_replication
@known_failure had exactly one user, so the tag and its matrix filter go
with it.
* test/behave: drop the imported gpstop scenarios that have no steps
The alignment commit brought seven gpstop.lock scenarios over from
Greenplum that cannot run: they need
the user runs gpstop -a, selects s and interrupt the process
verify if the gpstop.lock directory is present in coordinator_data_directory
and neither step is defined -- not in Cloudberry, and not in
gpdb-archive either, where the scenarios sit in gpstop.feature with no
implementation behind them. Greenplum tags them @concourse_cluster and
@demo_cluster both, so they are undefined in its own runs too.
Nothing caught this because the Behave matrix filters @concourse_cluster
out, and a scenario carrying that tag is excluded whatever else it
carries, so a dry run under the matrix's own tags reported zero
undefined steps. Running them on a five-node cluster reports 13
undefined steps and 11 failed scenarios.
They contribute nothing to the CI this series adds -- which does not run
@concourse_cluster at all -- and an imported scenario that can never
pass is the same trap as the differential-recovery and
recovery-config-hostname imports already dropped here. Remove them; the
two added gpstop scenarios that do have steps stay.
behave --dry-run on gpstop.feature now reports 0 undefined for every tag
combination, not just the matrix's.
* test/behave: drop the imported scenarios whose steps do not exist
Same finding as the gpstop.lock scenarios, in gprecoverseg and
gpmovemirrors: 51 undefined steps in scenarios the alignment brought
over, invisible to CI because they are @concourse_cluster and a scenario
carrying that tag is filtered out whatever else it carries.
Two kinds, and upstream decides which is which:
- Steps Greenplum defines and we simply did not port. Those are worth
having, and the previous commit took the four gpstart ones across.
- Steps that exist upstream but are not applicable. gpmovemirrors'
'a sample gpmovemirrors input file is created in "{config}"
configuration' is the clear case: it hardcodes Greenplum's Concourse
topology -- ports 21000/21001 and a fixed sdw1/sdw2/sdw3 mirror map,
with a comment saying so. This cluster uses 7000/7001 and its spread
layout differs at contents 2 and 3, so the code cannot be reused,
only rewritten.
- Steps upstream does not have at all: the pg_start_backup /
pg_stop_backup pair, 'pg_rewind is killed on mirror with content',
the recovery-type config-file steps, the SUSPEND_PG_REWIND pair.
Several belong to features Cloudberry does not implement -- the
recovery-type column is the 5-part -i config format already tagged
@not_implemented here.
Drop the scenarios that depend on either of the last two. behave
--dry-run now reports 0 undefined for gprecoverseg, gpmovemirrors,
gpstart, gpstop, gpstate, gpaddmirrors and gpactivatestandby under both
the CI matrix's tags and the @concourse_cluster tags.
* test/behave: tag the scenarios that need differential recovery
gprecoverseg --differential does not exist in Cloudberry: the option is
not declared, and the rsync machinery behind it (sync_pg_data,
sync_tablespaces, write_conf_files, isDifferentialSynchronization) was
never carried over either. The feature file already excludes such
scenarios with @Differential, and the CI job filters on it, but six
places were missed when the file was imported.
One of them runs in CI today. "differential recovery runs successfully"
lost its tags entirely in the import -- upstream has @demo_cluster and
@concourse_cluster on it -- so nothing filters it out and it calls
gprecoverseg -av --differential.
Three more only matter on a multi-node cluster, where the
@concourse_cluster scenarios run, and the last two are Examples rows
driving a Scenario Outline whose other rows are fine; those are split
into their own tagged block, which is how the pg_controldata and
replication-slot outlines in this file already handle it.
* test/behave: finish excluding the recovery options Cloudberry does not have
The earlier pass only looked for --differential behind 'the user runs',
and missed the asynchronous step form, so two scenarios still drove a
differential recovery. It also stopped at --differential: --replay-lag
and --disable-replay-lag are equally absent -- no options declared, and
no ALLOWED_REPLAY_LAG or rebalanceSegments.replay_lag behind them -- so
those two scenarios get @not_implemented, the way the --max-rate
scenario in this same file already does.
The last one is ours to pass rather than skip. recoveryinfo.py prints
Failed to {} the following segments. You must run gprecoverseg -F
for all incremental failures
which is right for a tree with no differential recovery to suggest, and
is what this repository's own unit test expects. The scenario was
asserting upstream's wording, which offers "--differential or" first.
Match the message the product actually emits.
* test/behave: restore @concourse_cluster on two scenarios that need a second host
Both sit under this file's "@concourse_cluster tests" banner and both
carry the tag upstream; it was lost on import, so they run against the
single-host demo cluster in CI, where they cannot pass.
gpmovemirrors "mirrors come up even if one pg_ctl_start fails" is the
one failure in the ic-behave-gpmovemirrors job:
Exception: Host cdw has both primary and mirror for content 0
at 'verify that mirror segments are in "spread" configuration'
which is simply what spread mirroring looks like when there is one host.
The other three scenarios in that section kept their tag.
gprecoverseg "behave test requires a cluster with at least 2 hosts" says
so in its name and asks for a mirror segment on a remote host.
The two gpstop scenarios that also lost tags are left alone: they touch
only the coordinator, so they do run on a demo cluster, and keeping them
untagged keeps them in CI.
* test/behave: skip the scenarios that need pg_basebackup-in-progress detection
Upstream's gprecoverseg asks gp_stat_replication which segments already
have a pg_basebackup running against them and leaves those out of the
recovery, announcing
Found pg_basebackup running for segments with contentIds [...],
skipping recovery of these segments
Cloudberry has none of it -- not get_segments_with_running_basebackup,
not the handling built around it in _convert_requests_to_triplets -- and
no part of the tree prints that message.
Seven scenarios assert it and cannot pass. On a five-node cluster they
also take the rest of their suite with them: gpmovemirrors.feature:246
failed and the five scenarios after it then failed on "all segments are
not currently running", which is how three real failures became six.
Tag them @not_implemented, as this file already does for --max-rate, and
say in the file what is missing so they can be turned back on with the
feature. Bringing that logic over is a change to gprecoverseg itself and
belongs in its own commit, not in the one that puts behave into CI.
gprecoverseg.feature:792 is deliberately left running: it only asserts
that the message is *not* printed, and the rest of it exercises a real
recovery with a suspended walsender.
The alignment brought over scenarios whose step definitions were missing,
wrong for this product, or wrong for PostgreSQL 16. Repair them so the
suite tests what it claims to:
* test/behave: make gpcheckcat's test operator a prefix operator
add_operator.sql declares my_pk_schema.!# with LEFTARG only -- a postfix
operator, which PostgreSQL removed in 14:
ERROR: operator right argument type must be specified
DETAIL: Postfix operators are not supported.
psql without ON_ERROR_STOP still exits 0, so the scenario's 'psql should
return a return code of 0' passed and the failure only showed up two
steps later: with no pg_operator row to delete there is nothing for
missing_extraneous to find, and gpcheckcat returned 0 where the scenario
wants 3.
RIGHTARG gives a prefix operator, which is still a pg_operator entry
identified by a non-oid primary key -- which is what 'gpcheckcat should
report and repair extra entries with non-oid primary keys' is about.
* test/behave: snapshot the logs of a scenario that failed
Scenarios routinely begin by emptying gpAdminLogs, so by the time a
feature finishes the logs belonging to the scenario that failed have
been deleted several times over and the CI artifact says nothing about
it. Chasing an intermittent gprecoverseg failure through three CI runs
turned up exactly one surviving recovery log, from a scenario that had
passed.
On failure, copy gpAdminLogs, gp_segment_configuration and every
segment's postgresql.conf into build-logs/failed-scenarios/<line>.<name>/
while they still describe the failure. Wrapped so that a problem
collecting diagnostics can never turn one failure into a different
one.
* test/behave: give each segment its own copy of postgresql.conf to check
'check segment conf: postgresql.conf' rsyncs every segment's
postgresql.conf to a local path named after the segment's host. On a
single-host demo cluster all of them are 'cdw', so all eight land on one
file -- and rsync's quick check skips a transfer whose size and mtime
match the destination. Segments initialised by the same gpinitsystem run
share an mtime, and their port lines are the same length (port=7003 vs
port=7006), so the transfer is skipped and the check reads the previous
segment's file:
port value in postgresql.conf of cdw is incorrect.
Expected:'7006', given:'7003'
The snapshot this series now takes when a scenario fails shows every
segment holding its own correct port moments later, dbid6 included -- the
file was never wrong, the check was reading the wrong one. It looked
intermittent because it depends on two segments' files agreeing on size
and mtime.
Greenplum has the same step, but runs it on a multi-host concourse
cluster where the hostnames differ and the collision cannot happen.
Name the copy after the dbid as well, and say which dbid in the error.
* test/behave: port the gpstart steps the alignment missed
Running gpstart's @concourse_cluster scenarios on a real cluster reports
four undefined steps:
fts probing is disabled
the host for the {seg_type} on content {content} is made unreachable
and do not wait for failover
the role of the {seg_type} on content {content} should be "{role}"
Greenplum has all of them, so this is a gap in the port rather than
anything to invent: gpstart.py factors the body of the 'is made
unreachable' step into _make_unreachable(context, seg_type, content,
utility=False) and builds both variants on it, change_hostname takes the
same utility flag, and must_have_expected_role mirrors the existing
must_have_expected_status. 'fts probing is disabled' is in mgmt_utils.py
beside the walsender fault-injection steps.
Taken across as-is; the files now line up with upstream.
behave --dry-run on gpstart.feature and gpstop.feature reports 0
undefined for every tag combination.
* test/behave: teach the background-pid steps about the coordinator
gpstop.feature's lock-file scenario runs both of these against
"coordinator", and neither handles it, so hostname is never assigned:
UnboundLocalError: local variable 'hostname' referenced before
assignment
at 'we run a sample background script to generate a pid on
"coordinator" segment'
which is the one failure in the ic-behave-gpstop job. The scenario is
identical to upstream's; only the elif arm was lost on import.
The kill step lost its second command as well, the one that removes
/tmp/bgpid. That matters beyond tidiness: the step that starts the
script waits for /tmp/bgpid to appear and then reads it, so a file left
behind by an earlier scenario is picked up immediately and the test goes
on to kill a pid that is no longer running. Upstream also sleeps a second
before that wait for the same reason; restore both.
* test/behave: save the port and address of the remote primary again
'the information of a "primary" segment on a remote host is saved' lost
the last two of its six assignments, so context.remote_pair_primary_port
was left to whatever an earlier scenario happened to store and
context.remote_pair_primary_address was never assigned anywhere in the
tree at all -- while seven steps in this same file read it:
port, hostname, datadir, address = context.remote_pair_primary_port,
context.remote_pair_primary_host,
context.remote_pair_primary_datadir,
context.remote_pair_primary_address
Eight scenarios in gprecoverseg.feature start from that step, all of
them @concourse_cluster, which is why nothing noticed: they only run on
a multi-host cluster, and there they fail on AttributeError before
testing anything.
* test/behave: repair the demo-cluster command and wait out the failover
Two things in replication_slots_utils.py, both reached from CI jobs that
are green today.
create_cluster builds a shell command out of a continued string, and one
continuation had picked up a stray character:
export WITH_MIRRORS={with_mirrors} && \A
The backslash escapes the A instead of the newline, so the shell runs a
command called A, the && chain ends there, and demo_cluster.sh runs as a
separate statement on the next line. It happens to still build a cluster,
and the exit status checked afterwards is that last statement's, which is
why nobody noticed -- but nothing was actually holding the chain
together. replication_slots.feature drives this four times.
'a preferred primary has failed' stops the primary and returns
immediately, leaving the next step to race the failover. Upstream waits
for transactions to unblock first; the helper is already imported here.
gpaddmirrors.feature and replication_slots.feature both start scenarios
with that step.
* test/behave: always put back a hostname the test made unreachable
These scenarios point a segment at invalid_host and rely on their last
step to put the real name back. When anything in between fails, that step
never runs and the catalog keeps invalid_host, which is not a failure the
next scenario can survive: while a primary's address does not resolve the
coordinator will not accept an ordinary connection at all, so every
scenario after it dies in before_scenario.
That is what happened to the gpstart suite on a five-node cluster --
21 scenarios failed, but only two of them for a reason of their own; the
other 19 never got a connection.
The old name is already saved a couple of lines up, so register a cleanup
with it. behave runs cleanups whether the scenario passed or failed, and
utility mode still connects while the address is unresolvable, which is
how the name was changed to begin with. The same pattern is used for the
temp directories in gpconfig and the crashed mirror in
replication_slots_utils.
* test/behave: give gpstate's recovery scenarios a lock with a live pid
These two scenarios fake an in-progress recovery: they write a sample
recovery_progress.file and create a gprecoverseg.lock directory. That was
enough while gpstate only checked whether the directory existed, and it
stops being enough now that it reads the pid inside and asks whether that
process is alive -- both scenarios started failing with "All segments are
running normally".
Upstream sets them up the way this change needs: start a background
script, build the lock directory around its pid, and kill it at the end.
The steps for that already exist here and now handle "coordinator", so
use them, and drop the pid-less variant -- it is Cloudberry-only, nothing
else calls it, and leaving it would invite someone to build a lock
directory that gpstate quite deliberately ignores.
* test/behave: match the way pg_rewind reports failure on PostgreSQL 16
The step that checks a failed incremental recovery looked for
pg_rewind: fatal
which is what PostgreSQL 12 printed. The fatal level was removed in 13:
this tree's src/common/logging.c emits only "error: ", "warning: " and
"detail: ", and pg_rewind never calls pg_log_fatal, so the string can
never appear and the scenario could not pass.
"error" alone is not the answer either. A failed pg_rewind now writes two
lines -- "error: postgres single-user mode in target cluster failed" and
then "detail: Command was: ..." -- while gprecoverseg streams whichever
line is last in the progress file when it next polls, which is nearly
always the detail. Observed on a five-node cluster: seven of the polls
for the failing dbid showed the detail line and none showed the error.
The assertion is a regex, so accept either. It still distinguishes a
failure from the ordinary progress lines ("syncing target data
directory", "49550/1202320 kB (4%) copied") that pg_rewind also emits.
* test/behave: stop counting recovery logs per failing segment
recovery_fail_check gained an assertion upstream does not make:
And gpAdminLogs directory has "{logfile_name}" files on respective
hosts only for content {content_ids}
with content_ids being only the segments whose recovery was expected to
fail. That step counts the log files on each host holding one of those
mirrors and insists the count matches exactly, so it is wrong whenever
another segment on the same host was recovered the same way in the same
run.
gprecoverseg.feature:1042 does exactly that -- content 1 full inplace
(succeeds), content 2 full to a bad directory (fails) -- and on a
single-host cluster both write a pg_basebackup log to the same
gpAdminLogs:
expected 1 pg_basebackup* files ... on host cdw, but found 2
The scenarios that do want this check call the step themselves with the
complete list of contents recovered that way, which is the only way the
count can be right: gprecoverseg.feature:784 asks for content 0,1 and
gpmovemirrors.feature:518 for content 0,1,2,3,4,5.
demo_cluster.sh writes the cluster config through an unquoted here-doc,
so
TRUSTED_SHELL="$(dirname "$0")/lalshell"
is expanded as the file is written, not as it is sourced. gpAux/gpdemo's
Makefile invokes the script as ./demo_cluster.sh, which makes $0
relative and dirname "." -- so the generated config carries
TRUSTED_SHELL="./lalshell"
gpinitsystem sources that file from wherever it happens to be running
and looks for lalshell in that directory instead.
Nothing noticed while the demo cluster was only ever created by
demo_cluster.sh itself, from the directory lalshell sits in, but the
Behave suite runs
gpinitsystem -a -c ../gpAux/gpdemo/clusterConfigFile
directly in a dozen scenarios. Resolve the directory the script itself
lives in and write that, and keep the quotes so a path containing
whitespace survives being sourced.
Rebalancing promotes the mirror and demotes the primary. If the mirror
still has a large amount of WAL left to replay, that promotion stalls
until it catches up, with the segment unavailable for the duration.
Greenplum guards against this by measuring the replay lag before
rebalancing a pair and refusing when it exceeds a limit. Cloudberry
carries the file this lives in but not the check: replay_lag(), the
block in rebalance() that calls it, the ALLOWED_REPLAY_LAG default and
the two options that drive it had all been dropped.
Port them back, so these two scenarios can run instead of being skipped:
gprecoverseg rebalance aborts and throws exception if replay lag on
mirror is more than or equal to the allowed limit
gprecoverseg errors out if invalid options are used with
--disable-replay-lag
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/operations/rebalanceSegments.py
gpMgmt/bin/gppylib/programs/clsRecoverSegment.py
gpMgmt/bin/gppylib/commands/gp.py
gpMgmt/bin/gppylib/test/unit/test_unit_rebalance_segment.py
The one deliberate departure from upstream: Greenplum reaches the same
check through a differential-recovery code path that Cloudberry does not
have, so only the rebalance caller is ported.
A full recovery runs pg_basebackup, which will use whatever network
bandwidth it can get. On a cluster that is still serving queries that
can starve everything else, so Greenplum lets the operator cap it with
--max-rate, validated and then passed through to pg_basebackup.
Cloudberry has the same plumbing -- gprecoverseg builds a
GpMirrorListToBuild, which builds a GpSegRecovery command line, which
gpsegrecovery parses and turns into a PgBaseBackup -- but no maxRate
parameter anywhere along it. Add it at each hop, along with the
validation, the two "this flag is ignored with -r/-o" warnings, and the
"Maximum Transfer Rate" row in the recovery plan the scenario asserts on.
This lets the scenario run instead of being skipped:
gprecoverseg recovers segment for valid max-rate options and errors
out for others
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/programs/clsRecoverSegment.py
gpMgmt/bin/gppylib/operations/buildMirrorSegments.py
gpMgmt/bin/gppylib/commands/gp.py
gpMgmt/bin/gppylib/commands/pg.py
gpMgmt/sbin/gpsegrecovery.py
gpMgmt/sbin/recovery_base.py
Two deliberate departures from upstream. Greenplum also threads maxRate
into its differential recovery path, which Cloudberry does not have.
And Cloudberry's FullRecovery retries pg_basebackup with --create-slot
when the first attempt fails on a cluster that never had mirrors;
upstream has no such retry, so the cap is applied to it too -- without
that the retry would silently ignore the limit the operator asked for.
The comment that claimed Greenplum does not implement this either was
wrong, and goes with the tag.
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.
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.
SUSPEND_PG_REWIND is a test-only hook, compiled in under FAULT_INJECTOR, that holds pg_rewind still long enough for a test to catch it in pg_stat_activity and assert that a second gprecoverseg leaves that segment alone. Cloudberry carries an older revision of it: the value has to be the literal string "true", and the sleep is a fixed 120 seconds. Greenplum later made the value the duration. The two scenarios that use this hook set it to "600", which strcmp() rejects, so pg_rewind is never suspended and the step that waits for it to appear gives up: verify that pg_rewind is running for content 0 Exception: Timed out after 600 retries Take the later form, so the caller says how long. Found by running gprecoverseg.feature against a real multi-node cluster, where "gprecoverseg rebalance should be able to terminate gracefully when interrupted by the user" failed on exactly this. With the hook working, that scenario gets much further but is still not reliable on a five-node cluster: it waits only for content 0's pg_rewind before sending SIGTERM, then asserts that the mirrors for content 0 and 1 are both down, and whether content 1 has got that far is a matter of timing. It passed on one run here and failed on two. Waiting for both rewinds first does not fix it -- they are not running at the same time, so the wait itself times out -- and the scenario is identical upstream, so this is left alone rather than papered over. It is @concourse_cluster, which the CI this series adds does not run. cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4, src/bin/pg_rewind/pg_rewind.c.
… retry
A full recovery streams from the primary through the replication slot
internal_wal_replication_slot. pg_basebackup --create-slot fails if that
slot already exists, and PostgreSQL offers no way to reuse one.
Cloudberry worked around it by guessing: run pg_basebackup without
--create-slot, and if anything at all goes wrong, run the whole thing
again with it. That is expensive when the backup failed for an unrelated
reason -- the second attempt also forces forceoverwrite=True, discarding
the target directory -- and it leaves the slot handling implicit. The
code says as much:
GPDB_12_MERGE_FIXME could we check it before? or let
pg_basebackup create slot if not exists.
Greenplum answers that FIXME: ask the primary whether the slot is there,
drop it if so, and pass --create-slot once. Port PgReplicationSlot and
use it, in gpsegrecovery for gprecoverseg and in gpconfigurenewsegment
for gpaddmirrors and gpexpand, dropping both retries.
Porting it into gpconfigurenewsegment is not optional. PgBaseBackup's
constructor now talks to the database, and there the second constructor
call sat inside an except block, so a failure to reach the primary would
have escaped from the handler rather than being reported.
This lets the last two skipped scenarios run, which also assert the
messages this prints:
gprecoverseg should drop existing slot on full recovery
<scenario> recovery should not try to drop slot if slot does not exist
With them, nothing in the suite is tagged @not_implemented any more.
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/commands/pg.py
gpMgmt/sbin/gpsegrecovery.py
gpMgmt/bin/lib/gpconfigurenewsegment
Two things in gpconfigurenewsegment are fixed in passing, both found by
running open-code-review over this commit:
- It called PgBaseBackup with logfile=, a keyword that constructor has
never had, so the mirror path raised TypeError before doing anything.
Upstream has the same line and the same bug; nothing in either test
suite reaches it, because gpaddmirrors builds mirrors through
gpsegrecovery and only gpexpand comes this way. It is progress_file=.
- The constructor call sat outside the try below. Now that it talks to
the database it can fail, and a failure there returned no result at
all, which the worker pool reports as "'NoneType' object has no
attribute 'wasSuccessful'" long after the fact.
One deliberate departure from upstream: drop_slot() catches pgdb's
DatabaseError, not psycopg2's, because that is the driver Cloudberry's
dbconn is built on.
tuhaihe
force-pushed
the
add-behave-test-ci
branch
from
October 19, 2026 09:21
4416f22 to
8377856
Compare
leborchuk
approved these changes
Oct 21, 2026
leborchuk
left a comment
leborchuk
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
I agree with paths
paths:
- 'gpAux/**'
- 'gpMgmt/**'
- 'src/bin/**'
# gp_stat_replication lives here, and gpstate and gprecoverseg steps
# query it directly.
- 'src/backend/catalog/system_views.sql'
- '.github/workflows/behave-cloudberry.yml'
Also I see failed ic-behave-gpstop. But we got coredump in that test. And it looks like a real issue we find out, it's not the failed beehave test.
3 of 8 tasks
MisterRaindrop
left a comment
MisterRaindrop
left a comment
Contributor
There was a problem hiding this comment.
Thanks for pushing this through, @tuhaihe. Getting the Behave suite running in CI is worth a lot, and since you asked for someone to take over, here is a suggestion for how.
Scope. Of the 23 commits, only four are about the tests themselves:
- ea42235 CI: add a GitHub Actions workflow for gpMgmt Behave tests
- 2b4b6d3 test/behave: align the gpMgmt suite with the Greenplum test suite
- f196edc test/behave: exclude the scenarios Cloudberry does not implement
- 70fd128 test/behave: repair the step definitions the alignment needs
The other 19 change product behaviour: fixes in gpinitsystem, gprecoverseg, gpcheckcat, gpstate, gpstop, seg_update_pg_hba, analyzedb, minirepro and gpdemo; features ported from Greenplum (gprecoverseg --max-rate, the rebalance replay-lag check, skipping segments already in recovery, hostnames in -i, dropping a stale replication slot); and five server-side C changes:
- 24faa8f
xlogrecovery.c/postmaster.c: wait for promotion before reporting ready withpromote_trigger_file - b432ad8
pg_basebackup.c: send the EXCLUDE options to the server - ac20f61
pg_dump.c: stop overriding--function-oidswith the namespace policy - 735b623
pg_rewind.c: letSUSPEND_PG_REWINDset the duration
Merged as one PR, all of these would land on a single review of a CI change, and the server-side ones in particular deserve a reviewer of their own.
Proposal.
- Reduce this PR to the four test commits, and exclude the scenarios that still fail without the fixes, as @leborchuk suggested. That gives us the Behave job on
mainquickly, with a diff that only touches tests and CI. - Turn the other 19 commits into small PRs, one fix each. They are already separate commits, so this is mostly cherry-picking; each new PR can re-enable the scenarios its fix makes pass, which gives every fix a test. The C changes first, since they need the most careful review.
If you're happy with that, I'll do the split and open the follow-up PRs, keeping you as the author of the commits.
About ic-behave-gpstop. All 7 scenarios passed; the job failed on the core check, and the core is the one the "gpstop succeeds when pg_ctl command fails" scenario is meant to produce. From the job's gpAdminLogs:
02:53:58 gpstop: Starting gpstop with args: -a -M fast
02:55:58 gpstop: Failed to shutdown coordinator with pg_ctl.
02:55:58 gpstop: Sending SIGQUIT signal...
02:56:03 gpstop: Sending SIGABRT signal...
The core (core-postgres-6-...-20378-1792403763) is the coordinator postmaster (qddir, port 7000), killed by signal 6 at 1792403763 = 09:56:03 UTC, i.e. 02:56:03 in the container's PDT: the same second gpstop sent SIGABRT. The postmaster was in ServerLoop → FaultInjector_InjectFaultIfSet_out_of_line → s_lock because, 5 seconds (SIGKILL_CHILDREN_AFTER_SECS) after the SIGQUIT, an immediate shutdown checks the postmaster_server_loop_no_sigkill fault on each loop, and that check takes the fault injector's spinlock. That is also when gpstop's 5-second wait ran out.
So the abort is the behaviour under test. The check misses it because it looks for "Sending SIGABRT signal" under build-logs/details/, but gpstop logs to /home/gpadmin/gpAdminLogs/gpstop_*.log, and behave doesn't print a passing step's output into the make log. Searching /home/gpadmin/gpAdminLogs as well should turn this job green; the "Collect gpAdminLogs" step already archives that directory.
Member
Author
Hi @MisterRaindrop, thanks for your help! Good idea. I'm glad you can help move this project forward. Just keep going! |
2 of 6 tasks
Contributor
|
Closing this in favour of the split agreed above; the branch and its history stay here for reference.
A correction to my review: I wrote "five server-side C changes" but listed four commits; 24faa8f touches two files ( |
MisterRaindrop
added a commit
to MisterRaindrop/cloudberry
that referenced
this pull request
Oct 23, 2026
The suite comes from apache#1639, where it ran with that PR's fixes. Here it runs without them, and these scenarios fail until they land. Tag them @needs_fix and skip that tag in every job; each fix removes the tag from the scenarios it makes pass, which is also how it shows they do. A comment beside each tag names the fix. analyzedb 1 0977ac5 (identifiers and pg.escape_string) gpactivatestandby 1 apache#1889 (forced activation after a clean shutdown) gpaddmirrors 2 mirror postgresql.conf check; not pinned down yet, among b971d48, a3bfe47 and 8377856 gpcheckcat 11 0bfc1d7 gpinitstandby 2 apache#2172 (pg_basebackup EXCLUDE); without it the new standby promotes itself gpinitsystem 19 b971d48 gpmovemirrors 4 a3bfe47, 51955ac, 7056e02 (one of them a Scenario Outline with four examples) gpstop 2 45a1155, 51955ac (the gpstop lock) minirepro 3 3fccb75 (tables a view reads), 51955ac, apache#2175 (pg_dump --function-oids) The commit hashes are those in apache#1639. gprecoverseg is tagged as a whole: without its fixes a scenario hangs and the job reaches its 120 minute timeout, so the scenarios that do fail cannot be told apart from the ones never reached. It comes back as the gprecoverseg fixes land.
MisterRaindrop
added a commit
to MisterRaindrop/cloudberry
that referenced
this pull request
Oct 24, 2026
…USTED_SHELL demo_cluster.sh writes TRUSTED_SHELL="./lalshell" into the generated cluster config when it is run as ./demo_cluster.sh, so the scenarios that run gpinitsystem -c ../gpAux/gpdemo/clusterConfigFile from gpMgmt fail with "./lalshell: No such file or directory". 767f126 in apache#1639 fixes that; tag them needs_fix until it lands.
This was referenced Oct 24, 2026
MisterRaindrop
pushed a commit
to MisterRaindrop/cloudberry
that referenced
this pull request
Oct 24, 2026
* gpcheckcat: add the mix_distribution_policy check
Back-port from Greenplum a check Cloudberry was missing.
mix_distribution_policy reports tables whose distribution policy mixes
legacy and non-legacy hash opclasses, and cross-checks the result
against the gp_use_legacy_hashops GUC so the operator is told which of
the two states is inconsistent.
Add the SQL fixtures the corresponding Behave scenarios load.
Greenplum's unit tests for the check mock a psycopg2 cursor, but this
gpcheckcat reaches the database through PyGreSQL (conn.query() and
getresult()) and never opens a cursor, so three of them failed and the
rest passed without exercising the code. They are rewritten against
conn.query(), with tests added for a mixed policy, a clean one, and a
GUC that is read successfully.
Greenplum's companion ao_lastrownums check is deliberately not brought
over: it reads pg_attribute_encoding.lastrownums, a column Cloudberry's
catalog does not have, so the query errors out on every run and
gpcheckcat reports failure on a freshly created database.
* gpcheckcat: give pg_shdescription its TableMainColumn entry
TableMainColumn maps a catalog to the column that identifies the object
an entry belongs to. Greenplum has a row for pg_description and one for
pg_shdescription; Cloudberry has only the first. Without it,
processMissingDuplicateEntryResult() falls through to getOidFromPK(),
which looks the entry up by
SELECT oid FROM pg_shdescription WHERE ...
and pg_shdescription has no oid column, so missing_extraneous died with
Execution error: ERROR: column "oid" does not exist
LINE 4: SELECT oid FROM pg_shdescription
* gpcheckcat: attribute an AO table's visimap to the table in the owner check
checkOwners() compares each relation's owner across segments, and
folds an append-optimized table's auxiliary relations into the table
they belong to, so a mismatch is reported, and repaired, once, on the
table. It knew the segment relation, the block directory and the block
directory's index, but not the visimap relation or its index. Those
were reported as relations of their own, and the repair script tried
ALTER TABLE "pg_aoseg"."pg_aovisimap_17032" OWNER TO "gpadmin";
which the server refuses:
ERROR: permission denied: "pg_aovisimap_17032" is a system catalog
so "gpcheckcat -R owner" on an append-optimized table whose owner
differs on a segment produced a repair that could not run. Match the
visimap and its index, and the block directory's index by the table it
indexes. The repair is then the ALTER TABLE on the table itself, which
carries its auxiliary relations with it.
The feature-file half of this change -- dropping the two ao_lastrownums
scenarios and tagging the -x session GUC scenario, which Cloudberry no
longer supports -- edits the Behave suite as apache#2170 aligns it, so it goes
with that PR.
Split out of apache#1639 (originally 0bfc1d7).
MisterRaindrop
added a commit
to MisterRaindrop/cloudberry
that referenced
this pull request
Oct 24, 2026
…fixes Eighteen gprecoverseg scenarios and three gpmovemirrors scenarios were tagged @not_implemented, on the grounds that Cloudberry lacks what they test. apache#1639 implements all of it, so they are tagged @needs_fix instead, each with the apache#1639 commit that makes it pass, the way the other scenarios waiting on a fix are: 7056e02 skip segments that already have a recovery running (7) 0aa2666 a hostname in the -i recovery config file (9) 8377856 drop a stale replication slot (2) 993b99d --max-rate (1) ad3c9d5 the rebalance replay-lag check (2) The comment on the feature-level tag now names only the fixes the whole feature waits on; the three Greenplum ports above are tagged scenario by scenario. The two gpaddmirrors scenarios whose fix was not pinned down are pinned to a3bfe47. Without it both fail on port value in postgresql.conf of cdw dbid 6 is incorrect because a recovered mirror keeps the port of the primary it was copied from, and with it both pass.
MisterRaindrop
added a commit
to MisterRaindrop/cloudberry
that referenced
this pull request
Oct 24, 2026
Three changes to gpcheckcat.feature, the half of apache#1639's gpcheckcat commit (0bfc1d7) that edits the suite rather than gpcheckcat: - The two ao_lastrownums scenarios go. That check reads pg_attribute_encoding.lastrownums, which Cloudberry's catalog does not have, so the gpcheckcat fixes do not bring it over and the scenarios can never pass. - "validate session GUC passed with -x is set" is tagged @not_implemented instead of @needs_fix. Its first half needs a postmaster started with gp_role=utility to refuse non-utility connections, a check Greenplum makes in InitPostgres() and Cloudberry dropped; no gpcheckcat change makes it pass. The gpcheckcat job now skips @not_implemented, as the gprecoverseg job already does. - The primary-key scenario deletes the operator through psql, as upstream does, rather than in utility mode on the first primary. Both pass; this keeps the scenario aligned with upstream. The remaining eight @needs_fix tags in the file still point at 0bfc1d7. With the tags removed, all eight fail without it and pass with it, including "report and repair owner errors on appendonly tables": its repair script alters the table's visimap relation, which the server refuses as a system catalog, until the gpcheckcat fix maps the visimap to its table.
This was referenced Oct 24, 2026
MisterRaindrop
pushed a commit
to MisterRaindrop/cloudberry
that referenced
this pull request
Oct 24, 2026
… retry
A full recovery streams from the primary through the replication slot
internal_wal_replication_slot. pg_basebackup --create-slot fails if that
slot already exists, and PostgreSQL offers no way to reuse one.
Cloudberry worked around it by guessing: run pg_basebackup without
--create-slot, and if anything at all goes wrong, run the whole thing
again with it. That is expensive when the backup failed for an unrelated
reason -- the second attempt also forces forceoverwrite=True, discarding
the target directory -- and it leaves the slot handling implicit. The
code says as much:
GPDB_12_MERGE_FIXME could we check it before? or let
pg_basebackup create slot if not exists.
Greenplum answers that FIXME: ask the primary whether the slot is there,
drop it if so, and pass --create-slot once. Port PgReplicationSlot and
use it, in gpsegrecovery for gprecoverseg and in gpconfigurenewsegment
for gpexpand, dropping both retries.
Porting it into gpconfigurenewsegment is not optional. PgBaseBackup's
constructor now talks to the database, and there the second constructor
call sat inside an except block, so a failure to reach the primary would
have escaped from the handler rather than being reported.
This lets the last two skipped scenarios run, which also assert the
messages this prints:
gprecoverseg should drop existing slot on full recovery
<scenario> recovery should not try to drop slot if slot does not exist
With them, nothing in gprecoverseg.feature or gpmovemirrors.feature is
tagged @not_implemented any more.
cherry-pick from Greenplum Database, greenplum-db/gpdb-archive @ 482967c1b4:
gpMgmt/bin/gppylib/commands/pg.py
gpMgmt/sbin/gpsegrecovery.py
gpMgmt/bin/lib/gpconfigurenewsegment
Two things in gpconfigurenewsegment are fixed in passing, both found by
running open-code-review over this commit:
- It called PgBaseBackup with logfile=, a keyword that constructor has
never had, so the mirror path raised TypeError before doing anything.
Upstream has the same line and the same bug; nothing in either test
suite reaches it, because gpaddmirrors builds mirrors through
gpsegrecovery and only gpexpand comes this way. It is progress_file=.
- The constructor call sat outside the try below. Now that it talks to
the database it can fail, and a failure there returned no result at
all, which the worker pool reports as "'NoneType' object has no
attribute 'wasSuccessful'" long after the fact.
One deliberate departure from upstream: drop_slot() catches pgdb's
DatabaseError, not psycopg2's, because that is the driver Cloudberry's
dbconn is built on.
Split out of apache#1639 (originally 8377856).
Draft
2 tasks done
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.
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions