Re: [PATCH] s3-mirror: avoid potential unexpected shell-expansion, dedup
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
> >
> > -# 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
Re: [PATCH] s3-mirror: avoid potential unexpected shell-expansion, dedup
Note that I'd like to propose followup for this.
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?
Pavel
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
>
> -# 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/* \
[PATCH] s3-mirror: avoid potential unexpected shell-expansion, dedup
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
-# 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/* \
-
