Repository navigation
gpinitsystem: fix the defects the Behave suite exposes - #2178
Open
MisterRaindrop wants to merge 1 commit into
Open
MisterRaindrop wants to merge 1 commit into
MisterRaindrop wants to merge 1 commit into
Conversation
Each of these is one or two lines Cloudberry lost against Greenplum,
invisible until the Behave suite exercises the feature:
* gpinitsystem: set the mirror's own port after pg_basebackup
CREATE_QES_MIRROR seeds the mirror with pg_basebackup, which copies the
primary's postgresql.conf verbatim -- including the primary's port. The
mirror was then started with START_QE before that port was corrected, so
on a single host it either bound the wrong port or collided with its own
primary. Rewrite postgresql.conf before starting the segment.
Greenplum has the same SED_PG_CONF call but places it after START_QE,
which leaves the same window open there.
Also emit explicit 127.0.0.1/32 and ::1/128 trust entries in the
coordinator's pg_hba.conf. The hostname-based entry alone is not enough
when the loopback address the client picks depends on the host's IPv4
and IPv6 resolution order.
* gpinitsystem: make FORCE_FTS_PROBE actually request a probe
The loop waits for every segment to leave mode 'n'/status 'd', but the
call that asks FTS to look -- Greenplum ends each iteration with
select gp_request_fts_probe_scan()
-- is missing, so all sixty iterations re-read an unchanged
gp_segment_configuration in a couple of seconds and give up long before
gp_fts_probe_interval elapses. gpinitsystem then logs
[WARN]:-Failed to start Cloudberry instance; please review
gpinitsystem log to determine failure.
and returns 0 with all three mirrors still down. That is also why the
FATAL was commented out with a 'should support notify fts' TODO: the
function could never succeed, so the error it raised had to go instead.
Restore the probe call. Leaving the FATAL commented out for now -- that
is a separate decision about how hard gpinitsystem should fail.
Fixes the three gpinitsystem.feature scenarios that begin 'all the
segments are running' after a gpinitsystem run.
* gpinitsystem: do not log the default ETCD/FTS config as a warning
Every gpinitsystem run that is not given -e/-f logs
[WARN]:-No ETCD cluster host config provided, use default configuration.
[WARN]:-No FTS cluster host config provided, use default configuration.
and its own SCAN_LOG_FILE then greps the log for \[WARN\], so every run
ends with
[WARN]:-Scan of log file indicates that some warnings or errors
[WARN]:-were generated during the array creation
'Log file scan check passed' is therefore unreachable, and the two
gpinitsystem.feature scenarios that assert it can never pass. Neither
message reports a problem -- they say which configuration is in use --
so log them at INFO, which the scan already ignores.
* gpinitsystem: only pass -f to gpinitstandby when there are FTS hosts
CREATE_STANDBY_QD branches on CLUSTER_BOOT_MODE and, on the PRODUCTION
side, always appends
-f $FTS_HOST_STRING_LIST
CLUSTER_BOOT_MODE is PRODUCTION for every cluster whose coordinator and
segments are on different hosts -- that is, every real one; DEMO is only
reached when they are the same host. FTS_HOST_STRING_LIST, though, stays
empty unless the cluster was given external FTS hosts. So on an ordinary
multi-host cluster the call expands to a bare trailing -f and
gpinitstandby dies on its own option parser:
gpinitstandby: error: -f option requires 1 argument
in the same second, before it opens a log file. gpinitsystem reports
[WARN]:-Failed to complete standby coordinator initialization
[WARN]:-Cluster setup finished, but Standby Coordinator failed to
initialize.
and still exits 0, so a cluster built with -s comes up with no standby
and nothing but a warning to say so. Running gpinitstandby by hand
afterwards works, which is what has been hiding this.
Guard -f on having a value, the way -P, -S and --hba-hostnames already
are, and drop the branch: the two sides are now the same statement.
Greenplum has no -f here at all and a single unconditional invocation.
Also fix the list builder feeding it: [ -z FTS_HOST_STRING_LIST ] tests
a literal string, which is never empty, so the first host took the
append branch and the list came out with a leading comma. That path
needs external FTS to reach and is not exercised here.
Found on a five-node cluster on EC2; it is also why gpinitsystem.feature's
HBA_HOSTNAMES=1 standby scenario could never pass -- with -I the DEMO
assignment is skipped too, so the demo cluster took the same branch.
* gp_bash_functions: let an [ERROR] message set a failing exit status
LOG_MSG derives the log level from the message and raises EXIT_STATUS so
that a script which ends in "exit $EXIT_STATUS" reports the failure it
just logged. The case statement kept the FATAL arm but lost the ERROR
one, so "[ERROR]" was logged and then thrown away.
gpinitsystem is the only script that both logs [ERROR] and exits on
EXIT_STATUS, and it is how the gap shows: an unrecognised long option
logs "Unknown option --x", prints the usage text, and exits 0. A caller
checking the return code is told the cluster was created.
The behave suite could not catch it, because the scenario that passes an
unknown option was itself asserting the wrong thing -- it ran
gpinitsystem with --ignore-warnings, which is not an option of any
version of this script, and expected 0. It passed on the bogus 0 without
initialising anything, then satisfied its follow-up grep from an older
log still matching the ~/gpAdminLogs/gpinitsystem*log glob. Drop the
stray argument so that scenario tests TRUSTED_SHELL as its name says,
and cover the exit status separately.
The level is matched against the text up to the first ']', so an INFO
line that merely mentions an error is still INFO.
* gp_bash_functions: anchor the substitutions SED_PG_CONF makes
SED_PG_CONF decides between appending and replacing by grepping for
"^[ ]*${SEARCH_TXT}[ ]*=", but the sed expressions it then runs match
${SEARCH_TXT} anywhere on a line, so they edit settings that merely
contain the requested one in their name.
gpinitsystem -p passes each parameter of the supplied file through here.
Asking it for work_mem=999 rewrites
maintenance_work_mem = 64MB
into
maintenance_work_mem=999 #work_mem = 64MB
which is not a comment: the cluster comes up with maintenance_work_mem
silently set to the value asked for work_mem. The same happens to
autovacuum_work_mem and logical_decoding_work_mem, and shared_buffers
reaches into the wal_buffers line.
Restore the anchors so a substitution can only touch the assignment it
names. The searches gpinitsystem and gpcreateseg.sh do for themselves
are unaffected -- "#port" still resolves to an active port= line, which
is what the gpinitsystem behave scenario checks on every segment.
* gpinitsystem: correct CHECK_FTS's argument-count guard
CHECK_FTS reads only $1, the FTS host, and both call sites pass exactly
that, but its guard was copied from CHECK_ETCD -- which does take a
second argument, the boot mode -- and still demands two. So every
external-FTS cluster logged
[ERROR]: CHECK_FTS invalid params...
on a perfectly good check. It was invisible while an [ERROR] could not
influence the exit status; now that it can, gpinitsystem -F would report
failure after succeeding.
CLEANUP_ETCD's guard names RETRIVE_ETCD_CONFIG_VAL in its message, which
sends anyone reading the log after a real failure to a function that was
not involved. Name the function that is.
Every remaining $#-guard in the library and in gpinitsystem was checked
against its call sites; these were the only two wrong.
Split out of apache#1639 (originally b971d48).
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 six gpinitsystem defects that the gpMgmt Behave suite (#2170) exposes. Each is a line or two that Cloudberry lost relative to Greenplum. They are kept together because they share
gpinitsystem/gp_bash_functions.shand the same feature file; the commit message gives each one's full reasoning.CREATE_QES_MIRRORstarted the mirror before fixing the port in thepostgresql.confthat pg_basebackup copied from its primary, so on a single host the mirror bound its primary's port, or collided with it. The port is now rewritten before the start. This change also writes explicit127.0.0.1/32and::1/128trust entries into the coordinator'spg_hba.conf, because a hostname entry alone breaks when the client picks::1.FORCE_FTS_PROBEnever asked for a probe. Thegp_request_fts_probe_scan()call was missing, so the 60 iterations re-read an unchanged catalog in a couple of seconds and gave up. gpinitsystem then exited 0 with all mirrors still down.[WARN]. gpinitsystem's own log scan made every run end with "warnings or errors were generated". These messages are now logged at INFO.-f. On any multi-host cluster without external FTS hosts, gpinitsystem passed-fwith no value, gpinitstandby failed parsing its options, and gpinitsystem still exited 0 with no standby.-fis now passed only when there is a value, and the list builder's always-false[ -z FTS_HOST_STRING_LIST ]is fixed. This was first seen on a five-node EC2 cluster.[ERROR]did not set the exit status inLOG_MSG. As a result,gpinitsystem --unknown-optionexited 0. The scenario that should have caught this was itself passing--ignore-warnings, which is not a gpinitsystem option. That stray argument is dropped, and a scenario for the exit status is added.SED_PG_CONFsubstitutions were unanchored.gpinitsystem -pwithwork_mem=999rewrotemaintenance_work_mem. The anchors are restored.CHECK_FTS's argument-count guard (copied fromCHECK_ETCD) andCLEANUP_ETCD's error message are also corrected, because once fix 5 lands, the wrong guard would makegpinitsystem -Freport a failure after it succeeded.Split out of #1639 (originally b971d48, by @tuhaihe).
Test Plan
Rocky 9 build image, built from #2170's tree plus #2177 (
767f12685a4, absoluteTRUSTED_SHELL), with a demo cluster created the way the Behave workflow creates it. I removed the@needs_fixtags that point at either fix, then ran each feature with CI's arguments. Run A is without this PR; run B is with it, with the three scripts installed into$GPHOME/bin.gpinitsystem.feature(--tags ~@extended)gpaddmirrors.feature(--tags ~@concourse_cluster)Why the scenarios fail in A:
no pg_hba.conf entry for host "::1". That is the loopback part of fix 1.pg_ctlfallback raisesNameError(fixed in CI: run the gpMgmt Behave suite #2170), so all of them start from a cluster that can't be stopped.-Dand bothtrusted_shellscenarios passed.15437, got15434, which fix 1 addresses.#2170 tags these scenarios
@needs_fixpointing at this change. Whichever PR lands second removes those tags.Type of Change
Checklist