fix(otherpkgs): the postscript truncates its syslog output and logs a failed install as installed - #7850
Open
dhilst wants to merge 9 commits into
Open
fix(otherpkgs): the postscript truncates its syslog output and logs a failed install as installed#7850dhilst wants to merge 9 commits into
dhilst wants to merge 9 commits into
Conversation
…as one truncated message The otherpkgs postscript passes the whole package manager transaction to logger as a single message argument. rsyslog escapes every newline to #12 and cuts the message at 8 KiB, which is where the Failed:, Error: and summary lines sit. A line that starts with -- is also read as a logger option. The seven sites in xCAT/postscripts/otherpkgs that log $result now pipe it to logger. logger without a message argument reads standard input and sends one message per line. postscripts_otherpkgs.bats drives the upgrade block and the two preremove blocks with logger and the package manager shadowed, and counts the messages. Against the unfixed script the three tests report one message where three are expected. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
In xCAT/postscripts/otherpkgs the success message runs after the branch that records a non-zero return, not inside an else. A node that fails to install its otherpkgs therefore reports both "... failed." and "... installed." to syslog, and a reader that greps for the success line sees a clean install. The same shape covers the four install sites and the four removal sites. Each success message now sits in the else branch of the status test. postscripts_otherpkgs.bats drives the install and postremove blocks with a package manager that fails, and uses refute_grep to require that no "installed."/"removed." message is sent. Against the unfixed script both tests find the success message. refute_grep is new in helpers/shell_source.bash, because bash ignores errexit for a command inverted with "!". Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…ring Line 672 of xCAT/postscripts/otherpkgs reads [ -n "OTHERPKGDIR_INTERNET" ]. The $ is missing, so the test is on the name of the variable and is always true. The guard cannot select the case it was written for. The guard now tests $OTHERPKGDIR_INTERNET. postscripts_otherpkgs.bats extracts the condition from the script and runs it with the variable empty and with an http entry. Against the unfixed script the empty case is true. The block is inert when the variable is empty, because the loop inside it iterates an empty array, so the run of the postscript does not change. The guard is still wrong. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…baseurl
Line 297 of xCAT/postscripts/otherpkgs appends each http OTHERPKGDIR entry as "${dir} ,".
The list is split on the comma alone, so every url keeps the space, and the url repository
block writes "baseurl=<url> " into the yum repository file and "deb <url> " into the apt
source.
The separator no longer carries the space.
postscripts_otherpkgs.bats runs the split and the url repository block and compares the
generated baseurl and deb lines with the url. Against the unfixed script both differ by the
trailing space.
dnf 4.20 strips trailing whitespace from an ini value, and apt splits a source line on
whitespace, so no package manager reads the space today. The generated file is still wrong.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The local otherpkgs repository is written with the alias xcat-otherpkgs$localrepoindex, where localrepoindex is urlrepoindex plus the array index. Lines 833 and 843 of xCAT/postscripts/otherpkgs then refresh and delete xcat-otherpkgs$index. With http OTHERPKGDIR entries present urlrepoindex is not zero, so zypper names a repository that block did not add: the refresh fails for a repository that exists, and the delete removes another one. Both lines now use $localrepoindex. The log and the echo of a failed SDK repository add name $bname, which is what zypper ar used; they said bname without the $. postscripts_otherpkgs.bats drives the add, the refresh and the delete with zypper shadowed and urlrepoindex set to 2, and reads the alias back out of the repository file the script wrote. Against the unfixed script the refresh and the delete name xcat-otherpkgs0 while the file says xcat-otherpkgs2, and the SDK log carries no repository name. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The otherpkgs postscript writes two diagnostic lines for each package sublist: the split package array with its size, and the detected package manager. On a list with tens of entries these lines fill the updatenode output and the node log, and hide the install results. The two echo commands sit in xCAT/postscripts/otherpkgs, after the IFS split that builds pkgsarray, and no condition guards them. Every other diagnostic in the script runs only when VERBOSE is set. This change puts the two lines in the same "if [ $VERBOSE ]" block the rest of the script uses. xCAT-test/bats/postscripts_otherpkgs.bats runs the split and the lines that follow it. One case asserts no output when VERBOSE is empty, and fails on the parent commit. A second case asserts both lines are still printed when VERBOSE is set. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
A failed package install through the rpm or dpkg fallback leaves no record in syslog. The same holds for the four package removal branches that run after the install. The postscript sets its return code, but a reader of the node log sees nothing, because the package manager output goes to the console only when VERBOSE is set. In xCAT/postscripts/otherpkgs the fallback install block, and the yum, zypper, apt and rpm removal blocks, set RETURNVAL on a non-zero status and stop there. Only the three repository install branches send a message with local4.err. This change adds the same local4.err message to the five branches that have none. Each message names the command that ran, as the repository install branches do. xCAT-test/bats/postscripts_otherpkgs.bats drives each block with a package manager that returns a failure. Two cases assert the message is present, and both fail on the parent commit. Two more assert the success path still logs "installed." and "removed." alone. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
Four cases in xCAT-test/bats/postscripts_otherpkgs.bats fail on the integrated tree. extract_shell_if_block reports that the upgrade anchor occurs 4 times and the url repository anchor occurs 2 times, where 1 is expected. "if [ $hasyum -eq 1 ]; then" opens the upgrade block and also the yum branch of the preremove, the install and the postremove blocks. "OTHERPKGDIR_INTERNET" matches the guard that opens the url repository block and an assignment inside that block. The earlier helper took the first match and said nothing. otherpkgs_block now passes NTH and TOTAL through to extract_shell_if_block. The two upgrade cases take occurrence 1 of 4, so a fifth identical line fails the test instead of moving it. The two url repository cases anchor on the guard line itself, which occurs once. Both blocks the earlier helper took are the blocks the assertions describe, so no case measured the wrong code. On the integrated tree bats -r xCAT-test/bats gives 113 ok and 0 not ok. On this branch, where the helper is not yet hardened, the file gives 18 ok. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
viniciusferrao
requested changes
Sep 17, 2026
viniciusferrao
left a comment
viniciusferrao
left a comment
Member
There was a problem hiding this comment.
Thanks @dhilst, I would only suggest a SLES test:
could run_zypper_local_repo() execute
the whole refresh/cleanup branch rather than individual lines? It
currently runs zypper sd even in the success case, so it doesn’t
check that a successful refresh keeps the repository. The alias
assertions are useful; checking that distinction would complete them.
Otherwise... it's LGTM.
…both cases run_zypper_local_repo extracted six lines of the zypper branch of otherpkgs and evaluated each one in turn. The branch it measures is an if/else: the repository is deleted only when the refresh fails. Evaluating the lines separately ran the delete every time, so the test showed that zypper sd is reachable and never that a repository which refreshes is kept. The helper now evaluates the whole branch, from the #use zypper comment to the apt branch that follows, with pmatch lifted from the same file. The success case asserts rc=0 and no zypper sd; the failure case keeps its delete assertion. Flipping the refresh test in otherpkgs to "if [ $? -ne 0 ]" turns both cases red. The same mutation left the previous helper green, because it never evaluated that line. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
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
2 participants
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.
otherpkgspassed the whole package-manager transaction tologgeras one argument, so rsyslog escaped every newline to#012and cut the message at 8 KiB, which is whereFailed:and the summary appear; it then logged "<packages> installed." whether or not the install returned zero, at eight sites;[ -n "OTHERPKGDIR_INTERNET" ]omitted the$and always tested true; the list that guard walks left a space before each separator, which reached the generatedbaseurl; and the zypper path refreshed and removedxcat-otherpkgs$indexwhile the file had been written asxcat-otherpkgs$localrepoindex, which differ once an httpOTHERPKGDIRentry is present. Five commits, one per defect, each with its own assertions in the newxCAT-test/bats/postscripts_otherpkgs.bats, which fail against the parent commit.