Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not consider failure to sync fatal #238

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kalikiana
Copy link
Member

We still log the error but we assume it is okay to fail.

See: https://progress.opensuse.org/issues/112871

script/rsync.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, but this would ignore any error in read_files.sh?
I don't have a good idea how to find out the cause, but ignoring all errors might hide real problems in the future

@perlpunk
Copy link
Contributor

perlpunk commented Dec 22, 2023

So read_files.sh is doing

rsync -4 --list-only $rsync_pwd_option dist.suse.de::repos/SUSE:/SLE-15-SP6:/GA:/Staging:/A/images/repo/
 | grep -P '...' | awk '...' | grep -v IGNOREPATTERN
 | grep -E 'Module-Basesystem|...' | grep -E '...'  >> .../files_repo.lst

and if one of the grep fails, the script fails.
Maybe there is a way to make only the grep commands non-fatal.

@perlpunk
Copy link
Contributor

perlpunk commented Dec 22, 2023

This should work:

rsync ... | (grep ... | grep ... >>logfile || true)

I tested it with a mocked rsync.
So if rsync has a problem, the script would still fail.

@okurz
Copy link
Member

okurz commented Jan 11, 2024

How would this look in reality? I would like to see two cases, one with error and one without. And what about the explicit log message we planned?

@perlpunk
Copy link
Contributor

This is a line from a genererated read_files.sh:

rsync -4 --list-only $rsync_pwd_option dist.suse.de::repos/SUSE:/SLE-15-SP6:/Update:/BCI/images//*repo/SLE-BCI-**
 | grep -P 'Media1(.license)?$'
 | awk '{ $1=$2=$3=$4=""; print substr($0,5); } '
 | grep -v IGNOREPATTERN
 | grep -E '' 
| grep -E 'aarch64|ppc64le|s390x|x86_64'  >> /opt/openqa-trigger-from-ibs/SUSE:SLE-15-SP6:Update:BCI/files_repo_SLE-BCI-*.lst

And I would guess that some of the grep commands is failing, but we probably want to ignore the return value of grep here.

@@ -674,7 +674,7 @@ def gen_read_files(self, f):
if self.assets and self.assets_flavor:
for k, v in self.asset_folders.items():
self.p(
"""rsync -4 --list-only $rsync_pwd_option PRODUCTPATH/{}/*{} | awk '{{ $1=$2=$3=$4=""; print substr($0,5); }}' >> __envsub/files_asset.lst""".format(
"""rsync -4 --list-only $rsync_pwd_option PRODUCTPATH/{}/*{} | awk '{{ $1=$2=$3=$4=""; print substr($0,5); }}' >> __envsub/files_asset.lst || true""".format(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in jitsi. This may be the wrong part of the generator, see #238 (comment) - I will look into that after coming up with a fake rsync that produces successful or error output, and see how grep and awk respond to that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seems the read_files_repo value in cfg.py:23 is the basis for that rsync call and the p function in scriptgen.py actually replaces the target file that is being piped into i.e. __Envsub/files_repo.lst

We still log the error but we assume it is okay to fail.

See: https://progress.opensuse.org/issues/112871
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guess I still don't get the full picture of how this repo works but the tests look like we've discussed (and you found an even better way to mock the commands).

@kalikiana kalikiana marked this pull request as draft August 27, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants