Re: [PATCH] s3-mirror: avoid potential unexpected shell-expansion, dedup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Mar 22, 2020 at 12:49:18PM +0100, Pavel Raiskup wrote:
> Note that I'd like to propose followup for this.

+1 to your patch, looks correct and it makes sense to deduplicate all
the exclude lines.

> 1. CMD1 could IMO safely exclude _only_ repomd.xml files.
> 2, CMD2 could safely _only_ sync/replace repomd.xml files.
> 3, then caches need to be invalidated, so we are sure we can --delete
> 4. CMD3 is keep as-is
> 
> The current approach is fine as well, but CMD2 must take unnecessarily
> long time to calculate (do we have stats?).
> 
> There actually is slight risk, in current $CMD2, that 'repomd.xml' is
> updated slightly faster than the rest of metadata.  So repomd.xml for
> slight moment references non-existing metadata files...  which might
> result in some unnecessary dnf fallbacks (that should succeed, but still).
> 
> WDYT?

Sounds correct. If I understood it correctly you are proposing to
exclude only the file 'repomd.xml' instead of the complete 'repodata'
directory. Makes sense. Especially the change to step 2 should reduce
the required time. Good idea.

		Adrian

> On Sunday, March 22, 2020 12:30:08 PM CET Pavel Raiskup wrote:
> > Doing this has risk that '*' expands to directories:
> > 
> >     $> x='echo *'
> >     $> $x
> >     bin boot dev etc home
> > 
> > So it is better to use bash array (we have bash shebang anyways):
> >     $> x=( echo '*' )
> >     $> "${x[@]}"
> >     *
> > 
> > Also de-duplicate few things so it is easier to concentrate on the code.
> > ---
> >  roles/s3-mirror/files/s3.sh | 288 +++++++++++++-----------------------
> >  1 file changed, 106 insertions(+), 182 deletions(-)
> > 
> > diff --git a/roles/s3-mirror/files/s3.sh b/roles/s3-mirror/files/s3.sh
> > index c70defb52..163e3dc36 100644
> > --- a/roles/s3-mirror/files/s3.sh
> > +++ b/roles/s3-mirror/files/s3.sh
> > @@ -3,207 +3,131 @@
> >  # LGPL
> >  # Author: Rick Elrod <relrod@xxxxxxxxxx>
> >  
> > -# first run this command that syncs, but does not delete.
> > +base_cmd=(
> > +  aws s3 sync
> > +  --no-follow-symlinks
> > +  --exclude "*/.snapshot/*"
> > +  --exclude "*/source/*"
> > +  --exclude "*/SRPMS/*"
> > +  --exclude "*/debug/*"
> > +  --exclude "*/beta/*"
> > +  --exclude "*/ppc/*"
> > +  --exclude "*/ppc64/*"
> > +  --exclude "*/repoview/*"
> > +  --exclude "*/Fedora/*"
> > +  --exclude "*/EFI/*"
> > +  --exclude "*/core/*"
> > +  --exclude "*/extras/*"
> > +  --exclude "*/LiveOS/*"
> > +  --exclude "*/development/rawhide/*"
> > +  --exclude "*/releases/8/*"
> > +  --exclude "*/releases/9/*"
> > +  --exclude "*/releases/10/*"
> > +  --exclude "*/releases/11/*"
> > +  --exclude "*/releases/12/*"
> > +  --exclude "*/releases/13/*"
> > +  --exclude "*/releases/14/*"
> > +  --exclude "*/releases/15/*"
> > +  --exclude "*/releases/16/*"
> > +  --exclude "*/releases/17/*"
> > +  --exclude "*/releases/18/*"
> > +  --exclude "*/releases/19/*"
> > +  --exclude "*/releases/20/*"
> > +  --exclude "*/releases/21/*"
> > +  --exclude "*/releases/22/*"
> > +  --exclude "*/releases/23/*"
> > +  --exclude "*/releases/24/*"
> > +  --exclude "*/releases/25/*"
> > +  --exclude "*/releases/26/*"
> > +  --exclude "*/releases/27/*"
> > +  --exclude "*/releases/28/*"
> > +  --exclude "*/releases/29/*"
> > +  --exclude "*/updates/8/*"
> > +  --exclude "*/updates/9/*"
> > +  --exclude "*/updates/10/*"
> > +  --exclude "*/updates/11/*"
> > +  --exclude "*/updates/12/*"
> > +  --exclude "*/updates/13/*"
> > +  --exclude "*/updates/14/*"
> > +  --exclude "*/updates/15/*"
> > +  --exclude "*/updates/16/*"
> > +  --exclude "*/updates/17/*"
> > +  --exclude "*/updates/18/*"
> > +  --exclude "*/updates/19/*"
> > +  --exclude "*/updates/20/*"
> > +  --exclude "*/updates/21/*"
> > +  --exclude "*/updates/22/*"
> > +  --exclude "*/updates/23/*"
> > +  --exclude "*/updates/24/*"
> > +  --exclude "*/updates/25/*"
> > +  --exclude "*/updates/26/*"
> > +  --exclude "*/updates/27/*"
> > +  --exclude "*/updates/28/*"
> > +  --exclude "*/updates/29/*"
> > +  --exclude "*/updates/testing/8/*"
> > +  --exclude "*/updates/testing/9/*"
> > +  --exclude "*/updates/testing/10/*"
> > +  --exclude "*/updates/testing/11/*"
> > +  --exclude "*/updates/testing/12/*"
> > +  --exclude "*/updates/testing/13/*"
> > +  --exclude "*/updates/testing/14/*"
> > +  --exclude "*/updates/testing/15/*"
> > +  --exclude "*/updates/testing/16/*"
> > +  --exclude "*/updates/testing/17/*"
> > +  --exclude "*/updates/testing/18/*"
> > +  --exclude "*/updates/testing/19/*"
> > +  --exclude "*/updates/testing/20/*"
> > +  --exclude "*/updates/testing/21/*"
> > +  --exclude "*/updates/testing/22/*"
> > +  --exclude "*/updates/testing/23/*"
> > +  --exclude "*/updates/testing/24/*"
> > +  --exclude "*/updates/testing/25/*"
> > +  --exclude "*/updates/testing/26/*"
> > +  --exclude "*/updates/testing/27/*"
> > +  --exclude "*/updates/testing/28/*"
> > +  --exclude "*/updates/testing/29/*"
> > +)
> > +
> > +# First run this command that syncs, but does not delete.
> >  # It also excludes repodata. 
> > -CMD1="aws s3 sync                   \
> > -  --exclude */repodata/*           \
> > -  --exclude */.snapshot/*          \
> > -  --exclude */source/*             \
> > -  --exclude */SRPMS/*              \
> > -  --exclude */debug/*              \
> > -  --exclude */beta/*               \
> > -  --exclude */ppc/*                \
> > -  --exclude */ppc64/*              \
> > -  --exclude */repoview/*           \
> > -  --exclude */Fedora/*             \
> > -  --exclude */EFI/*                \
> > -  --exclude */core/*               \
> > -  --exclude */extras/*             \
> > -  --exclude */LiveOS/*             \
> > -  --exclude */development/rawhide/* \
> > -  --exclude */releases/8/*         \
> > -  --exclude */releases/9/*         \
> > -  --exclude */releases/10/*        \
> > -  --exclude */releases/11/*        \
> > -  --exclude */releases/12/*        \
> > -  --exclude */releases/13/*        \
> > -  --exclude */releases/14/*        \
> > -  --exclude */releases/15/*        \
> > -  --exclude */releases/16/*        \
> > -  --exclude */releases/17/*        \
> > -  --exclude */releases/18/*        \
> > -  --exclude */releases/19/*        \
> > -  --exclude */releases/20/*        \
> > -  --exclude */releases/21/*        \
> > -  --exclude */releases/22/*        \
> > -  --exclude */releases/23/*        \
> > -  --exclude */releases/24/*        \
> > -  --exclude */releases/25/*        \
> > -  --exclude */releases/26/*        \
> > -  --exclude */releases/27/*        \
> > -  --exclude */releases/28/*        \
> > -  --exclude */releases/29/*        \
> > -  --exclude */updates/8/*          \
> > -  --exclude */updates/9/*          \
> > -  --exclude */updates/10/*         \
> > -  --exclude */updates/11/*         \
> > -  --exclude */updates/12/*         \
> > -  --exclude */updates/13/*         \
> > -  --exclude */updates/14/*         \
> > -  --exclude */updates/15/*         \
> > -  --exclude */updates/16/*         \
> > -  --exclude */updates/17/*         \
> > -  --exclude */updates/18/*         \
> > -  --exclude */updates/19/*         \
> > -  --exclude */updates/20/*         \
> > -  --exclude */updates/21/*         \
> > -  --exclude */updates/22/*         \
> > -  --exclude */updates/23/*         \
> > -  --exclude */updates/24/*         \
> > -  --exclude */updates/25/*         \
> > -  --exclude */updates/26/*         \
> > -  --exclude */updates/27/*         \
> > -  --exclude */updates/28/*         \
> > -  --exclude */updates/29/*         \
> > -  --exclude */updates/testing/8/*  \
> > -  --exclude */updates/testing/9/*  \
> > -  --exclude */updates/testing/10/* \
> > -  --exclude */updates/testing/11/* \
> > -  --exclude */updates/testing/12/* \
> > -  --exclude */updates/testing/13/* \
> > -  --exclude */updates/testing/14/* \
> > -  --exclude */updates/testing/15/* \
> > -  --exclude */updates/testing/16/* \
> > -  --exclude */updates/testing/17/* \
> > -  --exclude */updates/testing/18/* \
> > -  --exclude */updates/testing/19/* \
> > -  --exclude */updates/testing/20/* \
> > -  --exclude */updates/testing/21/* \
> > -  --exclude */updates/testing/22/* \
> > -  --exclude */updates/testing/23/* \
> > -  --exclude */updates/testing/24/* \
> > -  --exclude */updates/testing/25/* \
> > -  --exclude */updates/testing/26/* \
> > -  --exclude */updates/testing/27/* \
> > -  --exclude */updates/testing/28/* \
> > -  --exclude */updates/testing/29/* \
> > -  --no-follow-symlinks             \
> > -  "
> > -  #--dryrun                         \
> > +CMD1=( "${base_cmd[@]}" --exclude "*/repodata/*" )
> >  
> >  # Next we run this command which also includes repodata.
> > -CMD2="aws s3 sync                   \
> > -  --exclude */.snapshot/*          \
> > -  --exclude */source/*             \
> > -  --exclude */SRPMS/*              \
> > -  --exclude */debug/*              \
> > -  --exclude */beta/*               \
> > -  --exclude */ppc/*                \
> > -  --exclude */ppc64/*              \
> > -  --exclude */repoview/*           \
> > -  --exclude */Fedora/*             \
> > -  --exclude */EFI/*                \
> > -  --exclude */core/*               \
> > -  --exclude */extras/*             \
> > -  --exclude */LiveOS/*             \
> > -  --exclude */development/rawhide/* \
> > -  --exclude */releases/8/*         \
> > -  --exclude */releases/9/*         \
> > -  --exclude */releases/10/*        \
> > -  --exclude */releases/11/*        \
> > -  --exclude */releases/12/*        \
> > -  --exclude */releases/13/*        \
> > -  --exclude */releases/14/*        \
> > -  --exclude */releases/15/*        \
> > -  --exclude */releases/16/*        \
> > -  --exclude */releases/17/*        \
> > -  --exclude */releases/18/*        \
> > -  --exclude */releases/19/*        \
> > -  --exclude */releases/20/*        \
> > -  --exclude */releases/21/*        \
> > -  --exclude */releases/22/*        \
> > -  --exclude */releases/23/*        \
> > -  --exclude */releases/24/*        \
> > -  --exclude */releases/25/*        \
> > -  --exclude */releases/26/*        \
> > -  --exclude */releases/27/*        \
> > -  --exclude */releases/28/*        \
> > -  --exclude */releases/29/*        \
> > -  --exclude */updates/8/*          \
> > -  --exclude */updates/9/*          \
> > -  --exclude */updates/10/*         \
> > -  --exclude */updates/11/*         \
> > -  --exclude */updates/12/*         \
> > -  --exclude */updates/13/*         \
> > -  --exclude */updates/14/*         \
> > -  --exclude */updates/15/*         \
> > -  --exclude */updates/16/*         \
> > -  --exclude */updates/17/*         \
> > -  --exclude */updates/18/*         \
> > -  --exclude */updates/19/*         \
> > -  --exclude */updates/20/*         \
> > -  --exclude */updates/21/*         \
> > -  --exclude */updates/22/*         \
> > -  --exclude */updates/23/*         \
> > -  --exclude */updates/24/*         \
> > -  --exclude */updates/25/*         \
> > -  --exclude */updates/26/*         \
> > -  --exclude */updates/27/*         \
> > -  --exclude */updates/28/*         \
> > -  --exclude */updates/29/*         \
> > -  --exclude */updates/testing/8/*  \
> > -  --exclude */updates/testing/9/*  \
> > -  --exclude */updates/testing/10/* \
> > -  --exclude */updates/testing/11/* \
> > -  --exclude */updates/testing/12/* \
> > -  --exclude */updates/testing/13/* \
> > -  --exclude */updates/testing/14/* \
> > -  --exclude */updates/testing/15/* \
> > -  --exclude */updates/testing/16/* \
> > -  --exclude */updates/testing/17/* \
> > -  --exclude */updates/testing/18/* \
> > -  --exclude */updates/testing/19/* \
> > -  --exclude */updates/testing/20/* \
> > -  --exclude */updates/testing/21/* \
> > -  --exclude */updates/testing/22/* \
> > -  --exclude */updates/testing/23/* \
> > -  --exclude */updates/testing/24/* \
> > -  --exclude */updates/testing/25/* \
> > -  --exclude */updates/testing/26/* \
> > -  --exclude */updates/testing/27/* \
> > -  --exclude */updates/testing/28/* \
> > -  --exclude */updates/testing/29/* \
> > -  --no-follow-symlinks             \
> > -  "
> > -  #--dryrun                         \
> > +CMD2=( "${base_cmd[@]}" )
> > +
> > +# Then we delete old RPMs and old metadata (but after invalidating caches).
> > +CMD3=( "${base_cmd[@]}" --delete )
> > +
> > +S3_MIRROR=s3-mirror-us-west-1-02.fedoraproject.org
> > +DIST_ID=E2KJMDC0QAJDMU
> >  
> >  # Sync EPEL
> > -#echo $CMD /srv/pub/epel/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/epel/
> > +#echo $CMD /srv/pub/epel/ s3://$S3_MIRROR/pub/epel/
> >  echo "Starting EPEL sync at $(date)" >> /var/log/s3-mirror/timestamps
> > -$CMD1 /srv/pub/epel/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/epel/
> > -$CMD2 /srv/pub/epel/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/epel/
> > +"${CMD1[@]}" /srv/pub/epel/ "s3://$S3_MIRROR/pub/epel/"
> > +"${CMD2[@]}" /srv/pub/epel/ "s3://$S3_MIRROR/pub/epel/"
> >  echo "Ending EPEL sync at $(date)" >> /var/log/s3-mirror/timestamps
> >  for file in $(echo /srv/pub/epel/6/*/repodata/repomd.xml | sed 's#/srv##g'); do
> > -  aws cloudfront create-invalidation --distribution-id E2KJMDC0QAJDMU --paths "$file"
> > +  aws cloudfront create-invalidation --distribution-id "$DIST_ID" --paths "$file"
> >  done
> >  
> >  for file in $(echo /srv/pub/epel/7/*/repodata/repomd.xml | sed 's#/srv##g'); do
> > -  aws cloudfront create-invalidation --distribution-id E2KJMDC0QAJDMU --paths "$file"
> > +  aws cloudfront create-invalidation --distribution-id "$DIST_ID" --paths "$file"
> >  done
> >  
> >  for file in $(echo /srv/pub/epel/8/*/repodata/repomd.xml | sed 's#/srv##g'); do
> > -  aws cloudfront create-invalidation --distribution-id E2KJMDC0QAJDMU --paths "$file"
> > +  aws cloudfront create-invalidation --distribution-id "$DIST_ID" --paths "$file"
> >  done
> > -$CMD2 --delete /srv/pub/epel/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/epel/
> > +"${CMD3[@]}" /srv/pub/epel/ "s3://$S3_MIRROR/pub/epel/"
> >  
> >  # Sync Fedora
> > -#echo $CMD /srv/pub/fedora/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/fedora/
> > +#echo $CMD /srv/pub/fedora/ s3://$S3_MIRROR/pub/fedora/
> >  echo "Starting Fedora sync at $(date)" >> /var/log/s3-mirror/timestamps
> > -$CMD1 /srv/pub/fedora/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/fedora/
> > -$CMD2 /srv/pub/fedora/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/fedora/
> > +"${CMD1[@]}" /srv/pub/fedora/ "s3://$S3_MIRROR/pub/fedora/"
> > +"${CMD2[@]}" /srv/pub/fedora/ "s3://$S3_MIRROR/pub/fedora/"
> >  echo "Ending Fedora sync at $(date)" >> /var/log/s3-mirror/timestamps
> >  
> >  for file in $(echo /srv/pub/fedora/linux/updates/*/*/*/repodata/repomd.xml | sed 's#/srv##g'); do
> > -  aws cloudfront create-invalidation --distribution-id E2KJMDC0QAJDMU --paths "$file"
> > +  aws cloudfront create-invalidation --distribution-id "$DIST_ID" --paths "$file"
> >  done
> > -$CMD2 --delete /srv/pub/fedora/ s3://s3-mirror-us-west-1-02.fedoraproject.org/pub/fedora/
> > +"${CMD3[@]}" /srv/pub/fedora/ s3://$S3_MIRROR/pub/fedora/
> > -- 
> > 2.25.1
> > _______________________________________________
> > infrastructure mailing list -- infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
> > To unsubscribe send an email to infrastructure-leave@xxxxxxxxxxxxxxxxxxxxxxx
> > Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives: https://lists.fedoraproject.org/archives/list/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
> > 
> 
> 
> 
> _______________________________________________
> infrastructure mailing list -- infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
> To unsubscribe send an email to infrastructure-leave@xxxxxxxxxxxxxxxxxxxxxxx
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives: https://lists.fedoraproject.org/archives/list/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx

		Adrian

-- 
Adrian Reber <adrian@xxxxxxxx>            http://lisas.de/~adrian/
	"It's hard to believe that something which is neither seen nor felt can
do so much harm."
	"That's true.  But an idea can't be seen or felt.  And that's what kept
the Troglytes in the mines all these centuries.  A mistaken idea."
		-- Vanna and Kirk, "The Cloud Minders", stardate 5819.0
_______________________________________________
infrastructure mailing list -- infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to infrastructure-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/infrastructure@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Development]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]

  Powered by Linux