[OE-Core][PATCH 0/2] oeqa/utils/gitarchive: fix tag name computation

[OE-Core][PATCH 0/2] oeqa/utils/gitarchive: fix tag name computation

[Alexis Lothoré]

Hello,
this series brings a fix to a sporadic tag push issue observed in
autobuilder. The bug is documented in bugzilla #15140 ([1]). Basically,
whenever the autobuilder creates a new tag on test results, it is only
aware of "local" tags, which is kind of faulty since used repository is a
shallow clone.

This series then brings two patches:
- a first one to introduce gitarchive tests as a safety net
- a second one to implement the fix by replacing "git tag" calls by "git
  ls-remote" calls where appropriate

[1] https://bugzilla.yoctoproject.org/show_bug.cgi?id=15140

Alexis Lothoré (2):
  oeqa/selftest: introduce gitarchive tests
  oeqa/utils/gitarchive: fix tag computation when creating archive

 .../oeqa/selftest/cases/gitarchivetests.py    | 96 +++++++++++++++++++
 meta/lib/oeqa/utils/gitarchive.py             |  6 +-
 2 files changed, 100 insertions(+), 2 deletions(-)
 create mode 100644 meta/lib/oeqa/selftest/cases/gitarchivetests.py

-- 
2.41.0

[OE-Core][PATCH 1/2] oeqa/selftest: introduce gitarchive tests

[Alexis Lothoré]

From: Alexis Lothoré <alexis.lothore@bootlin.com>

Add a test suite for gitarchive.py. For now, only introduce tests on
methods which needs to read existing tags

The tests rely on tmpdirs to create local, "fake" results repository in
order to allow basic git commands

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 .../oeqa/selftest/cases/gitarchivetests.py    | 96 +++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 meta/lib/oeqa/selftest/cases/gitarchivetests.py

diff --git a/meta/lib/oeqa/selftest/cases/gitarchivetests.py b/meta/lib/oeqa/selftest/cases/gitarchivetests.py
new file mode 100644
index 000000000000..4f7acd3311d5
--- /dev/null
+++ b/meta/lib/oeqa/selftest/cases/gitarchivetests.py
@@ -0,0 +1,96 @@
+#
+# Copyright OpenEmbedded Contributors
+#
+# SPDX-License-Identifier: MIT
+#
+
+import os
+import sys
+basepath = os.path.abspath(os.path.dirname(__file__) + '/../../../../../')
+lib_path = basepath + '/scripts/lib'
+sys.path = sys.path + [lib_path]
+import oeqa.utils.gitarchive as ga
+import tempfile
+import shutil
+import scriptutils
+from oeqa.selftest.case import OESelftestTestCase
+
+logger = scriptutils.logger_create('resulttool')
+
+def create_fake_repository(commit, tag_list=[]):
+    """ Create a testing git directory
+
+    Initialize a simple git repository with one initial commit, and as many
+    tags on this commit as listed in tag_list
+    Returns both git directory path and gitarchive git object
+    If commit is true, fake data will be commited, otherwise it will stay in staging area
+    If commit is true and tag_lsit is non empty, all tags in tag_list will be
+    created on the initial commit
+    Fake remote will also be added to make git ls-remote work
+    """
+    fake_data_file = "fake_data.txt"
+    tempdir = tempfile.mkdtemp(prefix='fake_results.')
+    repo = ga.init_git_repo(tempdir, False, False, logger)
+    repo.run_cmd(["remote", "add", "origin", "."])
+    with open(os.path.join(tempdir, fake_data_file), "w") as fake_data:
+        fake_data.write("Fake data")
+    if commit:
+        repo.run_cmd(["add", fake_data_file])
+        repo.run_cmd(["commit", "-m", "\"Add fake data\""])
+        for tag in tag_list:
+            repo.run_cmd(["tag", tag])
+    
+    return tempdir, repo
+
+def delete_fake_repository(path):
+    shutil.rmtree(path)
+
+def tag_exists(git_obj, target_tag):
+    for tag in git_obj.run_cmd(["tag"]).splitlines():
+        if target_tag == tag:
+            return True
+    return False
+
+class GitArchiveTests(OESelftestTestCase):
+    TEST_BRANCH="main"
+    TEST_COMMIT="0f7d5df"
+    TEST_COMMIT_COUNT="42"
+
+    def test_create_first_test_tag(self):
+        path, git_obj = create_fake_repository(False)
+        keywords = {'commit': self.TEST_COMMIT, 'branch': self.TEST_BRANCH, "commit_count": self.TEST_COMMIT_COUNT}
+        target_tag = f"{self.TEST_BRANCH}/{self.TEST_COMMIT_COUNT}-g{self.TEST_COMMIT}/0"
+
+        ga.gitarchive(path, path, True, False,
+                              "Results of {branch}:{commit}", "branch: {branch}\ncommit: {commit}", "{branch}",
+                              False, "{branch}/{commit_count}-g{commit}/{tag_number}",
+                              'Test run #{tag_number} of {branch}:{commit}', '',
+                              [], [], False, keywords, logger)
+        self.assertTrue(tag_exists(git_obj, target_tag), msg=f"Tag {target_tag} has not been created")
+        delete_fake_repository(path)
+
+    def test_create_second_test_tag(self):
+        first_tag = f"{self.TEST_BRANCH}/{self.TEST_COMMIT_COUNT}-g{self.TEST_COMMIT}/0"
+        second_tag = f"{self.TEST_BRANCH}/{self.TEST_COMMIT_COUNT}-g{self.TEST_COMMIT}/1"
+        keywords = {'commit': self.TEST_COMMIT, 'branch': self.TEST_BRANCH, "commit_count": self.TEST_COMMIT_COUNT}
+
+        path, git_obj = create_fake_repository(True, [first_tag])
+        ga.gitarchive(path, path, True, False,
+                              "Results of {branch}:{commit}", "branch: {branch}\ncommit: {commit}", "{branch}",
+                              False, "{branch}/{commit_count}-g{commit}/{tag_number}",
+                              'Test run #{tag_number} of {branch}:{commit}', '',
+                              [], [], False, keywords, logger)
+        self.assertTrue(tag_exists(git_obj, second_tag), msg=f"Second tag {second_tag} has not been created")
+        delete_fake_repository(path)
+
+    def test_get_revs_on_branch(self):
+        fake_tags_list=["main/10-g0f7d5df/0", "main/10-g0f7d5df/1", "foo/20-g2468f5d/0"]
+        tag_name = "{branch}/{commit_number}-g{commit}/{tag_number}"
+
+        path, git_obj = create_fake_repository(True, fake_tags_list)
+        revs = ga.get_test_revs(logger, git_obj, tag_name, branch="main")
+        self.assertEqual(len(revs), 1)
+        self.assertEqual(revs[0].commit, "0f7d5df")
+        self.assertEqual(len(revs[0].tags), 2)
+        self.assertEqual(revs[0].tags, ['main/10-g0f7d5df/0', 'main/10-g0f7d5df/1'])
+        delete_fake_repository(path)
-- 
2.41.0

[OE-Core][PATCH 2/2] oeqa/utils/gitarchive: fix tag computation when creating archive

[Alexis Lothoré]

From: Alexis Lothoré <alexis.lothore@bootlin.com>

Sporadic errors have been observed in autobuilder when trying to store new
tests results:

error: failed to push some refs to 'push.yoctoproject.org:yocto-testresults'
hint: Updates were rejected because the tag already exists in the remote.

The new tag name is generated by gitarchive based on known tags from the
repository (learnt with git tag). In autobuilder case, this repository is a
shallow clone, so git tag only returns most recent tags, which mean we
could miss some older tags which exist in remote but not locally. In this
case, gitarchive will likely create a tag which already exists in remote,
and so will fail to push

Fix this tag duplication by using git ls-remote to learn about existing
tags instead of git tag. Two places which wrongly read only local tags has
been identified in gitarchive:  expand_tag_strings and get_test_runs

