| @tristanc_:matrix.org | Albin Vass: have you tried something like https://github.com/TristanCacqueray/zuul-nix/blob/master/zuul.nix#L3-L7 ? | 12:58 |
|---|---|---|
| @sylvass:albinvass.se | tristanC: yeah, but the version in the flake could easily be forgotten and would stay at 10.0.0. So I might as well keep the build outside of the zuul-client repo | 13:15 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/nodepool] 914658: Fix inheritance of delete-after-upload option https://review.opendev.org/c/zuul/nodepool/+/914658 | 16:19 | |
| -@gerrit:opendev.org- Zuul merged on behalf of James E. Blair https://matrix.to/#/@jim:acmegating.com: [zuul/nodepool] 914658: Fix inheritance of delete-after-upload option https://review.opendev.org/c/zuul/nodepool/+/914658 | 17:57 | |
| @vlotorev:matrix.org | Hi, | 20:48 |
| this commit in zuul/zuul started writing `.git/packed-refs` file - https://opendev.org/zuul/zuul/commit/518194af1dca82dd42ae5b599cb2e1d817031068?style=split&whitespace=show-all&show-outdated=#diff-2e459048c0e4a2f61e4975290ed881f0e3ddc31c | ||
| It seems zuul generates .git/packed-refs file improperly, `git describe` in git repo directory returns: | ||
| ``` | ||
| fatal: No annotated tags can describe 'db62e....'. | ||
| However, there were unannotated tags: try --tags. | ||
| ``` | ||
| Indeed proper `.git/packed-refs` has extra line starting with `^<SHA>` after lines with tag while packed-refs generated by zuul doesn't `^<SHA>` lines. | ||
| This issue is not reproduced on zuul.opendev.org because git repo prepared by merger is uploaded to CI nodes using `prepare-workspace-git` role. This role does `git push` and .git/packed-refs is regenerated on CI node. | ||
| I found this issue while using `prepare-workspace` role. This roles uses rsync to upload files and `.git/packed-refs` is copied 'as is' on node and hence `git describe` fails while running playbook. | ||
| @vlotorev:matrix.org | * Hi, | 20:49 |
| this commit in zuul/zuul started writing `.git/packed-refs` file - https://opendev.org/zuul/zuul/commit/518194af1dca82dd42ae5b599cb2e1d817031068?style=split&whitespace=show-all&show-outdated=#diff-2e459048c0e4a2f61e4975290ed881f0e3ddc31c | ||
| It seems zuul generates .git/packed-refs file improperly, `git describe` in git repo with annotated tags fails with error: | ||
| ``` | ||
| fatal: No annotated tags can describe 'db62e....'. | ||
| However, there were unannotated tags: try --tags. | ||
| ``` | ||
| Indeed proper `.git/packed-refs` has extra line starting with `^<SHA>` after lines with tag while packed-refs generated by zuul doesn't `^<SHA>` lines. | ||
| This issue is not reproduced on zuul.opendev.org because git repo prepared by merger is uploaded to CI nodes using `prepare-workspace-git` role. This role does `git push` and .git/packed-refs is regenerated on CI node. | ||
| I found this issue while using `prepare-workspace` role. This roles uses rsync to upload files and `.git/packed-refs` is copied 'as is' on node and hence `git describe` fails while running playbook. | ||
| @vlotorev:matrix.org | * Hi, | 20:50 |
| this commit in zuul/zuul started writing `.git/packed-refs` file - https://opendev.org/zuul/zuul/commit/518194af1dca82dd42ae5b599cb2e1d817031068?style=split&whitespace=show-all&show-outdated=#diff-2e459048c0e4a2f61e4975290ed881f0e3ddc31c | ||
| It seems zuul generates .git/packed-refs file improperly, `git describe` in git repo with annotated tags fails with error: | ||
| ``` | ||
| fatal: No annotated tags can describe 'db62e....'. | ||
| However, there were unannotated tags: try --tags. | ||
| ``` | ||
| Indeed proper `.git/packed-refs` has extra line starting with `^<SHA>` after lines with tag while packed-refs generated by zuul doesn't `^<SHA>` lines (note leading `^` at the beginning). | ||
| This issue is not reproduced on zuul.opendev.org because git repo prepared by merger is uploaded to CI nodes using `prepare-workspace-git` role. This role does `git push` and .git/packed-refs is regenerated on CI node. | ||
| I found this issue while using `prepare-workspace` role. This roles uses rsync to upload files and `.git/packed-refs` is copied 'as is' on node and hence `git describe` fails while running playbook. | ||
| @clarkb:matrix.org | vlotorev: the motivation behind that change was a fairly large speedup in merger performance. That said one of my concerns was that we'd somehwo get the format wrong. Are you able to share a more concrete example of the problem? | 20:52 |
| @vlotorev:matrix.org | just run `git describe` in repo prepared by merger | 20:53 |
| @clarkb:matrix.org | no I understand that. I meant the format differences but I think I see them in the zuul repo itself | 20:53 |
| @clarkb:matrix.org | I think those entries may be for tags | 20:53 |
| @clarkb:matrix.org | looks like this in zuul: | 20:54 |
| ``` | ||
| ``` | ||
| @clarkb:matrix.org | * looks like this in zuul: | 20:54 |
| ``` | ||
| 7ce69d5a7b0a9db9d33604f54bb09ae7f4e3679d refs/tags/1.1.0 | ||
| ^ad61501575100c6992f4109bdac045aab5c16809 | ||
| ``` | ||
| @jim:acmegating.com | the second one is the signature object | 20:55 |
| @clarkb:matrix.org | `7ce69` is the hash of the tag object and `ad6150` is the ref the tag points to? | 20:55 |
| @clarkb:matrix.org | ah ok | 20:55 |
| @jim:acmegating.com | no that's backwards, you're right clark | 20:56 |
| @jim:acmegating.com | * the first one is the signed tag object the second in the ref it points to | 20:56 |
| @jim:acmegating.com | * yep :) | 20:57 |
| @clarkb:matrix.org | so I guess when writing out refs/tags we need to add entries for the target refs | 20:58 |
| @clarkb:matrix.org | ``` | 21:02 |
| diff --git a/zuul/merger/merger.py b/zuul/merger/merger.py | ||
| index 452ebb854..8469955bb 100644 | ||
| --- a/zuul/merger/merger.py | ||
| +++ b/zuul/merger/merger.py | ||
| @@ -554,6 +554,12 @@ class Repo(object): | ||
| repo.odb.info(binsha) | ||
| f.write(f'{hexsha} {path}\n'.encode(encoding)) | ||
| msg = f"Set reference {path} at {hexsha} in {repo.git_dir}" | ||
| + if path.startswith('refs/tags/'): | ||
| + # Tags are special as they have a ref for the tag | ||
| + # object and a ref for the target commit | ||
| + # TODO determine tag_target_hexsha | ||
| + f.write(f'^{tag_target_hexsha}\n'.encode(encoding)) | ||
| + msg += f" with tag target {tag_target_hexsha}" | ||
| if log: | ||
| log.debug(msg) | ||
| else: | ||
| ``` | ||
| @clarkb:matrix.org | Something like that maybe? | 21:02 |
| @clarkb:matrix.org | I've channeled my inner mordred and left out the tricky bit determining the tag target hexsha for now | 21:02 |
| @clarkb:matrix.org | ok I think I have it after some local testing with gitpython. Now to see if any test cases need updating | 21:10 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/zuul] 914715: Handle tags when packing refs https://review.opendev.org/c/zuul/zuul/+/914715 | 21:38 | |
| @clarkb:matrix.org | There is one piece in there I'm not quite sure about which I'll leave a comment on | 21:38 |
| @clarkb:matrix.org | ok posted. corvus you may know? | 21:42 |
| @jim:acmegating.com | Clark: it's only signed tags that have the extra line | 21:44 |
| @clarkb:matrix.org | really? I guess regular tags don't have an object and are just a normal ref? | 21:49 |
| @jim:acmegating.com | yep like a symlink | 21:49 |
| @clarkb:matrix.org | ok that means we need to check if tag.object exists and if so then do the work | 21:49 |
| @clarkb:matrix.org | and my test isn't valid | 21:50 |
| @jim:acmegating.com | Clark: yeah, i wrote some comments on that change | 21:51 |
| @jim:acmegating.com | Clark: gitpython docs say that if it's a tag object then `.object` should be the object it points to | 21:53 |
| @jim:acmegating.com | Clark: though it looks like `repo.commit(hexsha)` will return the pointed-to commit | 21:59 |
| @jim:acmegating.com | however, i'm not sure how fast/slow repo.commit is, so we would need to benchmark it with tens of thousands of tags to make sure we're not adding to the runtime; if it's too slow, we'll need to get the data from the repo state | 22:01 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: | 22:02 | |
| - [zuul/zuul] 914415: Move fake gerrit and pagure into dedicated files https://review.opendev.org/c/zuul/zuul/+/914415 | ||
| - [zuul/zuul] 914561: Make the test change database serializable https://review.opendev.org/c/zuul/zuul/+/914561 | ||
| - [zuul/zuul] 914562: Add an upgrade test https://review.opendev.org/c/zuul/zuul/+/914562 | ||
| @clarkb:matrix.org | corvus: ok I was using repo.rev_parse | 22:04 |
| @clarkb:matrix.org | corvus: if that is a signed tag we get back a TagObject which .object is the commit pointed to otherwise we get a Commit object if it is unsigned | 22:05 |
| @jim:acmegating.com | rev_parse is doing a lot of extra work since we know we're starting with a hexsha | 22:06 |
| @clarkb:matrix.org | ya I guess if repo.commit(hexsha).hexsha does not equal the origianl hexsha then we know we're a signed tag? | 22:06 |
| @jim:acmegating.com | seems reasonable | 22:07 |
| @clarkb:matrix.org | corvus: any idea how to test this properly? I don't know that we have gpg configured for signing tags in the zuul test suite? | 22:10 |
| @jim:acmegating.com | Clark: no, it seems like it might be a lot of work to set up gpg just for a unit test. could maybe stick an entire git repo with a signed tag in there as a fixture, but that's probably almost as much work. either way i don't think it's going to be easy. | 22:11 |
| @jim:acmegating.com | (especially fighting the new gpg daemon) | 22:12 |
| @clarkb:matrix.org | Looks like annotated tags have the same behavior so we don't actually need to sign anything | 22:16 |
| @jim:acmegating.com | ah cool | 22:18 |
| @jim:acmegating.com | Clark: then we should have an end-to-end test where we put an annotated tag in the repo so that we get the input data from the repo state (to validate the assertion than the tag points to the tag object not the commit) | 22:20 |
| @jim:acmegating.com | vlotorev: which sha did zuul write, the tag sha or commit sha? | 22:22 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/zuul] 914715: Handle annotated and signed tags when packing refs https://review.opendev.org/c/zuul/zuul/+/914715 | 22:27 | |
| @clarkb:matrix.org | corvus: oh I like the idea of an end to end test. That latest patchset doesn't do that but does do the more unittest testing | 22:28 |
| @clarkb:matrix.org | vlotorev: ya that info would be helpful as spot checking zuul's repo on a zuul merger and a zuul executor shows valid packged refs | 22:31 |
| @clarkb:matrix.org | so we aren't going through the code paths that reset everything there I guess? | 22:31 |
| @jim:acmegating.com | yeah, that's only used in a workspace checkout | 22:32 |
| @clarkb:matrix.org | aha | 22:32 |
| @clarkb:matrix.org | I found a builds/uuid entry with openstack/tempest in it but then packed_refs only has master in it | 22:34 |
| @clarkb:matrix.org | I think I found one that should give us info | 22:35 |
| @clarkb:matrix.org | openstack/neutron-tempest-plugin has `1f418b95227975058463464ae327470e3672f686 refs/tags/0.1.0` in it | 22:35 |
| @clarkb:matrix.org | I think that is the tag object hash and not the commit hash | 22:38 |
| @clarkb:matrix.org | so my assumption in the change I wrong should hold | 22:38 |
| @jim:acmegating.com | Clark: good, that looks like the tag object, which is what we expected. i also manually tested the merger repo state code and got the same thing. we should still add the end-to-end test, but i think that's good. | 22:38 |
| @vlotorev:matrix.org | corvus: here is logs for repo cloned from gerrit and for repo fetched from CI node: | 22:39 |
| ``` | ||
| $ cat sandbox/.git/packed-refs | ||
| # pack-refs with: peeled fully-peeled sorted | ||
| f14249f65247fa8ae273d52a185c8f3afae45a28 refs/remotes/origin/master | ||
| 24d70188aae833bb4fd43f16cb6f5cde3392e7f4 refs/tags/0.1 | ||
| ^f14249f65247fa8ae273d52a185c8f3afae45a28 | ||
| $ cat zuul-sandbox/.git/packed-refs | ||
| # pack-refs with: peeled fully-peeled sorted | ||
| db62e3c9bbebb7e77dead2009eea3922832faade refs/heads/master | ||
| f14249f65247fa8ae273d52a185c8f3afae45a28 refs/remotes/origin/master | ||
| 24d70188aae833bb4fd43f16cb6f5cde3392e7f4 refs/tags/0.1 | ||
| db62e3c9bbebb7e77dead2009eea3922832faade refs/zuul/4f26aeafdb2367620a393c973eddbe8f8b846ebd | ||
| db62e3c9bbebb7e77dead2009eea3922832faade refs/zuul/fetch | ||
| ``` | ||
| @clarkb:matrix.org | thanks I think that confirms what I just found too so ya we need to add another test but then this code should be well covered | 22:40 |
| @vlotorev:matrix.org | * corvus: here are logs for repo cloned from gerrit and for repo fetched from CI node: | 22:42 |
| ``` | ||
| $ cat sandbox/.git/packed-refs | ||
| # pack-refs with: peeled fully-peeled sorted | ||
| f14249f65247fa8ae273d52a185c8f3afae45a28 refs/remotes/origin/master | ||
| 24d70188aae833bb4fd43f16cb6f5cde3392e7f4 refs/tags/0.1 | ||
| ^f14249f65247fa8ae273d52a185c8f3afae45a28 | ||
| $ cat zuul-sandbox/.git/packed-refs | ||
| # pack-refs with: peeled fully-peeled sorted | ||
| db62e3c9bbebb7e77dead2009eea3922832faade refs/heads/master | ||
| f14249f65247fa8ae273d52a185c8f3afae45a28 refs/remotes/origin/master | ||
| 24d70188aae833bb4fd43f16cb6f5cde3392e7f4 refs/tags/0.1 | ||
| db62e3c9bbebb7e77dead2009eea3922832faade refs/zuul/4f26aeafdb2367620a393c973eddbe8f8b846ebd | ||
| db62e3c9bbebb7e77dead2009eea3922832faade refs/zuul/fetch | ||
| ``` | ||
| @jim:acmegating.com | as for timing, i created a repo with 10k signed tags, and it takes 0.0923 seconds to do an odb.info on all of them. if i add a repo.commit() that increases to 0.5810365611687303. so it is considerably more work, though it may be acceptable; the alternative would be a significant increase in the repo state size. | 22:42 |
| @jim:acmegating.com | * as for timing, i created a repo with 10k signed tags, and it takes 0.0923 seconds to do an odb.info on all of them. if i add a repo.commit() that increases to 0.5810. so it is considerably more work, though it may be acceptable; the alternative would be a significant increase in the repo state size. | 22:43 |
| @jim:acmegating.com | (i checked, and unfortunately, odb.info doesn't have the commit info | 22:43 |
| @jim:acmegating.com | Clark: lol, repo.commit() calls rev_parse, and somehow adds lots of runtime | 22:45 |
| @clarkb:matrix.org | oh should we be using rev_parse? | 22:45 |
| @jim:acmegating.com | yeah, but lemme check real quick if there's an odb method that will get it for us | 22:46 |
| @clarkb:matrix.org | I've foudn a test that appears to run against a regular tag. I think I can cobble something together based on that that works with an annotated tag | 22:46 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 914729: Optimize the merger tag sha translation https://review.opendev.org/c/zuul/zuul/+/914729 | 23:09 | |
| @jim:acmegating.com | Clark: ^ that's as optimized as i can get it. i tried using the odb methods directly, but no change in runtime. that gets us 0.2258 seconds in my test. we could achieve most of that with rev_parse as well, that's like 0.25 or 0.26; but this saves a little bit of processing. | 23:10 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 914729: Optimize the merger tag sha translation https://review.opendev.org/c/zuul/zuul/+/914729 | 23:13 | |
| @clarkb:matrix.org | ok that actually looks really similar to what I ended up with rev_parse. Should I take your change and squash it into mine adding in the updated testing? | 23:14 |
| @jim:acmegating.com | yeah, feel free; mostly did it as a followup to avoid collision | 23:17 |
| @clarkb:matrix.org | corvus: `git.Object.new_from_sha(repo, binsha)` feels like a huge departure from the repo api, btu I can't find anything like `repo.create_object_from_sha` so I'll live with it | 23:25 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/zuul] 914715: Handle annotated and signed tags when packing refs https://review.opendev.org/c/zuul/zuul/+/914715 | 23:25 | |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/zuul] 914715: Handle annotated and signed tags when packing refs https://review.opendev.org/c/zuul/zuul/+/914715 | 23:26 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!