| pabelanger[m] | So tristanC and I are working to get 2 github apps working on a single zuul instances, but running into what I believe to be a bug in zuul | 00:17 |
|---|---|---|
| pabelanger[m] | https://github.com/ansible-collections/ansible.netcommon/pull/304 has some more information, but we have 2 github apps, with 2 different pipelines. Noop jobs work, but running of an Zuul jobs fails | 00:17 |
| pabelanger[m] | it looks like something in the zuul.AnsibleJob isn't using the correct connection_name | 00:18 |
| clarkb | pabelanger[m]: are they set up as different connections? | 00:18 |
| pabelanger[m] | yah | 00:18 |
| pabelanger[m] | 2 connections via 2 different github apps | 00:18 |
| pabelanger[m] | I believe we are getting confused on the repo state between the 2 connection, while both are connection to github.com | 00:19 |
| clarkb | is it happening when operating on the same repo? | 00:20 |
| pabelanger[m] | no, we only have the repo in 1 connection | 00:21 |
| pabelanger[m] | tristanC: believe it is happening due to https://opendev.org/zuul/zuul/src/branch/master/zuul/executor/common.py#L183 | 00:21 |
| pabelanger[m] | and we are matching on the first connection loaded that is github.com | 00:22 |
| pabelanger[m] | if so, we likely need to add a secondary check, to see if the project it loaded into the tenant config too | 00:23 |
| clarkb | you can probably write a test that reproduces the behavior? | 00:25 |
| clarkb | I want to say the github test infrastructure is pretty robust and complete | 00:25 |
| pabelanger[m] | sorry, not tenant config, but is trusted / untrusted for the connection in the tenant | 00:26 |
| pabelanger[m] | yes, I think that's my first step to understand what is happening again | 00:26 |
| clarkb | I've got to go now, sorry I wasn't more help | 00:26 |
| pabelanger[m] | np, thanks for jumping in. This can wait until tomorrow | 00:26 |
| felixedel | clarkb, corvus: I've left a comment regarding the "dummy" nodeset. Maybe we could improve the comments there to be less misleading. I just wasn't sure if we could say that this is a real nodeset since it's not created from the job's configuration. Apart from that I'm fine with the current state of this change :) | 05:15 |
| *** bhagyashris_ is now known as bhagyashris|ruck | 06:25 | |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Lock tenants, pipelines, queues during processing https://review.opendev.org/c/zuul/zuul/+/796013 | 06:27 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Create config cache ltime before requesting files https://review.opendev.org/c/zuul/zuul/+/800659 | 07:24 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Ensure config cache stages are used correctly https://review.opendev.org/c/zuul/zuul/+/800660 | 07:24 |
| opendevreview | Merged zuul/zuul master: zuul-stream: drop ARA bindep install https://review.opendev.org/c/zuul/zuul/+/793868 | 08:25 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Ensure config cache stages are used correctly https://review.opendev.org/c/zuul/zuul/+/800660 | 08:53 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Create config cache ltime before requesting files https://review.opendev.org/c/zuul/zuul/+/800659 | 08:54 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Ensure config cache stages are used correctly https://review.opendev.org/c/zuul/zuul/+/800660 | 08:54 |
| opendevreview | Benjamin Schanzel proposed zuul/nodepool master: Add Tenant-Scoped Resource Quota https://review.opendev.org/c/zuul/nodepool/+/800765 | 10:11 |
| opendevreview | Benjamin Schanzel proposed zuul/zuul master: Update tenant quota spec config example https://review.opendev.org/c/zuul/zuul/+/800766 | 10:17 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Ensure config cache stages are used correctly https://review.opendev.org/c/zuul/zuul/+/800660 | 10:39 |
| *** bhavikdbavishi1 is now known as bhavikdbavishi | 10:39 | |
| *** dviroel|out is now known as dviroel | 11:24 | |
| tobiash[m] | corvus: awesome, thanks. Meanwhile we've looked closer at the intermediate reporter change and since those reportings are actually decoupled now from the async reporting we're confident that it's quite simple to make those async as well (should be easy to handle that isolated inside the sql reporter itself) | 11:44 |
| tobiash[m] | corvus: so thanks a lot for waiting and feel free to merge the intermediate reporting | 11:44 |
| opendevreview | Felix Edel proposed zuul/zuul master: Make reporting asynchronous https://review.opendev.org/c/zuul/zuul/+/691253 | 12:15 |
| opendevreview | Benjamin Schanzel proposed zuul/nodepool master: Add Tenant-Scoped Resource Quota https://review.opendev.org/c/zuul/nodepool/+/800765 | 12:30 |
| opendevreview | James E. Blair proposed zuul/zuul master: Always report the build page https://review.opendev.org/c/zuul/zuul/+/800112 | 12:31 |
| corvus | felixedel, tobiash: thanks! both approved :) | 12:31 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Mount nodepool externalConfig for aws and azure https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:05 |
| avass[m] | corvus: I'm not sure if I'm a fan of that ^ but it looks like that's the way openstack and kubernetes is handled. I also couldn't find any documentation for it | 13:08 |
| avass[m] | I really need to move my homeserver off digitalocean, apparently automatic upgrades to my kubernetes cluster resets the config on my load balancer so my matrix homeserver can't federate. Hope i didn't miss anything important | 13:09 |
| corvus | avass: yeah, i was just noticing the same thing; i'll see if i can write some docs. | 13:10 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Mount nodepool externalConfig for aws and azure https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:10 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Mount nodepool externalConfig for aws and azure https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:10 |
| corvus | avass: what is the external config file used by aws? | 13:13 |
| avass[m] | corvus: i think you need it if nodepools needs access to multiple profiles | 13:14 |
| corvus | avass: i guess the boto ~/.aws/config file? | 13:16 |
| avass[m] | had to check, but there's a ~/.aws/config and a ~/.aws/credentials, where the config file specifies region, output format and which credentials the profile should use from ~/.aws/credentials | 13:18 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: wip: Invalidate project config cache via ltime https://review.opendev.org/c/zuul/zuul/+/800788 | 13:18 |
| avass[m] | corvus: in short yep | 13:18 |
| corvus | avass: and i guess the user would also need to set env variables to point at those files? | 13:19 |
| corvus | since your change mounts them in /etc/aws | 13:19 |
| avass[m] | Oh right, I think there's a default path in ~/.aws... yes :) | 13:19 |
| opendevreview | Benjamin Schanzel proposed zuul/nodepool master: Add Tenant-Scoped Resource Quota https://review.opendev.org/c/zuul/nodepool/+/800765 | 13:19 |
| corvus | avass: we could either document that, and/or maybe set some defaults? | 13:20 |
| avass[m] | my main concern is that each new driver would need an operator update for it to work, and we don't take that approach for connections | 13:21 |
| corvus | avass: yeah, i don't love it either, though at least it's just one change for each new driver... but maybe we can make it more generic... | 13:22 |
| corvus | avass: actually, we can make it generic right now by just iterating over the externalConfig keys | 13:23 |
| avass[m] | ++, it's also not terribly important at the moment so maybe we should take a look at that later :) | 13:23 |
| corvus | avass: so instead of a bunch of if statements, we just mount externalConfig.foo at /etc/foo | 13:23 |
| corvus | i'll finish up the docs, and we can optimize it later | 13:24 |
| avass[m] | makes sense to me, we would also need to expose environment variables somehow... but maybe we already do that since I think I brought up http_proxy some time ago | 13:24 |
| corvus | avass: yep env variables are supported; they're also passed through to the launcher, but the docs omit that; i'll add that to my update. | 13:26 |
| *** ysandeep is now known as ysandeep|PTO | 13:31 | |
| corvus | oh it does say launcher, i just missed it :) | 13:33 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Document externalConfig https://review.opendev.org/c/zuul/zuul-operator/+/800791 | 13:34 |
| corvus | avass: ^ | 13:34 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Make nodepool external_config mount more generic https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:34 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Make nodepool external_config mount more generic https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:34 |
| avass[m] | oh I somehow messed that up and kep azure/aws | 13:35 |
| avass[m] | oh, no I see what I did. gonna push a quick fix then quit for today. I finally got vaccinated today and I'm starting to get a bit of a brain fog :) | 13:36 |
| opendevreview | Merged zuul/zuul master: Switch to ZooKeeper backed NodesProvisionedEvents https://review.opendev.org/c/zuul/zuul/+/799833 | 13:38 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Make nodepool external_config mount more generic https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:38 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Make nodepool external_config mount more generic https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:39 |
| corvus | avass: can i take over? :) | 13:39 |
| avass[m] | corvus: please do :) | 13:40 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Make nodepool external_config mount more generic https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 13:40 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Document externalConfig https://review.opendev.org/c/zuul/zuul-operator/+/800791 | 13:40 |
| corvus | avass: okay that looks right in gertty to me :) | 13:41 |
| avass[m] | corvus: looks good to me | 13:41 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: wip: Invalidate project config cache via ltime https://review.opendev.org/c/zuul/zuul/+/800788 | 13:46 |
| clarkb | felixedel: ya an indication that it contains real data other than the name might be a good thing. Its not truly a dummy record. It is a record with real data except for the name? | 14:38 |
| felixedel | clarkb: I think it's a record with real data except for the name and the groups (as those two are zuul-related information and not nodepool if I'm not mistaken). | 14:40 |
| clarkb | ah ok | 14:41 |
| clarkb | I think calling it a dummy nodeset is confusing to me because we do populate it with real data, it is just the real data that nodepool is aware of | 14:41 |
| clarkb | (and we lose some of the zuul specific context) | 14:41 |
| felixedel | yes. To me this was enough to call it a dummy :D, but like you said it might be misleading for others | 14:42 |
| swest | I think github3py would call this a ShortNodeset ;) | 14:49 |
| corvus | NotANodesetButActuallyReallyANodeset | 14:49 |
| *** dviroel is now known as dviroel|lunch | 14:50 | |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Use a temp ZK config cache for tenant validation https://review.opendev.org/c/zuul/zuul/+/800800 | 15:15 |
| clarkb | that is a great change number | 15:27 |
| clarkb | time to get some picnic baskets | 15:28 |
| *** dviroel|lunch is now known as dviroel | 15:39 | |
| corvus | clarkb: it's definitely better than the average change number | 15:57 |
| *** marios is now known as marios|out | 16:33 | |
| *** timburke_ is now known as timburke | 18:12 | |
| y2kenny | For start-message, the documentation listed the various available replacement fields https://zuul-ci.org/docs/zuul/reference/pipeline_def.html#attr-pipeline.start-message. Is it possible to do the same for success-url? (or other -url -message?) https://zuul-ci.org/docs/zuul/reference/job_def.html#attr-job.success-url | 19:55 |
| y2kenny | I played around with it a bit and it seems to work for "tenant" and "build" but I wasn't able to find documentation around that. | 19:56 |
| corvus | y2kenny: i would not reccomend it; i have a change in review to remove the success and failure urls entirely | 19:56 |
| y2kenny | corvus: uh oh... ok. Is it just url or the success-message as well? | 19:57 |
| corvus | y2kenny: instead, you can return an artifact, and that url will be displayed on the build page | 19:57 |
| corvus | y2kenny: just the url | 19:57 |
| y2kenny | corvus: would that url shows up in email or gerrit comment as well? | 19:58 |
| corvus | y2kenny: no, only links to the build page | 19:58 |
| corvus | if the report-build-page tenant option is set, then that's already the behavior. that's true for zuul's own zuul on opendev, so you can see for example we return the link to the doc preview site as an artifact: https://zuul.opendev.org/t/zuul/build/f270422bd9944124bdf20e8dcf8c75ea/artifacts | 19:59 |
| y2kenny | corvus: ok. I am doing something similar but I was hoping, for certain job, I can feed the artifact url directly to the code review feedback | 20:01 |
| y2kenny | I have a job that automatically compile tex into pdf and I was hoping I can feed the pdf url directly back to the code review page. | 20:01 |
| y2kenny | I already have the PDF url as part of the zuul_return but I was trying to see if I have access to zuul_return data from either success-message or success-url | 20:02 |
| corvus | y2kenny: there is a possibility that i haven't looked into much yet that with the new gerrit results plugin system that a zuul plugin could provide extra artifact links like that. i think it would be useful, but it would also be a non-trivial amount of gerrit plugin hacking | 20:03 |
| y2kenny | corvus: ok. As an alternative to that, is it possible to have success-message access zuul_return data as replacement fields (and except the secrets that is part of the zuul_return.) | 20:04 |
| corvus | y2kenny: note that success message overrides the word "SUCCESS", which could have unintended consequences | 20:04 |
| corvus | y2kenny: how about this idea: leave a "warning" message in the report? https://zuul-ci.org/docs/zuul/reference/jobs.html#leaving-warnings | 20:05 |
| corvus | y2kenny: i think that's going to look like "Warning: ..." in the review comment; but maybe you can prototype the idea with that and we could add another severity level if it's useful. | 20:08 |
| y2kenny | corvus: Oh... I did not know about that feature but this can be useful. I will give that a try. | 20:08 |
| avass[m] | corvus: y2kenny it's also possible to leave comments on specific files like the tox job does for linting, so you could leave the pdf link as a line 0 comment (file comment) on the .tex file | 20:19 |
| corvus | avass, y2kenny: oh that's a good idea too. also, i think if you omit the line number, it will render as a gerrit file comment. though it doesn't look like that detail is documented. https://zuul-ci.org/docs/zuul/reference/jobs.html#leaving-file-comments | 20:21 |
| y2kenny | avass[m]: That's possible, but then I will have to decide which file to add. There can be more than one file that generates the pdf. | 20:21 |
| y2kenny | I should rephrase... there can be multiple source files (other than the .tex file) that contributes to the generated pdf | 20:22 |
| corvus | gotcha :) | 20:22 |
| corvus | so possible, but extra work | 20:22 |
| corvus | (if you do try that method, i would set zuul.disable_file_comment_line_mapping for that case, btw) | 20:23 |
| y2kenny | corvus: the 'warnings' is actually pretty good because it's per change (except the 'warning' word ;).) And I just tried the warning method and it looks good. | 20:24 |
| corvus | y2kenny: cool; then i think we can copy-pasta that to make an 'info' level | 20:24 |
| y2kenny | so if you folks can add a level like "information" or "notes", that would be perfect. | 20:25 |
| y2kenny | yes, awesome! | 20:25 |
| avass[m] | y2kenny: got it, we have some old job that pieces together simulink models and tex snippets to generate documentation somewhere :) | 20:28 |
| y2kenny | avass[m]: Nice. I am using tikz+Beamer to create presentation. Initial drawing is a bit harder to start than WYSIWYG but linking components and positioning things and then animating it is much easier. | 20:31 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Make nodepool external_config mount more generic https://review.opendev.org/c/zuul/zuul-operator/+/800786 | 20:36 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Document externalConfig https://review.opendev.org/c/zuul/zuul-operator/+/800791 | 20:36 |
| y2kenny | avass[m]: the biggest plus is now one can code review presentation :) | 20:36 |
| avass[m] | y2kenny: heh, people really like their power point here :) | 20:39 |
| y2kenny | avass[m]: same here... but one day I will get legal dept. to do publication review for conference decks this way... one day... (one can always dream ;)) | 20:41 |
| clarkb | then you can hit approve and have zuul give the presentation at the conference :) | 20:42 |
| y2kenny | :) | 20:43 |
| avass[m] | And have speculative presentations in the gate? :) | 20:43 |
| clarkb | corvus: swest: reviewing https://review.opendev.org/c/zuul/zuul/+/796013/7/zuul/scheduler.py and I wonder if we should be pushing the locks into the functions we call with the locks | 21:26 |
| clarkb | instead of doing with lock(): foo() do def foo(): with lock(): stuff ; foo() | 21:27 |
| clarkb | doReconfigureEvent(), loadTenant(), process_tenant_trigger_queue(), process_pipelines() could probably do that? process_tenant_trigger_queue() and process_pipelines() may be weird to do that too if we want to hold the lock across those two subsequent calls | 21:29 |
| clarkb | but for the other two I thinkwe could push that down without changing behavior, but now it becomes safer to call those functions? | 21:29 |
| corvus | clarkb: normally i'd agree, but given the fact that we need to hold it across at least some functions, i think i like being consistent. for example, we can see in this one file how broadly we're holding the lock; we don't have to trace it across scheduler and configloader | 21:30 |
| corvus | i could maybe see process_pipelines and methods like that which are also on the scheduler class | 21:31 |
| clarkb | fair enough with loadTenant | 21:32 |
| corvus | well, process_tenant_trigger_queue and process_pipelines have a single lock | 21:32 |
| clarkb | I'm just selfishly trying to think of ways to simplify reviews of this going forward | 21:32 |
| clarkb | because now we're going to have to be very careful about ensuring locks are held when calling functions like this. That said we don't need to call them often | 21:32 |
| corvus | yeah, these are methods with very few call sites; i think making sure we can see the big picture is important. | 21:34 |
| clarkb | oh ya its important we skip the whole tenant when failing to get the tenant read lock too | 21:38 |
| clarkb | then we don't fail to lock and get the lock moments later in the next step | 21:38 |
| clarkb | corvus: I think putting it at the beginning of doReconfigureEvent() may help keep the high level view in that file while making that function safer to use. I'll leave a note but I think that isn't worth a -1 | 21:40 |
| clarkb | corvus: should I go ahead and approve it? and maybe we plan for a friday restart again? | 21:40 |
| corvus | <clarkb "corvus: I think putting it at th"> i don't think i understand this yet, but probably will when i look at your comment | 21:43 |
| corvus | <corvus "i don't think i understand this "> yeah, i think with felixedel's change and this one, a friday restart would be good. | 21:44 |
| clarkb | corvus: comment posted. Feel free to approve if you don't think that is critical | 21:44 |
| corvus | we might even get another change from Simon Westphahl in there, i think the config local cache stuff is close | 21:45 |
| clarkb | https://review.opendev.org/c/zuul/zuul/+/800659/ that one? | 21:45 |
| clarkb | I was goign to look at that one next | 21:45 |
| corvus | clarkb: yep, and the next 2 changes are worth looking at too; they aren't ready to merge, but i think they're mostly done and you can get the whole story | 21:46 |
| corvus | clarkb: your comment makes sense to me, thx :) i'll just hold off on approving until tomorrow; no rush | 21:47 |
| clarkb | wfm | 21:47 |
| *** dviroel is now known as dviroel|out | 22:01 | |
| *** dmellado_ is now known as dmellado | 22:34 | |
| clarkb | corvus: is zstat.last_modified_transaction_id and integer incrementer that bumps each time we call that file? | 23:01 |
| clarkb | s/and/an/ | 23:01 |
| clarkb | corvus: also can you check my comments on https://review.opendev.org/c/zuul/zuul/+/800659 particularly the first one | 23:17 |
| clarkb | I think the code may be correct but we've got some terminology collisions | 23:18 |
| corvus | clarkb: it's the global zk transaction id; so it will always be larger, but the magnitude of the increase is uncertain | 23:19 |
| clarkb | gotcha | 23:19 |
| clarkb | I left a similar comment on the child change. Basically I think it reads weird to do if updated_ltime < cached_ltime: return False to indicate the cache ltime is invalid. Because updated_ltime is the ltime for the cache and cache_ltime is the internal state ltime the code is correct, but it reads wrong | 23:30 |
| corvus | clarkb: yeah i was just writing a lengthy response (and triple checking it) -- just published comment | 23:31 |
| clarkb | looking, thanks | 23:31 |
| clarkb | responded | 23:32 |
| corvus | cool, switched to -1 | 23:33 |
| opendevreview | James E. Blair proposed zuul/zuul master: WIP: ZK keystore import/export https://review.opendev.org/c/zuul/zuul/+/800854 | 23:38 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!