Fixes [YOCTO #15140]

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 meta/lib/oeqa/utils/gitarchive.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/meta/lib/oeqa/utils/gitarchive.py b/meta/lib/oeqa/utils/gitarchive.py
index 6e8040eb5c96..73beafecb5fb 100644
--- a/meta/lib/oeqa/utils/gitarchive.py
+++ b/meta/lib/oeqa/utils/gitarchive.py
@@ -116,7 +116,8 @@ def expand_tag_strings(repo, name_pattern, msg_subj_pattern, msg_body_pattern,
         tag_re = tag_re.format(tag_number='(?P<tag_number>[0-9]{1,5})')
 
         keyws['tag_number'] = 0
-        for existing_tag in repo.run_cmd('tag').splitlines():
+        tags_refs = repo.run_cmd(['ls-remote', '--refs', '--tags', '-q'])
+        for existing_tag in ["".join(d.split()[1].split('/', 2)[2:]) for d in tags_refs.splitlines()]:
             match = re.match(tag_re, existing_tag)
 
             if match and int(match.group('tag_number')) >= keyws['tag_number']:
@@ -181,7 +182,8 @@ def get_test_runs(log, repo, tag_name, **kwargs):
 
     # Get a list of all matching tags
     tag_pattern = tag_name.format(**str_fields)
-    tags = repo.run_cmd(['tag', '-l', tag_pattern]).splitlines()
+    revs = repo.run_cmd(['ls-remote', '--refs', '--tags', 'origin', '-q', tag_pattern]).splitlines()
+    tags = ["".join(d.split()[1].split('/', 2)[2:]) for d in revs]
     log.debug("Found %d tags matching pattern '%s'", len(tags), tag_pattern)
 
     # Parse undefined fields from tag names
-- 
2.41.0

Re: [OE-Core][PATCH 2/2] oeqa/utils/gitarchive: fix tag computation when creating archive

[Richard Purdie]

On Fri, 2023-08-11 at 14:55 +0200, Alexis Lothoré via
lists.openembedded.org wrote:
> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> Sporadic errors have been observed in autobuilder when trying to store new
> tests results:
> 
> error: failed to push some refs to 'push.yoctoproject.org:yocto-testresults'
> hint: Updates were rejected because the tag already exists in the remote.
> 
> The new tag name is generated by gitarchive based on known tags from the
> repository (learnt with git tag). In autobuilder case, this repository is a
> shallow clone, so git tag only returns most recent tags, which mean we
> could miss some older tags which exist in remote but not locally. In this
> case, gitarchive will likely create a tag which already exists in remote,
> and so will fail to push
> 
> Fix this tag duplication by using git ls-remote to learn about existing
> tags instead of git tag. Two places which wrongly read only local tags has
> been identified in gitarchive:  expand_tag_strings and get_test_runs
> 
> Fixes [YOCTO #15140]
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> ---
>  meta/lib/oeqa/utils/gitarchive.py | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/lib/oeqa/utils/gitarchive.py b/meta/lib/oeqa/utils/gitarchive.py
> index 6e8040eb5c96..73beafecb5fb 100644
> --- a/meta/lib/oeqa/utils/gitarchive.py
> +++ b/meta/lib/oeqa/utils/gitarchive.py
> @@ -116,7 +116,8 @@ def expand_tag_strings(repo, name_pattern, msg_subj_pattern, msg_body_pattern,
>          tag_re = tag_re.format(tag_number='(?P<tag_number>[0-9]{1,5})')
>  
>          keyws['tag_number'] = 0
> -        for existing_tag in repo.run_cmd('tag').splitlines():
> +        tags_refs = repo.run_cmd(['ls-remote', '--refs', '--tags', '-q'])
> +        for existing_tag in ["".join(d.split()[1].split('/', 2)[2:]) for d in tags_refs.splitlines()]:
>              match = re.match(tag_re, existing_tag)
>  
>              if match and int(match.group('tag_number')) >= keyws['tag_number']:
> @@ -181,7 +182,8 @@ def get_test_runs(log, repo, tag_name, **kwargs):
>  
>      # Get a list of all matching tags
>      tag_pattern = tag_name.format(**str_fields)
> -    tags = repo.run_cmd(['tag', '-l', tag_pattern]).splitlines()
> +    revs = repo.run_cmd(['ls-remote', '--refs', '--tags', 'origin', '-q', tag_pattern]).splitlines()
> +    tags = ["".join(d.split()[1].split('/', 2)[2:]) for d in revs]
>      log.debug("Found %d tags matching pattern '%s'", len(tags), tag_pattern)
>  
>      # Parse undefined fields from tag names


I'm worrying a little that this change which recently merged might have
resulted in:

https://autobuilder.yoctoproject.org/typhoon/#/builders/133/builds/2099

?

I didn't take the test yet since that is causing other issues on the
autobuilder unrelated to the patch which I'm resolving first.

Cheers,

Richard

Re: [OE-Core][PATCH 2/2] oeqa/utils/gitarchive: fix tag computation when creating archive

[Alexis Lothoré]

Hello Richard,

On 8/15/23 23:13, Richard Purdie wrote:
> On Fri, 2023-08-11 at 14:55 +0200, Alexis Lothoré via
> lists.openembedded.org wrote:
>> From: Alexis Lothoré <alexis.lothore@bootlin.com>
>>
>> Sporadic errors have been observed in autobuilder when trying to store new
>> tests results:
>>
>> error: failed to push some refs to 'push.yoctoproject.org:yocto-testresults'
>> hint: Updates were rejected because the tag already exists in the remote.
>>
>> The new tag name is generated by gitarchive based on known tags from the
>> repository (learnt with git tag). In autobuilder case, this repository is a
>> shallow clone, so git tag only returns most recent tags, which mean we
>> could miss some older tags which exist in remote but not locally. In this
>> case, gitarchive will likely create a tag which already exists in remote,
>> and so will fail to push
>>
>> Fix this tag duplication by using git ls-remote to learn about existing
>> tags instead of git tag. Two places which wrongly read only local tags has
>> been identified in gitarchive:  expand_tag_strings and get_test_runs
>>
>> Fixes [YOCTO #15140]
>>
>> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
>> ---
>>  meta/lib/oeqa/utils/gitarchive.py | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/meta/lib/oeqa/utils/gitarchive.py b/meta/lib/oeqa/utils/gitarchive.py
>> index 6e8040eb5c96..73beafecb5fb 100644
>> --- a/meta/lib/oeqa/utils/gitarchive.py
>> +++ b/meta/lib/oeqa/utils/gitarchive.py
>> @@ -116,7 +116,8 @@ def expand_tag_strings(repo, name_pattern, msg_subj_pattern, msg_body_pattern,
>>          tag_re = tag_re.format(tag_number='(?P<tag_number>[0-9]{1,5})')
>>  
>>          keyws['tag_number'] = 0
>> -        for existing_tag in repo.run_cmd('tag').splitlines():
>> +        tags_refs = repo.run_cmd(['ls-remote', '--refs', '--tags', '-q'])
>> +        for existing_tag in ["".join(d.split()[1].split('/', 2)[2:]) for d in tags_refs.splitlines()]:
>>              match = re.match(tag_re, existing_tag)
>>  
>>              if match and int(match.group('tag_number')) >= keyws['tag_number']:
>> @@ -181,7 +182,8 @@ def get_test_runs(log, repo, tag_name, **kwargs):
>>  
>>      # Get a list of all matching tags
>>      tag_pattern = tag_name.format(**str_fields)
>> -    tags = repo.run_cmd(['tag', '-l', tag_pattern]).splitlines()
>> +    revs = repo.run_cmd(['ls-remote', '--refs', '--tags', 'origin', '-q', tag_pattern]).splitlines()
>> +    tags = ["".join(d.split()[1].split('/', 2)[2:]) for d in revs]
>>      log.debug("Found %d tags matching pattern '%s'", len(tags), tag_pattern)
>>  
>>      # Parse undefined fields from tag names
> 
> 
> I'm worrying a little that this change which recently merged might have
> resulted in:
> 
> https://autobuilder.yoctoproject.org/typhoon/#/builders/133/builds/2099

I may have taken the wrong assumption that directories manipulated with
gitarchive would always have at least one valid remote configured, but this log
tends to prove me wrong :/
Since the patch aimed to fix a sporadic issue but now leads to another
systematic issue, please feel free to revert it while I ensure about the
scenario and find a proper fix

Kind regards,
Alexis

> 
> ?
> 
> I didn't take the test yet since that is causing other issues on the
> autobuilder unrelated to the patch which I'm resolving first.
> 
> Cheers,
> 
> Richard
> 

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [OE-Core][PATCH 2/2] oeqa/utils/gitarchive: fix tag computation when creating archive

[Richard Purdie]

On Wed, 2023-08-16 at 08:36 +0200, Alexis Lothoré wrote:
> Hello Richard,
> 
> On 8/15/23 23:13, Richard Purdie wrote:
> > On Fri, 2023-08-11 at 14:55 +0200, Alexis Lothoré via
> > lists.openembedded.org wrote:
> > > From: Alexis Lothoré <alexis.lothore@bootlin.com>
> > > 
> > > Sporadic errors have been observed in autobuilder when trying to store new
> > > tests results:
> > > 
> > > error: failed to push some refs to 'push.yoctoproject.org:yocto-testresults'
> > > hint: Updates were rejected because the tag already exists in the remote.
> > > 
> > > The new tag name is generated by gitarchive based on known tags from the
> > > repository (learnt with git tag). In autobuilder case, this repository is a
> > > shallow clone, so git tag only returns most recent tags, which mean we
> > > could miss some older tags which exist in remote but not locally. In this
> > > case, gitarchive will likely create a tag which already exists in remote,
> > > and so will fail to push
> > > 
> > > Fix this tag duplication by using git ls-remote to learn about existing
> > > tags instead of git tag. Two places which wrongly read only local tags has
> > > been identified in gitarchive:  expand_tag_strings and get_test_runs
> > > 
> > > Fixes [YOCTO #15140]
> > > 
> > > Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> > > ---
> > >  meta/lib/oeqa/utils/gitarchive.py | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/meta/lib/oeqa/utils/gitarchive.py b/meta/lib/oeqa/utils/gitarchive.py
> > > index 6e8040eb5c96..73beafecb5fb 100644
> > > --- a/meta/lib/oeqa/utils/gitarchive.py
> > > +++ b/meta/lib/oeqa/utils/gitarchive.py
> > > @@ -116,7 +116,8 @@ def expand_tag_strings(repo, name_pattern, msg_subj_pattern, msg_body_pattern,
> > >          tag_re = tag_re.format(tag_number='(?P<tag_number>[0-9]{1,5})')
> > >  
> > >          keyws['tag_number'] = 0
> > > -        for existing_tag in repo.run_cmd('tag').splitlines():
> > > +        tags_refs = repo.run_cmd(['ls-remote', '--refs', '--tags', '-q'])
> > > +        for existing_tag in ["".join(d.split()[1].split('/', 2)[2:]) for d in tags_refs.splitlines()]:
> > >              match = re.match(tag_re, existing_tag)
> > >  
> > >              if match and int(match.group('tag_number')) >= keyws['tag_number']:
> > > @@ -181,7 +182,8 @@ def get_test_runs(log, repo, tag_name, **kwargs):
> > >  
> > >      # Get a list of all matching tags
> > >      tag_pattern = tag_name.format(**str_fields)
> > > -    tags = repo.run_cmd(['tag', '-l', tag_pattern]).splitlines()
> > > +    revs = repo.run_cmd(['ls-remote', '--refs', '--tags', 'origin', '-q', tag_pattern]).splitlines()
> > > +    tags = ["".join(d.split()[1].split('/', 2)[2:]) for d in revs]
> > >      log.debug("Found %d tags matching pattern '%s'", len(tags), tag_pattern)
> > >  
> > >      # Parse undefined fields from tag names
> > 
> > 
> > I'm worrying a little that this change which recently merged might have
> > resulted in:
> > 
> > https://autobuilder.yoctoproject.org/typhoon/#/builders/133/builds/2099
> 
> I may have taken the wrong assumption that directories manipulated with
> gitarchive would always have at least one valid remote configured, but this log
> tends to prove me wrong :/
> Since the patch aimed to fix a sporadic issue but now leads to another
> systematic issue, please feel free to revert it while I ensure about the
> scenario and find a proper fix

Thanks, I was wondering why one performance worker was affected and the
other was not. I guess one has the remote, maybe from when we've tried
to fix something and the other doesn't.

I've reverted for now until we get to the bottom of it as the stream of
failures would be a pain.

Cheers,

Richard