Tuesday, 2017-06-13

jeblairjamielennox: when all the streaming stuff is in place, i expect us to have a finger multiplexer (probably built into the websocket multiplexer) so that we can have a single streaming server for all of the executors; the executors should not be end-user addressible.  eg zuul.example.com/uuid00:01
jamielennoxjeblair: yea, that was my understanding - but it means that there's really no reason to run the executor fingerd on 79, and therefore no reason for root on the executor00:02
jeblairjamielennox: that service will either want to listen on 79, or it could listen on another port and you could port-forward to it.00:02
*** dmsimard has joined #zuul00:02
jamielennoxthe multiplexor might be 7900:02
jeblairjamielennox: yep, i think you've got it.00:02
jamielennoxjeblair: so would it be reasonable to change the default port of the executor fingerd?00:02
jeblairjamielennox: yeah, i was thinking we'd do it when we had the multiplexer up and running, but if inverting that would be helpful now, we could probably go ahead and do it.00:03
jamielennoxjeblair: it's purely the run as root that threw me, i've got it changed in local config i just wasn't sure if there was something else i was missing00:05
mordredtristanC: yes. zuul and nodepool both are fully py3 now00:11
jlkwell00:13
*** yolanda has quit IRC00:14
mordredjlk: no?00:14
jlkI think there is an open change to actually _use_ that setting in some ways00:14
jlksorry that was meant for jamielennox and jeblair00:14
mordredah - nod00:15
jlkhttps://review.openstack.org/47310300:15
mordredjlk: did you see the patch about github retries?00:15
jlkjamielennox: that's part of some changes00:15
jlkmordred: yeah, I -1d it00:15
mordredah - I see that now - cool00:15
jamielennoxjlk: ok, i can try and look at those - maybe the executor needs to register the finger port in zk when it picks up a job?00:16
jamielennoxcause at the moment it's configurable, but those changes don't appear to be00:17
jamielennoxoh, no, it's still configurable00:17
mordredjamielennox: the port is supported at least in the finger url bit when I added them00:18
mordredor, rather, here: https://review.openstack.org/#/c/473103/300:19
mordredso they show up _somewhere_00:19
jamielennoxmordred: yea, i'm trying to get back to the point where i can actually test that00:21
jamielennoxstuck on py3 and some ssl issues, maybe today, depends how badly the dentist goes00:22
pabelangerjeblair: mordred https://review.openstack.org/#/c/473533/ and https://review.openstack.org/#/c/472861/ need to land to make zuul +1 again00:25
pabelangerthat should help zuul +1 all our open changes00:26
pabelangerzuul (user)00:26
pabelangerhttps://review.openstack.org/#/c/473613/ would also be nice to get a few more nodes on nl01.o.o00:26
*** yolanda has joined #zuul00:37
openstackgerritDavid Shrewsbury proposed openstack-infra/zuul feature/zuulv3: Add log streaming test  https://review.openstack.org/47107900:48
Shrewsstupid pep800:48
Shrewsjamielennox: should only need to change the finger port to not run as root00:50
Shrewsthat was the only reason00:50
openstackgerritJesse Keating proposed openstack-infra/zuul feature/zuulv3: Improve fake github commit status handling  https://review.openstack.org/47363301:02
jlkblah, pep8.01:05
openstackgerritJesse Keating proposed openstack-infra/zuul feature/zuulv3: Improve fake github commit status handling  https://review.openstack.org/47363301:06
jamielennoxany zuul v3 outstanding patches?01:09
jamielennoxbah01:09
jamielennoxpython301:09
jamielennoxgetting some encoding issues01:09
openstackgerritTristan Cacqueray proposed openstack-infra/zuul feature/zuulv3: github: retry pull_request()  https://review.openstack.org/47330101:14
openstackgerritTristan Cacqueray proposed openstack-infra/zuul feature/zuulv3: zuul_stream: handle empty line  https://review.openstack.org/47322201:34
*** yolanda has quit IRC01:37
*** jiaohaolin1 has left #zuul01:45
openstackgerritJamie Lennox proposed openstack-infra/zuul feature/zuulv3: Encode webhook_token secret  https://review.openstack.org/47367403:36
openstackgerritJesse Keating proposed openstack-infra/zuul feature/zuulv3: Improve fake github commit status handling  https://review.openstack.org/47363303:36
*** bhavik1 has joined #zuul04:37
*** yolanda has joined #zuul05:54
*** isaacb has joined #zuul06:28
*** yolanda has quit IRC07:28
*** bhavik1 has quit IRC08:19
*** hashar has joined #zuul09:13
openstackgerritTristan Cacqueray proposed openstack-infra/zuul feature/zuulv3: RFC: zuul.d: configuration split  https://review.openstack.org/47376409:53
*** isaacb_ has joined #zuul11:07
*** isaacb has quit IRC11:09
*** jkilpatr has joined #zuul11:10
*** isaacb_ has quit IRC11:41
*** isaacb_ has joined #zuul11:42
openstackgerritTristan Cacqueray proposed openstack-infra/zuul feature/zuulv3: RFC: zuul.d: configuration split  https://review.openstack.org/47376411:56
*** openstackgerrit has quit IRC12:18
*** openstackgerrit has joined #zuul12:23
openstackgerritRicardo Carrillo Cruz proposed openstack-infra/nodepool feature/zuulv3: Create group for label type  https://review.openstack.org/47380912:23
rcarrillocruzjeblair: handwavy patch for adding per-label groups ^12:24
rcarrillocruzi need to spawn a nodepool VM to test that12:25
*** olaph has quit IRC12:27
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Add a success-url for status.json test  https://review.openstack.org/47360412:29
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Move streaming url formatting to model  https://review.openstack.org/47309012:29
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Support finger ports in finger URL  https://review.openstack.org/47310312:29
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Add build.started state flag  https://review.openstack.org/47381112:29
Shrewsmordred: https://review.openstack.org/471079 is really for sure ready now. sorry i left a pep8 problem in there13:17
*** _ari_|conf is now known as _ari_13:18
mordredShrews: +213:20
mordredrcarrillocruz: lgtm13:21
mordredrcarrillocruz: from a zuul perspective, check out https://review.openstack.org/#/c/467611/ and make sure it's good enough for what you want moving forward13:23
mordredrcarrillocruz, jeblair, jlk: there is a question that I've been mulling related to that, btw - which is whether or not we should add the "openstack" host vars to our nodes that get added by the openstack ansible inventory ...13:24
mordredargument for is that if someone has a playbook for a thing that expects to run against some nodes that are running in openstack and is using those variables, they won't be able to directly test their playbook against the nodes we launch for them13:25
mordredargument against is that we actually dont' communicate anywhere "these are ubuntu-xenial nodes in openstack" - the openstack part is an impl detail13:25
mordredperhaps we should consider including an optional node type in the nodeset spec - so that a user could say "I want an ubuntu-xenial node in an openstack cloud" and if they did, we'd attach the openstack hostvars13:26
mordredbut if they don't explicitly request that, we wouldn't do that13:26
mordred(that might also be a thing we need anyway for other types of workloads - "give me ubuntu-xenial in a container" or "give me ubuntu-xenial in GCE")13:27
mordredBUT - all of that also feeds in to a different thing I keep meaning on bringing up - which is "as a user, how do I get a list of available labels" - which seems like perhaps a good REST API call and dashboard page to add to tristanC's thing13:28
pabelangerjeblair: mordred: Shrews: would it be much effort if I requested finger @ze01.o.o list all running build ID and job names?13:38
Shrewspabelanger: the streamer doesn't have access to all available builds (not without processing on disk directories and trying to make a guess at which ones are valid)13:39
pabelangerShrews: we don't store that info in zk today?13:42
Shrewspabelanger: not that i'm aware of13:42
pabelangerk13:45
*** dkranz has joined #zuul13:47
jlkmordred: so I am thinking that we'd want extra details like that to come from the nodepool driver. If using the openstack nodepool driver, you get some openstack hints in your vars. If you're using the k8s driver, you get some k8s hints in inventory (if there are any), ditto GCP, AWS, and so on.13:52
*** dmsimard has quit IRC14:05
mordredjlk: yah - I definitley think htey should come from there - but if they're on by default is that muddying an abstraction?14:23
mordredjlk: like, today, you get a node named "ubuntu-xenial" in your inventory with some zuul variables and an ip14:24
mordredbut if we started adding the openstack dict to the variables for all openstack-provided nodes, would that be good? or bad?14:24
mordredpabelanger, Shrews: I think that's likely something we can/should add to the multi-plexor when we write it - because the multi-plexor would be in a good position to, at the very least, GET /status.json and then return a list14:26
mordredbut for the executor streamers that seems like the wrong layer14:26
jeblairmordred: agreed re multiplexer14:28
mordredjlk: I definitely agree re the k8s/aws/gce hints etc - just mostly not sure if we should add those hints if the user hasn't requested a certain delivery model- or whether we should expose/allow that request14:28
jeblairmordred: is there any harm to supplying extra info like the openstack hints?  as for requesting it -- i think the way you request something like that is via labels (ubuntu-xenial-container vs ubuntu-xenial-openstack) if that sort of thing is important to you.14:30
mordredjeblair: any harm is the question - I can't decide whether it's leaking abstraction details or not14:36
mordredjeblair: but for the labels thing - in my brain I was imagining that a deployer might have, for instance, an openstack provider, a k8s provider and an AWS provider - but similar to how we define an ubuntu-xenial across all of our clouds today and jobs don't care where it comes from, they define an ubuntu-xenial label across all three and jobs don't care14:38
jeblairmordred: yep, if you don't care, just call it 'ubuntu-xenial'.  if you do care, call it 'ubuntu-xenial-openstack'.  if you both do and don't care, make both labels.14:39
mordredjeblair: right. but if you make both labels, that would affect quota math14:39
jeblairmordred: nodepool v3 is noticeably less concerned with that than v0.14:40
mordrednod14:40
mordredjeblair: I was thinking if you had an ubuntu-xenial node on both and could further restrict it like label: ubuntu-xential provider: openstack14:40
pabelangerI'd appreciate a review on https://review.openstack.org/#/c/472853 please. I'd like to add our infra-root keys to nl01.o.o to make some debugging a little easier: https://review.openstack.org/#/c/47285414:40
mordredjeblair: but I can also see that being too much and not useful14:41
jeblairmordred: the only quota related thing that would come into play is min-ready, and i think that still maps pretty well here.  "i want 10 ubuntu-xenial-openstack nodes min-ready, and only 1 ubuntu-xenial-k8s"14:41
pabelangerWe should also consider adding auto-hold support back for zuulv3.o.o, its helpful when debugging job failures too14:41
jeblairpabelanger: it's in storyboard.  so we have considered it and accepted that it needs to be done.  no one has picked up the task yet.14:42
mordredjeblair: right- but "i want 10 ubuntu-xenial-openstack nodes min-ready" doesn't get you any min-ready of ubuntu-xenial nodes14:43
* Shrews recommends a more non-generic, understandable term than "multiplexor". What's a good term for the thing that gives you the finger?14:43
* fungi refrains from obvious wisecrack opening14:43
jeblairmordred: sure.  ubuntu-xenial would need its own min-ready, and there would be a minor loss of efficiency there.  if we really cared about that, we could probably map them to each other in nodepool.14:43
Shrewsfungi: come on... you know you wanna14:44
mordredjeblair: yah - that was mostly what I was thinking -some sort of map or even an alias or something14:44
jeblairmordred: but "provider: openstack" inside of zuul seems like the abstraction violation.  i'd very much prefer not to do that.14:44
jeblairmordred: if any of this stuff is important, let's make it happen in nodepool.14:45
mordredjeblair: yah - so, that's the question I had WRT exposing the openstack variables - is it an abstraction violation to do so if they're there14:45
pabelangerjeblair: sure, it would be great if we could find a volunteer to maybe work on it.14:45
mordredjeblair: and if it is a violation, is it a thing we should allow someone to opt-in to?14:46
mordredShrews: how about "finger gateway"?14:46
* rcarrillocruz back and reads scrollback14:46
jeblairmordred: i don't know what these variables are and what they are used for.  but if they are useful, i don't see why we can't pass them over.  and if we aren't sure we want to, we can make it an option on the nodepool side (add a "send openstack vars" option to the pool-label)14:46
Shrewsmordred: that is a good (but non-humorous) alternative14:47
Shrewsi like that14:47
mordredjeblair: maybe let's start with passing them, then see if we think they're a bad idea and need an option?14:47
jeblairmordred: do we have a need/use for them now?14:48
mordredjeblair: well - the concept came to mind because rcarrillocruz was talking about adding nodepool label to the groups nova metadata. this affects how the openstack dynamic inventory puts nodes in to groups. that mainly made me think that if people have playbooks that are expecting to be run in the context of the openstack dynamic inventory, we are not actually currently exposing the information those playbooks14:50
mordredare provided even though we have access to it14:50
mordredjeblair: (it's come up a few times in my brain as I've been reviewing the code around the node exchange, but just never bubbled all the way up to "discuss")14:51
jeblairmordred: rcarrillocruz's grouping issue is handled by zuul, right?14:52
jeblairmordred: i guess i'm having trouble understanding what automatic grouping would happen from openstack vars in a nodepool+zuulv3 scenario.14:53
mordredjeblair: oh! sorry - I understand the disconnect14:53
mordredjeblair: I don't mean for automatic grouping purposes - one sec, lemme make a quick paste14:54
rcarrillocruzyeah, groups in nodesets zuul is a superst of my patch14:54
rcarrillocruztrying to think of a use case i could use with what mordred is describing14:54
rcarrillocruzin just- nodepool scenarios14:54
jeblairpabelanger: yeah, i'm concerned that we're all getting a bit distracted by post zuul-v3 things and people are not working on the things we need to get to zuul v3 itself.  there is still a considerable backlog and almost no one is working it down.14:59
pabelangerjeblair: agree. infra-root keys is the first step to helping15:04
jeblairpabelanger: i +3d that change15:04
pabelangerI think I'll also have to work on log publishing today too. It's been too difficult tracking all the open streams in terminals15:05
jeblairmordred: can you +3 https://review.openstack.org/473222 ?15:12
jeblairpabelanger: what do you we need to do regarding log publishing?15:13
pabelangerjeblair: right now we are not publishing any logs to logs.o.o.  This is because only set it up before to publish to zuulv3-dev.o.o/logs. So we need to address that15:15
jeblairpabelanger: right.  what's your plan?15:16
pabelangerjeblair: I'd like to see what people would like to do. Specfically, have we solved our log url / log path issues we discussed at the PTG? Today we just dropped everything into a top level build uuid folder. I am not sure that will work well on logs.o.o15:18
jeblairpabelanger: no, i think we should use the same log path system we have in production now.  so any variables missing, we should add.  first step then is probably to make a new post-playbook scp log copier that copies logs up to logs.o.o.15:20
jeblairpabelanger: it will have to be in a trusted post playbook and use an ssh key on the executor to do the copying because we can't use secrets yet because we haven't turned on ssl support for gearman.15:20
openstackgerritMerged openstack-infra/zuul feature/zuulv3: Improve fake github commit status handling  https://review.openstack.org/47363315:21
jeblair(if we switch gearman to ssl, we can use secrets, then we can put the ssh key in a secret and do this in an untrusted repo)15:21
pabelangerjeblair: okay, do we have a SSL cert for gearman or will it just be a self-signed15:21
jeblairpabelanger: self-signed15:21
pabelangerk15:21
pabelangerwe might as well do that first, let me see what needs to be done15:22
jeblairShrews: i left a comment on https://review.openstack.org/47107915:23
Shrewsjeblair: regarding the file buffering, i quickly ran into a test run where i got less data from the file than what the recv() returned15:24
jeblairShrews, mordred: i know that zuul_console is unbuffered -- did we perhaps inadvertently make the callback plugin write buffered?  or ....15:25
Shrewsso i assumed that was due to buffering15:25
jeblairShrews: recv() returned *more* data than the file?15:25
Shrewsjeblair: yes15:25
jeblairShrews: hrm... you strip newlines from the file... is it possible it was short by the number of newlines?15:26
Shrewsjeblair: no, there were visible lines missing15:26
jeblairShrews: are you thinking they were written to the file after you read the file but before you streamed it?15:27
Shrewsjeblair: it's possible there is some sort of race around that15:28
*** jkilpatr_ has joined #zuul15:29
Shrewsjeblair: that's partly why i didn't also choose to keep calling recv() until the entire buffer size is returned (there is actually a recv flag to do that in a single call)15:29
Shrewsnot being able to guarantee that the file contents returned would meet the minimum15:30
Shrewsi didn't want the test to have a possibility of hanging15:30
*** jkilpatr has quit IRC15:31
jeblairShrews: how's this for a crazy idea: open the log file for reading in the test, open the streaming connection and spawn a thread to read the whole stream in the background and return when complete, remove the flag file, wait for job to complete, read entire file in (the file is deleted now, but we still have an open handle so we'll be able to read it), wait for stream read to complete, compare, close file.15:33
Shrewsjeblair: "return when complete" ... part of the problem is that we don't know when it is "complete"15:35
jeblairShrews: does the finger server disconnect when the job ends?15:36
Shrewsjeblair: in theory the server should break the connection on EOF15:37
jeblairShrews: if that's the case (and it seems like if it isn't, we probably should make it so), if we wait to the end of the job we'll get that signal15:38
*** isaacb_ has quit IRC15:39
Shrewsjeblair: how do you propose coordinating that? ending the job while the streaming is ongoing doesn't guarantee we will get the entire file streamed, does it? the file could get removed from underneath the streamer15:41
jeblairShrews: if so, that's a bug.15:41
jeblairShrews: but i expect it to hold it open until it gets to the end of the file.15:42
Shrewsthis test is really getting super complex. i'll try to add that new bit though15:43
jeblairShrews: i think it can be simpler this way15:43
Shrewshah15:44
jeblairShrews: there's only one sync point in the job15:44
Shrews1?15:44
jeblairsure, laugh, whatever15:44
jeblairyes one sync point15:44
jeblairthe job only has to wait until the test has opened the log file and made the initial stream connection15:44
jeblairthen it can run to the end15:44
jeblairand after that, the test can wait until the build completes in the usual manner15:45
Shrewsnot a derisive laugh, btw. that came across poorly15:45
jeblair'sok :)15:45
jeblairi'll reread it as an evil cackle for fun15:46
Shrewsmore a maniacal, point-of-turning-evil laugh15:47
openstackgerritPaul Belanger proposed openstack-infra/nodepool feature/zuulv3: Add region_name to zookeeper  https://review.openstack.org/47388715:50
pabelangerjeblair: Shrews: ^ was something I noticed missing when we try to setup our mirrors in zuulv3.o.o.  Would be interested in your feedback15:51
mordredjeblair, jlk: this is what I'm talking about http://paste.openstack.org/show/612438/15:51
Shrewspabelanger: lgtm15:52
mordredjeblair: the first thing is what we're emitting today15:53
mordredjeblair: if we add the "openstack" vars (which are added to a node's hostvars if you use the openstack dynamic inventory) you get the second thing15:53
openstackgerritPaul Belanger proposed openstack-infra/zuul feature/zuulv3: Update to nodepool_region_name from zookeeper data  https://review.openstack.org/47388915:55
pabelanger^ is the zuul change too15:55
mordredpabelanger: actually - region is already in the inventory15:55
jeblairmordred: gotcha -- so just things like getting the ip info from there is an obvious usage (like, i have an ansible play that sets up some iptables stuff and reads openstack.networks.private to do that)15:55
mordredpabelanger: should be in the "nodepool_region" variable in the node's hostvars15:55
mordredjeblair: yah - I could also imagine a playbook looking at the volumes dict and deciding to do or not do something with that information15:56
jeblairmordred: *nod*15:56
mordredpabelanger: should be in the "nodepool_region" variable in the node's hostvars15:57
rcarrillocruzi can imagine this would be useful for $provider testing15:57
rcarrillocruzlike15:57
mordredpabelanger: maybe we should also start saving the inventory file when we save logs15:57
jeblairmordred: i think pabelanger wants to transmit that to zuul though15:57
rcarrillocruzgive me a xenial-openstack cos i want to test specific openstack things on it15:57
pabelangermordred: yes, nodepool_region will always be null currently. The zuul patch problem pulls the data from zookeeper, and just renames it to nodepool_region_name, since we use region_name for zookeeper. However, if people okay with nodepool_region, I can omit that change15:58
mordredjeblair: yes. zuul already writes that to the inventory15:58
rcarrillocruzi see no harm on it, providing is opt-in15:58
mordredpabelanger: ah - I'm fine putting it in the zuul vars and not as nodepool_region15:58
jeblairmordred, rcarrillocruz: yeah, it's a big chunk of data... maybe we should have it disabled by default, but let folks opt in to sending it?15:58
rcarrillocruz++ from me for disabled by default15:59
pabelangermordred: okay, nodepool_region_name is okay for you?15:59
mordredjeblair: yah. we can also put a pin in that and figure out the best mechanism to communicate it later15:59
jeblairmordred: ya, not urgent15:59
mordredpabelanger, jeblair: I'm fine either way: zuul.region_name or nodepool_region - but I think I lean towards zuul.region_name16:00
mordredpabelanger, jeblair: if we do zuul.region_name I think we should remove nodepool_region from the hostvars16:00
jeblairmordred, pabelanger: give me a minute, i'm *really* confused by these patches.16:00
mordredkk16:00
pabelangernow I am confused too16:00
jeblair473887 looks like it's *adding* something to the zk node data structure16:01
pabelangerhttp://paste.openstack.org/show/612440/ is the issue we have today, nodepool_region will always be null16:02
jeblairlike, nodepool is not passing the region at all currently, regardless of whatever we name it.  is that correct?16:02
mordredyes16:02
pabelangerjeblair: correct. We must have missed that before16:03
jeblairok.  then the zuul change is needed because 473887 chose a different name than what zuul was expecting16:03
jeblairokay, i think i understand these, and i think we should call the zk attribute "region".  that's to match "provider" (which is not provider_name)16:04
pabelangerokay, I can make that change16:04
mordredjeblair, pabelanger: on 473889 - I put in a question on the review - but it's "should those variables be top-level or be moved to the zuul variables dict"16:05
* mordred votes for move - but is fine if we pick the other thing16:05
jeblairmordred: hrm, it's in as a host var... but i guess they all just share the same namespace, huh?16:06
jeblairmordred: so yeah, i'm leaning toward move16:06
jeblairmordred: how do we accomplish that though?16:06
mordredit's easy - we just put them into the zuul dict - I can doa followup16:07
jeblairmordred: a zuul dict for a single host?16:07
pabelangerI'd be okay with a zuul.nodepool dict() if we went that roune16:07
pabelangerrout*16:07
pabelangerfetching coffee16:07
*** hashar has quit IRC16:07
jeblairmordred: would it be like this https://etherpad.openstack.org/p/mHTWbqUQ3M ?16:08
jeblair(would those get merged?)16:09
mordredyes. to both questions16:10
jeblaircool, in that case, zuul.nodepool_region or zuul.nodepool.region both wfm.16:10
*** isaacb has joined #zuul16:11
Shrewslet's be careful with that. i'm not 100% sure how that would affect current nodes with the old style metadata16:11
mordredjeblair: nope. I take it all back.16:12
jeblairmordred: it overwrites the whole thing?16:12
mordredjeblair: we'd need to turn on variable dict merging to get merging, and we've been avoiding that on purpose so far16:13
mordredjeblair: so I think ignoring me and keeping them top-level per-node as we have them is fine16:13
Shrewsoh, these aren't node metadata vars. nm16:13
jeblairok.  so i think we just revise pabelanger's patch for my bikeshed, then no change to zuul.16:13
mordredShrews: cool - I was just about to ask which thing we should be careful about16:13
mordredjeblair: yup16:13
pabelangerAgree, I don't think we should enable dict merge setting16:14
mordredjeblair: we _could_ make nodepool a dict to be consistent with zuul being a dict16:14
mordredbut I do not think it's essential that we do so16:14
jeblairmordred: agreed, that seems like it would be fine too, but <shrug>16:14
jeblairi guess we should decide rsn though.  :)16:14
pabelangernodepool dict seems okay to me16:15
mordredwoot16:15
jeblairwfm16:15
openstackgerritPaul Belanger proposed openstack-infra/nodepool feature/zuulv3: Add region to zookeeper  https://review.openstack.org/47388716:15
pabelangerokay, ^ should mean we can abandon 47388916:16
jeblairpabelanger: yep.  but if you want to move them into a dict, now's the time.  :)16:17
pabelangerk, I can do that too16:17
pabelangerjeblair: mordred: still as a host_vars right?16:20
mordredpabelanger: yah - I think just right there in that same function16:20
pabelangerk16:23
jlkmordred: do you know of any consumers that would appreciate such variables? Or do you feel like "if you build it, they will come"?16:28
openstackgerritPaul Belanger proposed openstack-infra/zuul feature/zuulv3: Create nodepool dictionary for ansible inventory  https://review.openstack.org/47388916:29
mordredjlk: yah. that's the real rub - I don't know of specific consumers- only that it's variables we provide for openstack inventory users ... and so far everything we've put upstream in ansible gets used way more than I expect :)16:30
jlkhah. I see there has been some more discussion. I agree it's a lot of info, maybe way more than should be exposed by default. As much as I hate adding more config toggles, this does seem like one.16:31
*** isaacb has quit IRC16:35
*** isaacb has joined #zuul16:36
*** jkilpatr_ has quit IRC16:39
*** jkilpatr has joined #zuul16:39
rcarrillocruzjust tested https://review.openstack.org/#/c/473809/1 on my nodepool instance16:41
rcarrillocruzdeleted a node16:41
rcarrillocruzconfirm the per-label group is there16:41
rcarrillocruzjeblair: are we good to go ^ ?16:41
jeblairrcarrillocruz: almost -- we should call it 'label' and not type; left a comment on patch.16:48
rcarrillocruzah ok, i follwed the Node convention16:49
rcarrillocruzthx, i'll amend16:49
jeblairyeah, this way it'll be one less thing we need to change :)16:50
rcarrillocruzso jeblair , i should wait for your patch to land or put a depends-on ?16:50
jeblairrcarrillocruz: i haven't written it yet, so why don't you just go ahead and rework yours to use 'label' wherever you can... so it might look like "nodepool_node_label=self._node.type", then i'll fix that when i change the rest of the zk stuff16:51
rcarrillocruzk16:51
jeblairrcarrillocruz: but at least you won't have to deal with the metadata variable changing16:51
openstackgerritMerged openstack-infra/nodepool feature/zuulv3: Add region to zookeeper  https://review.openstack.org/47388716:54
openstackgerritRicardo Carrillo Cruz proposed openstack-infra/nodepool feature/zuulv3: Create group for label type  https://review.openstack.org/47380916:55
*** jkilpatr has quit IRC17:02
rcarrillocruzthx folks17:03
*** jkilpatr has joined #zuul17:04
* SpamapS sits back down at laptop, refreshed from 2 days of legolanding.17:04
*** isaacb has quit IRC17:07
jlkgood times!17:13
jlkmy boys were so bummed when we went that we were missing the opening of Ninjago by a couple weeks.17:13
openstackgerritPaul Belanger proposed openstack-infra/zuul feature/zuulv3: Add ssl support to gearman / gearman_server  https://review.openstack.org/47391617:15
pabelangerjeblair: okay, I think we first need ^ for gearman SSL. I didn't see anything in zuul today for it. I'd also like to setup a unit test for SSL, but want to make sure that is the right step first17:17
SpamapSjlk: It was meh for me, but my boys did in fact enjoy it since it was the only video game they got to play all day.17:19
SpamapSjlk: (re: Ninjago)17:19
jeblairpabelanger: yep agreed -- though i think we need to use "has_option" because if we don't, doesn't configparser.get raise an exception if the value isn't there?17:30
pabelangerjeblair: Ya, that is something I wanted to test. I thought it defaulted to None, but untested.17:31
pabelangerworking on generating our self-signed certs17:31
jeblairpabelanger: yeah, we need has_option: http://paste.openstack.org/show/612449/17:33
pabelangerjeblair: thanks17:34
openstackgerritMerged openstack-infra/nodepool feature/zuulv3: Create group for label type  https://review.openstack.org/47380917:34
jeblairjlk: cache change lgtm17:37
jlkyay!17:37
jeblairmordred: i commented on 47309017:55
SpamapSFYI, 7/11, CNCF would like another demo of Zuul, this time with v3. :)18:12
mordredSpamapS: sweet18:14
mordredjeblair: I have responded - thanks!18:34
*** hashar has joined #zuul18:36
jlkSpamapS: care to take a look at https://review.openstack.org/472468 and perhaps give it workflow?18:43
SpamapSjlk: looking now19:02
SpamapSzomg summer WFH is such a tire fire sometimes19:17
* SpamapS is going to have to escape to a coffee shop me thinks19:17
jlk:(19:18
SpamapSjlk: review progressing :)19:21
Shrewsmordred: after the meeting, we might need to put our heads together on your "follow_log" streaming logic. It doesn't seem to stop streaming on EOF19:43
mordredShrews: kk. the logic for serving on the node itself? or the logic for reading it on the executor?19:45
Shrewsmordred: the serving code19:46
openstackgerritMerged openstack-infra/zuul feature/zuulv3: Implement a cache of github change objects  https://review.openstack.org/47246819:46
Shrewsmordred: like, maybe if the file has NOT been truncated, is it ok to just return False since we've only gotten to that point (i think) if we've read all of the file?19:47
Shrewsreturning False should terminate the streaming19:47
mordredShrews: well, that is on purpose. it's basically doing a tail -f of the log and doesn't know if more things wil be added19:47
mordredShrews: HOWEVER - now that we're doing logfile-per-command, we could potentially change that logic, since we don't need to support subsequent commands appending to the file19:48
Shrewsmordred: that might work for the ansible plugin, but i'm talking about in reference to the finger streamer (where i copied that code)19:49
mordredgotcha19:49
Shrewsdoesn't seem like we'll ever really know if we've reached the true EOF19:49
mordredso - yah - let's rethink this part - because there have been some structural changes since it was written19:50
mordredShrews: on the node, we can definitely write an EOF to the log file, because we don't re-use log files across tasks on the node anymore19:51
mordredShrews: on the executor, we also know when we're done with the logfile - that's when we run zuul_console: state=absent19:52
mordredShrews: so the executor streamer can write a "yup, I'm done"19:52
mordredShrews: and then the finger streamer can notice that and stop19:53
mordredShrews: right?19:53
jeblairmordred, Shrews: the executor streamer knows it's done when the file is *deleted*19:53
Shrewshrm, yeah. maybe a presence check is what's needed there19:54
jeblairso while the node streamer is doing a tail -f (and i agree, that can probably change now), the executor streamer can replace that "did the file get truncated?" check with a "did the file get deleted?"19:54
mordredjeblair: yes. agree19:55
Shrewsright, we won't ever truncate that log19:55
jeblairthough, we'll have to think about startup -- what if someone requests a stream before the job starts?  then the file won't exist -- yet.19:55
Shrewsthe finger streamer returns "log file not found" in that case19:55
* SpamapS smells a lockfile coming19:55
jeblairhowever, the executor doesn't return the streaming information/url until after the job starts... so... maybe it's okay?19:55
jeblairwell, it sends it right before it runs ansible for the first time19:56
Shrewsor, if the build dir doesn't exist yet, "invalid build uuid"19:56
jeblairso... i guess there's a tiny race there.19:56
jeblairShrews: aha!19:56
jeblairShrews: build dir exists, but log file doesn't means that the job hasn't started yet.  so the streamer can wait for the log to show up (if we want to be nice).19:56
jeblairShrews: build dir doesn't exist means the job is finished.  terminate connection.19:57
Shrewsjeblair: yeah, we could be nicer that way19:57
mordred++ to both19:58
mordredbtw - the executor streamer can stop streaming even before the file is deleted if we want - "[Zuul] Task exit code: %s" is the last line emitted by the command module20:00
mordredOH - I say that - I actually see a logic flaw there20:00
jeblairmordred: there may be more tasks, so the executor streamer can't stop, right?20:02
openstackgerritJesse Keating proposed openstack-infra/zuul feature/zuulv3: Implement pipeline requirement of github labels  https://review.openstack.org/47396220:02
mordredjeblair: it can stop streaming that task20:02
jeblairmordred: oh, as a client.  yes.20:02
Shrewsjeblair: file exists check seems to work. good idea20:02
jeblair(i thought you made it do that? i recall a check for that line)20:02
mordredjeblair: it can also stop streaming that particular file20:02
mordredjeblair: I do on the executor20:02
mordredjeblair: but I was suggesting we could do the same check in the node streamer so that the thread streaming a given file could terminate20:03
jeblairmordred: ah, you said 'executor streamer' above.  okay, resetting --20:03
mordredsorry. we have many pieces20:04
jeblairmordred: yeah, i think that's okay.  just one thing we'll need to make sure of is that in both places (node and executor streamers), we flush all the data output before we close any open connections.20:05
mordred++20:05
pabelangerSo, has anyboby tried using SSL with python gear recently? I keep getting SSLError: [SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:661) back from my SSL certs I have generated20:06
Shrewspabelanger: i added that code loooooong ago (4+ years?). worked at the time.  :)20:06
jeblairShrews: i think you added tests too? :)20:07
pabelangerYa, that's what I see.  I've been following https://dst.lbl.gov/~boverhof/openssl_certs.html for my server / client / ca certs20:07
Shrewswow, /me just realizing how long he has been in this openstack stuff20:07
jeblairShrews: don't look down20:07
pabelangerI am starting to thing ssl_version=ssl.PROTOCOL_TLSv1 might be part of the issue20:07
SpamapSindeed20:08
jeblairah, we use fixed certs in the tests, so we're not generating new ones or anything20:08
jeblairpabelanger: what emits that error?20:11
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Make sure we always log the exit line  https://review.openstack.org/47396620:11
pabelangerjeblair: gear.Server: http://paste.openstack.org/show/612463/20:13
pabelangerafk for a few minutes, need to stretch legs20:13
openstackgerritDavid Shrewsbury proposed openstack-infra/zuul feature/zuulv3: Stop streaming from server when log file removed  https://review.openstack.org/47396720:13
Shrewsjeblair: mordred: ^^^ this seems to work20:13
jeblairpabelanger: are you sure the client is using ssl?20:14
*** jkilpatr has quit IRC20:15
pabelangerjeblair: I believe so, let me confirm again20:17
jeblairpabelanger: there should be a "Using SSL" debug log line right after "Connecting to"20:18
SpamapSyou can always 'openssl s_client -connect foo:4730' and try some admin commands :)20:19
jeblairya20:19
pabelangerthat is a good idea20:20
pabelangerjeblair: I don't see that, sorry was looking20:20
pabelangerlet me first debug with geard client20:20
*** jkilpatr has joined #zuul20:31
openstackgerritMerged openstack-infra/zuul feature/zuulv3: Use threads instead of processes in zuul_stream  https://review.openstack.org/47285020:38
openstackgerritDavid Shrewsbury proposed openstack-infra/zuul feature/zuulv3: Add log streaming test  https://review.openstack.org/47107920:39
openstackgerritMerged openstack-infra/zuul feature/zuulv3: Stop streaming from server when log file removed  https://review.openstack.org/47396720:39
Shrewsjeblair: okie dokie, reworked that ^^^ to use the thread for streaming. Although I could remove a couple of sync points from the job itself, I had to add some artificial spinwaits to wait for things to get populated20:40
openstackgerritMerged openstack-infra/zuul feature/zuulv3: Don't wait for forever to join streamer  https://review.openstack.org/47283920:40
Shrewsjeblair: oh, this is fun. ever seen this seeming non-related fail? http://paste.openstack.org/show/612464/20:42
openstackgerritMerged openstack-infra/zuul feature/zuulv3: Use display.display for executor debug messages  https://review.openstack.org/47284020:42
jeblairShrews: no, but i've never had a config/streamer/ directory -- did you lose that in some git operation somehow?20:43
Shrewsjeblair: nope. that's being added for that test20:44
Shrewshappened while running the test 100 times20:44
Shrewsfailed on run 7120:44
jeblairShrews: yeah, but it looks like it's not where it's expected on your local system20:45
jeblairShrews: did you perform any git operations while running those tests in the background?20:45
Shrewshrm, maybe doing the 'git review' during the run caused issues20:45
Shrewsyeah20:45
Shrewsmust be it20:45
jeblairyes, it absolutely did as part of the rebase check.20:45
* Shrews makes mental note to not do that20:45
jeblairShrews: or 'git review -R' if you really want to20:46
jeblairclarkb: can you take a look at 472964 ?20:48
clarkbjeblair: mordred my preference on the log indexing/parsing/etc side of things is to not make things conditionally different20:51
clarkbit complicates the rules necessary to parse and index and query20:51
jeblairclarkb: generally, i agree -- is it worth having the "hostname" (eg, ubuntu-xenial) in every log line on every single-node job though?  or is that a parsing complexity we'd be willing to accept in order to simplify the most common logs?20:52
mordredclarkb: ok- so the prepended hostname on every logline you don't think will bother folks?20:52
clarkbit wouldn't bother me, we often have people confused about what jobs ran on and that would make it pretty explicit20:53
mordredthat's a good point20:53
jeblair(yes, though i think even now our pre-playbook outputs that at the top of the job)20:53
clarkbI don't feel strongly about it. The difference in logstash rules for that is an optional match group for hostname20:55
clarkbso fairly straightforward20:56
mordredjeblair: I can split the if len > 1 bit out into its own patch and put it at the end if we want to get the rest of that stack in but till talk about the hostname part20:57
mordredoh - I guess there is nothing pressing on top of it20:57
jeblairi also don't feel strongly.  however, based on what i've seen, i think perhaps this is one of those cases where we can make users' lives better at a small cost to our logstash config complexity.  so if it's not too hard to do, let's do that.  if there's a compelling user story for always having it, we can always stick it back in.20:58
mordredjeblair: kk20:58
clarkbis there a problem we are trying to solve with it? just concern that too much info will make logs harder to read?20:59
clarkband if ^ is the case why are we not worried about it for multinode?20:59
jeblairclarkb: well, it's required by multinode in order to have any hope of reading it21:00
mordredclarkb: yah. but for single node it winds up being a change from the current log format21:00
clarkbright, but if users are capable of that wouldn't they be capable of it on signle node too?21:00
mordredso since it's a change, it made me want to think about whether the change would increase or decrease user happiness21:00
clarkbI'm personally a fan of consistency21:01
clarkbalso journalctl outputs with hostname by default too21:01
jeblairi think users will be able to handle it; i think it just might be annoying -- the same thing repeated on every line... though now that i say that... mordred, does the hostname perhaps make an easy way to distinguish between "ansible" log lines vs "console" log lines?21:02
clarkb(so having hostname there would potentialyl make it easier on people used to journalctl output)21:02
jeblair(now i wish i had one of these handy, but we're broken right now)21:02
mordredclarkb: is there an example default journalctl output lying around anywhere?21:02
clarkbmordred: just run journalctl -f on your laptop :)21:02
clarkbJun 13 08:49:10 nibbler systemd[2605]: Started XFCE notifications service.21:02
clarkbfor example21:02
clarkbnibbler is the hostname here21:02
jeblairhttp://zuulv3-dev.openstack.org/logs/0a06c318cf2340e7a99f08a7b6127164/ansible_log.txt21:03
mordredjeblair: yah - but a bunch has changed there21:03
jeblairmordred: hrm, it kind of does -- ignoring the stuff we've changed (like fixing dual timestamps, etc) -- the hostname does kind of make a nice indication that something is a console line vs an ansible line...21:03
mordredjeblair: yah - it's not terrible. why don't we just keep it for a bit and after we read logs for a little while revisit?21:05
mordredjeblair: so I'll pull just that bit out of the stack but leave the other bits so that it's trivial to re-add if we want21:05
jeblairmordred: sgtm!21:05
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Change log streaming link to finger protocol  https://review.openstack.org/43776421:09
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Extract get_playhosts listing in zuul_stream to a method  https://review.openstack.org/47296421:09
openstackgerritMonty Taylor proposed openstack-infra/zuul feature/zuulv3: Only prepend hostname on multi-node plays  https://review.openstack.org/47398521:09
mordredjeblair: kk. there is is reorganized a bit21:10
pabelangerokay21:15
pabelangerwould love some body to validate my steps for SSL and gearman: http://paste.openstack.org/show/612468/21:16
pabelangertaking a break before I dive back into it21:20
*** dkranz has quit IRC21:27
Shrewspabelanger: it may be that self-signed certs won't work? i seem to recall some sort of issue around those, but i really don't remember21:32
SpamapSpabelanger: I usually use CA.sh21:32
jeblairShrews: i think pabelanger's example signs the certs with a ca key, so they shouldn't be (only) self-signed21:33
Shrewsjeblair: k. like i said, my memory on that is fuzzy. 'self-signed' brought up bad feelings that i can't put a finger on atm21:35
Shrewswish i could remember, but that would risk bring up memories of libra21:40
Shrews*shudder*21:40
jeblairpabelanger: i confirm your error21:47
jeblairpabelanger: this process, however, works for me: http://paste.openstack.org/show/612472/21:48
jeblairpabelanger: i suspect the problem is the expiration -- if i look at the certs i generate with your instructions, their begin and end dates are the same21:57
jeblairpabelanger: oops, i'm wrong about that, nevermind21:58
jeblairpabelanger, Shrews: oh wait -- i think those might be self-signed!22:01
jeblairi mean, there's a CA cert there and all22:01
jeblairbut i think if you enter the same values for the subject information, you get that error22:02
jeblairpabelanger: i can't tell what you entered since you didn't include that in the paste, or use the -subj line22:02
jeblairbut that is something that's different about my procedure -- i use -subj and provide something different for each cert22:03
pabelangerjeblair: I just used the default values provided by openssl, I just pressed enter for most prompts22:05
pabelangerlet me make sure client and server different values22:05
pabelangerjeblair: yes, that was the issue. By using a different subject for client / server / ca, I no longer get errors. Thank you22:09
*** hashar has quit IRC22:12
*** hashar has joined #zuul23:02
openstackgerritJesse Keating proposed openstack-infra/zuul feature/zuulv3: Implement pipeline reject filter for github  https://review.openstack.org/47400123:10
* jlk hangs up his hat for the day.23:11
*** hashar has quit IRC23:28

Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!