Re: [PATCH 1/2] report: safely update the result.xml file

From: Zorro Lang <zlang@redhat.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>, fstests@vger.kernel.org
Subject: Re: [PATCH 1/2] report: safely update the result.xml file
Date: Sat, 8 Jul 2023 00:17:47 +0800	[thread overview]
Message-ID: <20230707161747.efz7azf6jta4e27w@zlang-mailbox> (raw)
In-Reply-To: <20230707150130.GA11442@frogsfrogsfrogs>

On Fri, Jul 07, 2023 at 08:01:30AM -0700, Darrick J. Wong wrote:
> On Thu, Jul 06, 2023 at 04:42:31PM -0400, Theodore Ts'o wrote:
> > After every single test, we rewrite result.xml from scratch.  This
> > ensures that the XML file is always in a valid, parseable state, even
> > if the check script is killed or the machine crashes in the middle of
> > a test.
> > 
> > If the test is being run in a Cloud VM as a "spot" (Amazon, Azure, or
> > GCE) or "preemptible" (Oracle) instance, it is possible that the VM
> > can be halted whenever the Cloud provider needs the capacity for
> > customers who are willing to pay full price.  ("Spot" instances can be
> > 60% to 90% cheaper --- allowing the frugal kernel developer to get up
> > to 10 times more testing for the same amount of money.  :-)
> > 
> > Since a "spot" VM can get terminated at any time, it is possible for
> > the VM to be terminated immediately after a test has completed and
> > while the result.xml file is in the middle of being written out.  In
> > that case, the result.xml file could partially written, resulting in
> > an invalid result.xml file and lost information about the tests run
> > before the VM was terminated.
> > 
> > To address this race, write the new result.xml file as result.xml.new,
> > and only rename it to result.xml after the XML file is fully written
> > out.
> > 
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  common/report | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/common/report b/common/report
> > index 9bfa09ecc..3ad14f94e 100644
> > --- a/common/report
> > +++ b/common/report
> > @@ -109,13 +109,15 @@ _xunit_make_section_report()
> >  	local notrun_count="$4"
> >  	local sect_time="$5"
> >  	local timestamp
> > +	local tmp_fn="$REPORT_DIR/result.xml.new"
> > +	local out_fn="$REPORT_DIR/result.xml"
> >  
> >  	if [ $sect_name == '-no-sections-' ]; then
> >  		sect_name='global'
> >  	fi
> >  	local report=$tmp.report.xunit.$sect_name.xml
> >  	# Header
> > -	echo "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" > $REPORT_DIR/result.xml
> > +	echo "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" > "$tmp_fn"
> 
> Nit: You might want to rm -f $tmp_fn first to reduce the nastiness if
> someone plants a named pipe at that path.
> 
> >  	if [ -n "$test_start_time" ]; then
> >  		timestamp="$(date -Iseconds --date="$test_start_time")"
> >  	else
> > @@ -123,7 +125,7 @@ _xunit_make_section_report()
> >  	fi
> >  
> >  	local fstests_ns="https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git"
> > -	cat >> $REPORT_DIR/result.xml << ENDL
> > +	cat >> "$tmp_fn" << ENDL
> >  <testsuite
> >   xmlns="$fstests_ns"
> >   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
> > @@ -142,19 +144,20 @@ ENDL
> >  	__generate_report_vars
> >  
> >  	# Properties
> > -	echo -e "\t<properties>" >> $REPORT_DIR/result.xml
> > +	echo -e "\t<properties>" >> "$tmp_fn"
> >  	(for key in "${!REPORT_VARS[@]}"; do
> >  		_xunit_add_property "$key" "${REPORT_VARS["$key"]}"
> >  	done;
> >  	for p in "${REPORT_ENV_LIST[@]}"; do
> >  		_xunit_add_property "$p" "${!p}"
> > -	done) | sort >> $REPORT_DIR/result.xml
> > -	echo -e "\t</properties>" >> $REPORT_DIR/result.xml
> > +	done) | sort >> "$tmp_fn"
> > +	echo -e "\t</properties>" >> "$tmp_fn"
> >  	if [ -f $report ]; then
> > -		cat $report >> $REPORT_DIR/result.xml
> > +		cat $report >> "$tmp_fn"
> >  	fi
> > -	echo "</testsuite>" >> $REPORT_DIR/result.xml
> > -	echo "Xunit report: $REPORT_DIR/result.xml"
> > +	echo "</testsuite>" >> "$tmp_fn"
> > +	mv "$tmp_fn" "$out_fn"
> 
> Second nit: Make sure we actually wrote tmp_fn before blowing away the
> old report.
> 
> sync "$tmp_fn" && mv "$tmp_fn" "$out_fn"

I can help to add this when I merge it, if Ted doesn't want to change
more than that.

Just curious, will renameat2 ignore data still in cached? I never did things
likes "sync && mv" before, is there any known issue or it's as expected?

Thanks,
Zorro

> 
> With that fixed,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> > +	echo "Xunit report: $out_fn"
> >  }
> >  
> >  _xunit_make_testcase_report()
> > -- 
> > 2.31.0
> > 
> 