[00:00:29] <twentyafterfour>	 I see Krinkle added a subtask
[00:00:40] <Krinkle>	 twentyafterfour: this one, or a differnt one that I removed again?
[00:00:46] <Krinkle>	 twentyafterfour: check you double check I got th 14 digit timestamp right?
[00:00:46] <Krinkle>	 https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/631318/
[00:00:56] <Krinkle>	 I tend to blind myself in those
[00:01:02] <twentyafterfour>	 I'll check
[00:02:33] <twentyafterfour>	 shouldn't it be 20200930? was it already after midnight utc by the time this happened?
[00:02:34] <Krinkle>	 twentyafterfour: dirty diff wikiversions.json on deploy1001
[00:02:52] <Krinkle>	 twentyafterfour: you're right, thanks
[00:02:54] <twentyafterfour>	 Krinkle: yeah I never committed and pushe the rollback sorry
[00:03:11] <twentyafterfour>	 want me to fix that now?
[00:03:37] <Krinkle>	 CR for you
[00:03:48] <Krinkle>	 I'll wait this time :)
[00:03:59] <Krinkle>	 twentyafterfour: yeahy, please undo or commit however it shoudl be
[00:06:08] <twentyafterfour>	 man gerrit is running real slow
[00:06:44] <Krinkle>	 oops, I did a git pull
[00:06:49] <Krinkle>	 aborting merge
[00:07:28] <twentyafterfour>	 it's taking forever to resovle deltas on what should be a tiny change to wikiversions
[00:07:31] <twentyafterfour>	 not sure what's happening
[00:08:17] <twentyafterfour>	 ok I aborted my push
[00:08:30] <twentyafterfour>	 yeah that doesn't like like the right tree
[00:09:21] <twentyafterfour>	 Krinkle: not sure how to fix this?
[00:09:39] <Krinkle>	 twentyafterfour: git reset origin/master perhaps?
[00:09:44] <twentyafterfour>	 my commit was just to put the repo in sync with the state on disk
[00:09:45] <Krinkle>	 then redo whatever you want to change
[00:09:47] <twentyafterfour>	 ok yeah that's fine
[00:09:59] <Krinkle>	 this will default to keeping the dirty stuff in sating
[00:10:11] <Krinkle>	 git reset --hard origin/master will undo it for real and keep only what's in the upstream repo
[00:10:36] <twentyafterfour>	 ok so it looks like your second patch with the date has a merge conflict?
[00:10:46] <twentyafterfour>	 oh not anymore
[00:11:11] <Krinkle>	 awaiting your push/merge
[00:11:49] <twentyafterfour>	 wtf it won't let me push
[00:12:03] <twentyafterfour>	  ! [remote rejected]     HEAD -> refs/for/master/1.36.0-wmf.11 (branch master/1.36.0-wmf.11 not found)
[00:12:05] <twentyafterfour>	 error: failed to push some refs to 'ssh://twentyafterfour@gerrit.wikimedia.org:29418/operations/mediawiki-config'
[00:12:13] <Krinkle>	 I generally don't like editing/pushing from deploy host. I've always preferred local commits, +2 first with test and then pull/deploy. perhaps changing wikiversions could be a simple command in the local bin/ directory like mwexpanddblist that we run locally and then push as a normal commit.
[00:12:29] <Krinkle>	 also doesn't require storing gerrit credentials in prod which seems preferrable
[00:12:31] <twentyafterfour>	 I prefer that too but this was an emergency
[00:12:36] <Krinkle>	 yeah
[00:12:38] <Krinkle>	 fair :)
[00:12:47] <Krinkle>	 but we don't have this for non-emergency either, right?
[00:12:52] <twentyafterfour>	 faster rollback if you don't have to wait for gerrit+jenkins
[00:13:02] <twentyafterfour>	 right
[00:14:44] <Krinkle>	 I can repro on comons /w/index.php?title=Image:SouthAfricanStub.png&action=render&uselang=he via mwdebug2001
[00:14:48] <Krinkle>	 nobody purge that
[00:15:14] <twentyafterfour>	 nice
[00:17:11] <Krinkle>	 Cached time: 20201001001638
[00:17:13] <Krinkle>	 still happens
[00:17:16] <Krinkle>	 this is a newer entry
[00:18:15] <RoanKattouw>	 twentyafterfour: "msater/1.36.0-wmf.11"?
[00:18:21] <RoanKattouw>	 Should that not be wmf/1.36.0-wmf.11?
[00:18:34] <twentyafterfour>	 uh
[00:18:45] <RoanKattouw>	 Oh wait nm
[00:18:50] <RoanKattouw>	 I see the full context now
[00:18:59] <twentyafterfour>	 the version was just a topic
[00:19:05] <RoanKattouw>	 Yeah gotcha
[00:19:24] <Krinkle>	 https://logstash.wikimedia.org/app/kibana#/dashboard/mwdebug1002
[00:19:35] <Krinkle>	 OK, I don't know what's going on.
[00:19:58] <Krinkle>	 I was able to reproduce hte warning after the config patch applied there
[00:20:03] <Krinkle>	 and the cached entry is from after the rollback completed
[00:20:13] <twentyafterfour>	 wth
[00:20:20] <Krinkle>	 https://commons.wikimedia.org/w/index.php?title=Image:Soy-whey-protein-diet.jpg&action=render&uselang=es specifically
[00:20:56] <twentyafterfour>	 uh
[00:20:57] <Krinkle>	 so it looks like wmf.10 is producing new corruptions
[00:21:06] <Krinkle>	 the page looks broken but that's normal
[00:21:09] <Krinkle>	 this is a raw call without skin
[00:21:35] <twentyafterfour>	 the timing really coincided with the rollout of wmf.11 to group1 pretty perfectly ... I don't know what else it could be?
[00:21:41] <Krinkle>	 wmf.10 on mwdebug2001 did a fresh parse and produced the same error
[00:21:52] <twentyafterfour>	 weird
[00:22:04] <Krinkle>	 and it's unlikely to have opcache corrupted her in the same esact way again'
[00:22:16] <twentyafterfour>	 maybe something that was backported right before the train
[00:22:34] <Krinkle>	 Maybe
[00:22:40] <Krinkle>	 or maybe iut's not parser cahce that's the issue but something in memcached
[00:22:42] <twentyafterfour>	 and somehow it just didn't show up on kibana until I sync'd? not sure how that would be
[00:22:50] <Krinkle>	 you guys were talking about BagOstuff/memcached earlier
[00:22:51] <twentyafterfour>	 hmm
[00:22:57] <Krinkle>	 is there a separate key involved for something here?
[00:23:10] <Krinkle>	 I thought CacheTime was a sub object of ParserCache
[00:23:17] <Krinkle>	 RoanKattouw: are you caught up more or less?
[00:23:34] <RoanKattouw>	 I was on a call, just came back
[00:23:37] <Krinkle>	 $optionsKey = $this->cache->get(
[00:23:37] <Krinkle>	 			$this->getOptionsKey( $wikiPage ), BagOStuff::READ_VERIFIED );
[00:23:37] <Krinkle>	 		if ( $optionsKey instanceof CacheTime ) {
[00:23:42] <Krinkle>	 OK. I understand this better now
[00:23:49] <Krinkle>	 I have no idea this code existed.
[00:24:12] <Krinkle>	 wait no, this cache is the parser cache
[00:24:13] <Krinkle>	 it's not memc
[00:25:09] <Krinkle>	 but the error happens in getKey()
[00:25:21] <Krinkle>	 before RejectParserCacheValue runs in get()
[00:25:42] * twentyafterfour gets coffee, brb
[00:25:53] <Krinkle>	 It still doesn't explain, however, how it is that an hour after we rolled back that still new bad CacheTime objecst were put into the parsercache
[00:26:08] <Krinkle>	 but at least it explains why the config patch will not be able to purge that
[00:30:41] <Krinkle>	 Relevant branch diff: https://github.com/wikimedia/mediawiki/compare/wmf/1.36.0-wmf.10...wmf/1.36.0-wmf.11#diff-a0afa6d40f254cfbd7c1de51ce0e8bc2
[00:31:06] <Krinkle>	 anyway, we know the wmf.11 code is bad. it made it private which is not backward-compatible
[00:31:20] <Krinkle>	 the question is now, how do we make MW ignore those entries.
[00:31:37] <Krinkle>	 RoanKattouw: leaning toward a new wmf.10-only patch in core that checks for it being set and returns false if not.
[00:32:48] <RoanKattouw>	 It made what private?
[00:32:54] <RoanKattouw>	 Sorry I'm not caught up on the issue here
[00:34:10] <Krinkle>	 RoanKattouw: see above wmf.10..11 diff. CacheTime::mUsedOptions was made private. Which is not backward-compatible with wmf.10.
[00:34:25] <Krinkle>	 also even while on wmnf.11 there were other issues with it
[00:34:28] <Krinkle>	 we rolled back
[00:34:41] <Krinkle>	 now we have 2 hours of broken parser cache entries causing 100s of erros every minute
[00:35:15] <Krinkle>	 I tried to purge them with RejectParserCacheValue  in wmf-config but that didn't work because the failures are from before that hook runs.
[00:36:15] <Krinkle>	 also there is still the unanswered question of how it can be that new broken versions of this cache key were persited to parsercache after we rolled back. I kind of want to know how that is possible, because if we do add an isset() there and cast them to expired/false, and if it is still happening with currently deployed code and newly parserd pages, that would effectively disable parser cache
[00:38:19] <RoanKattouw>	 Oh so you have evidence of new PC entries being created without mUsedOptions while wmf.10 was running?
[00:38:50] <Krinkle>	 Indeed
[00:38:50] <Krinkle>	 https://logstash.wikimedia.org/app/kibana#/dashboard/mwdebug1002
[00:38:51] <RoanKattouw>	 The diff link didn't highlight anything for me, it's just showing me a commit list
[00:39:06] <Krinkle>	 RoanKattouw: ah, click on "Files" then ctrl-f to "parser/"
[00:39:15] <Krinkle>	 I assumed the permalink pointed there
[00:39:36] <Krinkle>	 This Kibana URL shows wmf.10 (old version, all wikis now) and me accessing a commons url that trigers the issue
[00:39:47] <Krinkle>	 and view-source shows it was cached a few minutes before I made that request
[00:39:56] <Krinkle>	 an hour after we rolled back already
[00:40:10] <Krinkle>	 it was host:mwdebug2001 so it was a pc cache miss
[00:40:26] <Krinkle>	 visible stil at https://commons.wikimedia.org/w/index.php?title=Image:Soy-whey-protein-diet.jpg&action=render&uselang=es
[00:40:36] <Krinkle>	 Parsed by mwdebug2001
[00:40:36] <Krinkle>	 Cached time: 20201001001638
[00:40:43] <Krinkle>	 I don't understand how that's possible.
[00:43:44] <Krinkle>	 untested patch - https://gerrit.wikimedia.org/r/c/mediawiki/core/+/631324/
[00:43:46] <RoanKattouw>	 Hmm
[00:43:57] <Krinkle>	 again, if somehow new entries are made with wmf.10, this would be a bad patch
[00:43:58] <RoanKattouw>	 I guess we don't have a reliable way of finding out what was actually on mwdebug2001 at that time
[00:44:15] <Krinkle>	 It was wmf.10 per the stack trace
[00:44:21] <Krinkle>	 I had just sca-pulled the wmf-config patch there
[00:44:34] <Krinkle>	 for rejecting entries from the 19:00-21:00 timeframe
[00:44:44] <RoanKattouw>	 Oh right
[00:44:58] <RoanKattouw>	 Yeah your patch makes sense, and also yes it would be bad if wmf.10 still generated entries like these
[00:45:05] <Krinkle>	 well, I suppose the stack trace isn't definitive per se, since it's on cache-access
[00:45:12] <twentyafterfour>	 so there really was an opcache corruption coinciding with this, right?
[00:45:26] <Krinkle>	 I don't know actually how that came into being either. I did open it multiple times
[00:45:30] <twentyafterfour>	 it certainly seemed to be an opcache bit switch corruption that caused the majority of errors
[00:45:51] <Krinkle>	 yeah, those were fatals for various Math and Wikibase classes not existing.
[00:45:56] <RoanKattouw>	 OK so here's what I propose
[00:46:08] <RoanKattouw>	 Let's search in logstash to see if there are other PC corruptions like this
[00:46:24] <Krinkle>	 https://logstash.wikimedia.org/app/kibana#/dashboard/mediawiki-errors is producing 2000+ per minute
[00:46:25] <RoanKattouw>	 idk how easy it will be to distinguish wmf.10-generated ones from wmf.11-generated ones though
[00:46:29] <RoanKattouw>	 Oh yikes
[00:46:30] <Krinkle>	 but that is cache-access, not per se new parse
[00:46:54] <Krinkle>	 we can go through a few URLs with logged-out./canoical pereferences and see if some are from Oct 1
[00:47:20] <RoanKattouw>	 Yeah I guess, but we won't easily be able to prove that your example is the only one that was generated by wmf10
[00:48:06] <Krinkle>	 https://en.wiktionary.org/wiki/luhodluhod  / Parsed by mw2282 / Cached time: 20200930210119 / <meta name="generator" content="MediaWiki 1.36.0-wmf.10"/>
[00:48:07] <RoanKattouw>	 We could try running your patch for a little while and see if it wreaks havoc, but that would also be hard to tell. We'd expect to see a bunch of parser cache misses early on, the real problem would be if the same page keeps missing
[00:48:23] <Krinkle>	 this is generator of cache-access
[00:48:32] <Krinkle>	 the timestamp is from when it was wmf.11 so this one makes sense
[00:48:32] <RoanKattouw>	 Right
[00:48:42] <RoanKattouw>	 What was the time window that we had wmf.11 deployed?
[00:48:52] <Krinkle>	 it ended 19:30 -  21:05 is "bad"
[00:48:57] <Krinkle>	 started
[00:49:01] <RoanKattouw>	 Gotcha
[00:49:09] <RoanKattouw>	 So this was a few minutes before the rollback
[00:49:37] <RoanKattouw>	 So if your fears are true, we should be able to find more articles that have a broken ParserCache entry that was saved before 19:30 or after 21:05
[00:49:49] <Krinkle>	 commons /w/index.php?title=Image:Cyanid-Ion.svg&action=render&uselang=sr - another one that makes sense
[00:50:03] <Krinkle>	 RoanKattouw: that's right.
[00:50:10] <Krinkle>	 I assume onlhy after because efore we had no php errorrs
[00:50:25] <Krinkle>	 I can stage the patch on mwdebug2001 and just browse some random urls
[00:50:32] <Krinkle>	 see if the log message gets triggered
[00:50:56] <Krinkle>	 and yeah, we could do a quick deploy and stand by
[00:50:58] <twentyafterfour>	 so the first scap which was for group0 was finished at 19:12:42
[00:51:48] <RoanKattouw>	 Oh hmm yeah maybe let's try that
[00:51:55] <RoanKattouw>	 I'll gather a few URLs that are currently broken
[00:52:23] <RoanKattouw>	 We'll stage your patch on mwdebug2001, then purge those pages on mwdebug2001, and check if the issue goes away
[00:52:35] <Krinkle>	 ack
[00:52:51] <Krinkle>	 I've force mergd the patch as it woudl take 25-35min otherwise
[00:52:56] <Krinkle>	 will let jenkins run while we test
[00:53:09] <RoanKattouw>	 If we can get a dozen confirmed broken pages to be fixed by rerendering them on mwdebug2001, and none of them stay broken, then I will comfortable enough to deploy the patch
[00:53:19] <RoanKattouw>	 We'd still need to carefully watch the parser cache miss rate and be prepared to revert
[00:54:09] <Krinkle>	 Live on mwdebug2001.codfw
[00:54:17] * Krinkle looks at Grafana
[00:54:27] <RoanKattouw>	 Uhh wait I found another broken one that was generated by wmf.10
[00:54:29] <RoanKattouw>	 https://commons.wikimedia.org/w/index.php?title=Image:Matabeleland_-_Zimbabwe.svg&action=render&uselang=ang
[00:54:42] <Krinkle>	 https://grafana.wikimedia.org/d/000000106/parser-cache?orgId=1&from=now-2d&to=now
[00:54:45] <RoanKattouw>	 <!-- Saved in parser cache with key commonswiki:pcache:idhash:73993907-1!userlang=ang!wb=3 and timestamp 20201001004959 and revision id 453053759  -->
[00:55:31] <Krinkle>	 RoanKattouw: so, it looks like there are two parser cache keys involved with a parser cache fetch, is that right? One that stores a small CacheTime and one that stores the ParserOutput?
[00:55:48] <RoanKattouw>	 Although, maybe not, the time of the error and the time of that cache key are identical
[00:55:53] <Krinkle>	 I don't know why it is this way, but that's the way it is, right?
[00:55:55] <RoanKattouw>	 That's possible
[00:56:01] <RoanKattouw>	 Let me look at the code
[00:56:13] <Krinkle>	 could be that the CacheTime is surviving longer or separeately for some reason
[00:56:24] <Krinkle>	 and that these are only new ParserOutput objects
[00:56:33] <Krinkle>	 I don't know why they would be 1:many but if that's the case, it woudl explain it
[00:57:17] <Krinkle>	 so per Grafana it loooks like our hit rate has not suffered thus far, so the theory that these errors are causing *something* to be cached is correct.
[00:57:54] <RoanKattouw>	 Oh hmm yeah that would be interesting
[00:57:58] <Krinkle>	 well, likely anyway. it's also possible that the time window is too small to cause a big drop
[00:58:01] <RoanKattouw>	 The actual issue is with CacheTime objects, right?
[00:58:05] <Krinkle>	 that's right
[00:59:23] <Krinkle>	 $msg = "Saved in parser cache with key $parserOutputKey"
[00:59:27] <Krinkle>	 this is added to ParserOutput
[00:59:28] <Krinkle>	 but..
[00:59:36] <Krinkle>	 " and timestamp $cacheTime"
[00:59:42] <Krinkle>	 cacheTime looks like it is given to both
[00:59:56] <Krinkle>	 so upon save() we do generally seem to set both
[01:02:50] <Krinkle>	 patch live on mwdebug2001.codfw
[01:05:20] <Krinkle>	 https://logstash.wikimedia.org/goto/ab5bedef86f74c9058f22f768f6125f4
[01:05:31] <Krinkle>	 Kibana view on mwdebug2001 reduced to PHP erorrs and ParserCache channel
[01:05:40] <Krinkle>	 (given this mesasge rrequires me to enable verbose mode)
[01:07:17] <Krinkle>	 I'm rolling out the CommonSettings.php to other servers now first as well
[01:07:20] <Krinkle>	 that was still staged
[01:07:24] <RoanKattouw>	 Thanks, I was just trying to figure out how to do that (in logstash
[01:07:31] <Krinkle>	 which rejects the 19:00-21:00 range
[01:07:49] <RoanKattouw>	 I tried a couple of known bad articles, purging them on mwdebug2001 fixed the error both on mwdebug and in productoin
[01:07:57] <RoanKattouw>	 Oh OK so that'll help
[01:08:05] <RoanKattouw>	 We could try that first and see what errors remain
[01:11:32] <Krinkle>	 I think scap is stuck
[01:11:43] <Krinkle>	 twentyafterfour: https://logstash.wikimedia.org/goto/90e5bd41093f68c87e6f0eb428c936a0
[01:11:53] <Krinkle>	 nothing since Finished sync-proxies (duration: 00m 01s)
[01:11:59] <Krinkle>	 sync-apaches:   0% (ok: 0; fail: 0; left: 367)
[01:12:52] <Krinkle>	 ok now it went through all at once after I scrolled back down in my screen
[01:12:54] <Krinkle>	 that's.. weird
[01:13:02] <Krinkle>	 process blocked by output?
[01:13:04] <Krinkle>	 cli output
[01:13:07] <Krinkle>	 anyway
[01:13:14] <Krinkle>	 stack.pop() stack.pop()
[01:13:29] <twentyafterfour>	 hmm
[01:13:45] <cdanis>	 did you hit enter a few times and then it worked?
[01:13:56] <cdanis>	 (wasn't that a thing?)
[01:14:03] <Krinkle>	 I just scroll back down in the ctrl-a scroll mode thingy in screen
[01:14:09] <twentyafterfour>	 Krinkle: we've had it get blocked before and hitting enter would unblock it
[01:14:18] <Krinkle>	 great
[01:14:22] <Krinkle>	 I'll remember that :)
[01:14:29] <twentyafterfour>	 never can reproduce it
[01:15:01] <Krinkle>	 I'm seeing the expected blib of forced expired entries at https://grafana.wikimedia.org/d/000000106/parser-cache?orgId=1&from=now-6h&to=now
[01:15:05] <twentyafterfour>	 tried and tried to understand what would cause it and couldn't even after studying the source of python stdlib and cpyton
[01:15:13] <Krinkle>	 "Miss (rejected)"
[01:16:16] <Krinkle>	 twentyafterfour: maybe screen copy/select/scroll buffer creates the circumstances in an easiier to repro way. or maybe not. I haven't seen it happen before, but I also haven't tried scrolling before during a sync I htink
[01:17:00] <Krinkle>	 01:12 UTC is the time of finishing this config sync
[01:17:05] <Krinkle>	 e.g. for logstash seeing of errors
[01:18:01] <Krinkle>	 2020-10-01T01:17:34  mw2272 commonswiki /wiki/File:Antonio_Casale_in_%22The_Good,_the_Bad_and_the_Ugly%22,_1966.jpg   ErrorException from line 116 of /srv/mediawiki/php-1.36.0-wmf.10/extensions/Math/src/MathHooks.php: PHP Warning: in_array() expects parameter 2 to be array, null given
[01:19:53] <RoanKattouw>	 I saw one for https://commons.wikimedia.org/wiki/File:Giovanni_Paolo_Panini_-_Alexander_the_Great_at_the_Tomb_of_Achilles_-_Walters_37510.jpg too
[01:20:12] <RoanKattouw>	 But when I visited it, I got a pcache timestamp that was later than the error message
[01:20:38] <RoanKattouw>	 And it didn't error again
[01:21:32] <Krinkle>	 that one, logged-out, gives <!-- Saved in parser cache with key commonswiki:pcache:idhash:18845440-0!wb=3 and timestamp 20201001011843 and revision id 457332991
[01:21:54] <RoanKattouw>	 Yeah I saw the same thing
[01:21:56] <Krinkle>	 and without varrnish.. the same
[01:21:59] <Krinkle>	 still
[01:22:08] <RoanKattouw>	 Error from 01:17:xx, when I visited the pcache timestamp was 01:18:xx
[01:22:19] <Krinkle>	 right
[01:22:32] <RoanKattouw>	 Hmm you said this error occurred before the RejectParserCacheValue hook, right?
[01:22:38] <Krinkle>	 that's right
[01:22:52] <RoanKattouw>	 So what we would expect is we'd continue seeing these errors being logged, but only once for each page, and they'd dissipate over time
[01:23:04] <Krinkle>	 if it makes it there without issue, then yeah, it'd self-correct after it misses the second time
[01:23:28] <Krinkle>	 assumning the indirect key thing isn't surviving separately
[01:23:53] <Krinkle>	 RoanKattouw: shall we roll the second patch then, that should make it go away with no other side effects
[01:24:24] <RoanKattouw>	 Yeah let's
[01:24:31] <Krinkle>	 k
[01:24:40] * Krinkle is hungry and tired
[01:24:42] <RoanKattouw>	 The only side effect could be that we'd see too many parser cache rejections
[01:24:53] <RoanKattouw>	 No kidding, I am too and it's only 6:30pm here
[01:25:26] <Krinkle>	 that's about the same time I feel I suppose, fair I suppose.
[01:26:46] <Krinkle>	 syncing
[01:27:53] * twentyafterfour is too and it's 8:30 here :-... but I haven't been doing much other than monitoring the situation ;)
[01:28:23] <twentyafterfour>	 thanks for everyone who's jumped in to work on this
[01:28:26] <Krinkle>	 https://grafana.wikimedia.org/d/000000106/parser-cache?orgId=1&from=now-1h&to=now
[01:28:32] <Krinkle>	 rejections are slowing down it seems
[01:29:14] <RoanKattouw>	 They're a tiny fraction in any case
[01:29:31] <Krinkle>	 01:27 UTC end of sync
[01:29:58] <Krinkle>	 none in Logstash for 2min
[01:30:03] <RoanKattouw>	 If we were causing a catastrophe because we were effectively running without parser cache, I'd expect to see more rejections than that red sliver in the graph
[01:31:22] <Krinkle>	 https://usercontent.irccloud-cdn.com/file/A8v1UiDt/Screenshot%202020-10-01%20at%2002.31.08.png
[01:31:23] <Krinkle>	 indeed
[01:33:23] <Krinkle>	 RoanKattouw: can you keep an eye / take another look in an hour or so?
[01:33:43] * Krinkle updates task and prepares to sign off
[01:33:46] <RoanKattouw>	 Sure
[01:33:50] <RoanKattouw>	 I'll look again after dinner
[01:33:53] <Krinkle>	 thx for brain storming and helping out
[01:35:33] <Krinkle>	 o/
[04:04:12] <twentyafterfour>	 looks like the reject rate has dropped off some more since I last checked...
[09:12:12] <_joe_>	 I didn't read all the backlog, but an opcache corruption is both rare and usually not resolved by a new deployment
[13:34:57] <volans>	 FYI Python users: https://mastodon.social/@brainwane/104950044427849411
[13:39:30] <kormat>	 volans: hum. well, it's been badly needed for a Very long time. but it doesn't seem to be clear on whether the dependencies of _already installed packages_ will be honoured, which has been a related problem.
[13:40:50] <volans>	 kormat: partipate to the survey: https://docs.google.com/forms/d/e/1FAIpQLSeBkbhuIlSofXqCyhi3kGkLmtrpPOEBwr6iJA6SzHdxWKfqdA/viewform :D
[13:41:13] <kormat>	 ah hah!
[13:44:35] <klausman>	 It's almost as if package management is one of those things that continually keeps getting reinvented, badly.
[13:44:42] <kormat>	 klausman: quite.
[13:44:57] <volans>	 not a solved problem in 2020, that's for sure
[13:45:09] <klausman>	 I know! We're going to make a _new_ package management system that,... I'll see myself out.
[13:45:58] <volans>	 klausman: mandatory https://xkcd.com/927/ :D
[13:46:38] <volans>	 or https://xkcd.com/754/
[13:47:03] <volans>	 and, for last, for the specific topic at hand: https://xkcd.com/1987/
[13:47:06] * volans stops now
[13:52:50] <bblack>	 this is why you should recursively copy all your dependencies into the main repo of your project at a fixed known-good version and publish it all together! :)
[13:53:58] * kormat sobs
[13:56:24] <volans>	 kormat: have you seen our *-deploy repos?
[13:57:16] <kormat>	 volans: i have not/your question fills me with concern
[13:57:53] <klausman>	 One day, something as simple as left_pad will end up vendoring the bloody kernel.
[14:00:22] <rzl>	 well sure, you weren't going to run something like left_pad outside of a VM, were you?
[14:00:25] <rzl>	 that could be dangerous
[14:01:27] <ema>	 or maybe left_pad will be packaged and become a dependency of lots of projects... lots of builds will break when the maintainer removes the package from the archive, that would be funny
[14:16:39] <klausman>	 Bit unrealistic as a scenario
[14:18:34] <ema>	 silly, I know
[16:23:02] <volans>	 [FYI] chaomodus and I will migrate esams's DNS records to the Netbox-generated ones in ~10 minutes. Shout if we should hold for any reason!
[16:51:36] <volans>	 this is now deployed ^^^
[18:53:52] <sukhe>	 so if someone wants to join me for some Puppet fun, in https://gerrit.wikimedia.org/r/c/operations/puppet/+/631508, it seems like  0.0.0.0/0  is not considered a valid IP address per the definition of Stdlib::IP::Address (see https://puppet-compiler.wmflabs.org/compiler1001/25604/malmok.wikimedia.org/change.malmok.wikimedia.org.err)
[18:55:10] <rzl>	 sukhe: I clicked on your second link, and I looked directly into it, and now I have regrets
[18:55:13] <sukhe>	 I could have just used 0.0.0.0 and that would be fine except dnsdist doesn't like it without the netmask
[18:55:16] <sukhe>	 haha
[18:55:24] <cdanis>	 I've seen worse error messages
[18:55:24] <mutante>	 it's a bug in stdlib, i think
[18:55:27] <cdanis>	 used to be a C++ programmer...
[18:55:47] <mutante>	 0.0.0.0/0 is not accepted as valid IP because the regex says /[1-9] for the part after the /
[18:55:52] <mutante>	 but should be valid CIDR
[18:56:39] <mutante>	 so either we can separate IP and netmask into 2 different variables and then combine them.. or list 0.0.0.0/0 specifically as a Variant.. or create our own Wmflib::IP type .. or ..just use String
[18:57:45] <mutante>	 given that a lot of code still does not have any types.. even just String is not so bad
[18:57:51] <bblack>	 oh I missed this over here
[18:57:58] <shdubsh>	 fwiw: https://github.com/puppetlabs/puppetlabs-stdlib/commit/f37d1a46b4f15c95acf05e5d1225a2f44244dc00#diff-71d0eb816ed8445aee4a26948383b35e
[18:58:05] <sukhe>	 for now I did the Variant thing mutante suggested and that works as expected
[18:58:17] <sukhe>	 shdubsh: aaa!
[18:58:26] <bblack>	 FWIW the ::CIDR one does allow leading zero
[18:58:46] <bblack>	 hmmm
[18:59:32] <bblack>	 mutante: where's the regex that doesn't allow the leading zero?
[18:59:41] <mutante>	 wtf.. i just looked at that 30 min ago and pasted the [1-9] but it's not there
[19:00:18] <bblack>	 and the plain Stdlib::IP::Address should cover the cidr case too, it's all very confusing
[19:01:56] <bblack>	 I don't think the problem is the zeros, I think the problem is array vs single
[19:02:03] <sukhe>	 oh yeah in the current commit, yes
[19:02:08] <mutante>	 I ... I am confused indeed.. i have the 1-9 in my backlog but it's not actually there now
[19:02:17] <mutante>	 puppet things it gets a String here
[19:02:20] <bblack>	 the hieradata has a single value, the type expects an array of them
[19:02:38] <sukhe>	 yep, that was just me trying where it fails. I should revert it back to the the patchset 2
[19:02:45] <mutante>	 "but got String" is the important part.. but it's not quoted in Hiera
[19:03:07] <bblack>	 hmmm looking at PS2
[19:03:40] <mutante>	 I feel like somehow this wouldn't happen if this wasn't part of the config array.
[19:04:46] <mutante>	 I had suggested to try: acl => Variant[Array[Stdlib::IP::Address], '0.0.0.0/0']  and that works
[19:05:06] <mutante>	 so since that fixes it... it somehow does not match the 0.0.0.0/0
[19:05:07] <bblack>	 yeah but that's blending array vs single
[19:05:19] <bblack>	 the rest of the code expects an array, e.g. it does a join on acl
[19:05:29] <bblack>	 either way the hieradata should probably be an array
[19:05:55] <mutante>	 oh, does it have to be flipped around?   Array[Variant[Stdlib::IP::Address,  '0.0.0.0/0']]
[19:06:04] <bblack>	 yeah that would make more sense
[19:06:10] <sukhe>	 yeah for the join to work?
[19:06:16] <bblack>	 but the current ps6 hieradata also isn't an array
[19:06:32] <bblack>	 and really this should've just works, we have other such cases
[19:06:42] <bblack>	 s/works/worked/
[19:08:13] <bblack>	 sorry ps8
[19:08:28] <bblack>	 https://gerrit.wikimedia.org/r/c/operations/puppet/+/631508/8/hieradata/role/common/wikidough.yaml -> not an array
[19:09:06] <sukhe>	 yeah so PS2 is my ideal version of it. I guess I over engineered it. I should have probably said "0.0.0.0/0, ::/0", labelled it a String and called it a day
[19:09:32] <bblack>	 well, type-checking is a good thing
[19:09:42] <bblack>	 I'm still not sure what's actually wrong with PS2, do you have a compiler output from that one?
[19:09:58] <sukhe>	 yes, give me a sec. I closed the ten tabs I had open but I will get it
[19:10:49] <mutante>	 How about splitting it up into "Stdlib::IP::Address $ip = '0.0.0.0'" and  "Integer[0, 32] $netmask = '0'" and then just combine them in the config template
[19:11:02] <mutante>	 you would have proper types and should also just work
[19:11:22] <bblack>	 yeah but we've done this elsewhere, e.g. the base firewall role uses arrays of stdlib::ip::address with cirds
[19:11:25] <bblack>	 *cidrs
[19:11:36] <bblack>	 it should work, something's odd here
[19:11:57] <mutante>	 (I still agree that it _should_ work and it would be gone if this wasn't an array of $webserver config but just 'standlone')
[19:15:25] <sukhe>	 pushing ps2 again to clear things up and will share PCC output
[19:17:17] <bblack>	 https://puppet-compiler.wmflabs.org/compiler1003/25601/malmok.wikimedia.org/change.malmok.wikimedia.org.err
[19:17:22] <bblack>	 I think this is your old PS2 run
[19:20:14] <mutante>	 Stdlib::IP::Address::V4::CIDR = Pattern[/\A([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(\.([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])){3}\/([1-9]|[12][0-9]|3[0-2])?\z/],
[19:20:27] <mutante>	 there..isn't that a /([1-9] ...
[19:20:43] <mutante>	 this is from the compiler output.. as opposed to looking directly at the type file in the repo
[19:21:31] <bblack>	 oh
[19:21:34] <bblack>	 the mask
[19:21:37] <mutante>	 i thought i was going crazy
[19:21:42] <bblack>	 I was looking for the leading zero in the IP itself
[19:21:43] <mutante>	 when it was 0-9 in the repo
[19:21:48] <mutante>	 yea, the mask part
[19:22:07] <bblack>	 heh
[19:22:54] <bblack>	 the ipv6 is similar but more-complicated
[19:23:27] <bblack>	 messy!
[19:23:49] <mutante>	 but when i look at my local repo at modules/stdlib/types/ip/address/v4/cidr.pp then ... /([0-9]
[19:23:54] <mutante>	 why is that not the same ?
[19:24:45] <bblack>	 have you pulled recently?
[19:25:05] <bblack>	 hmmm it hasn't changed
[19:25:14] <mutante>	 yea, just made sure and did not change
[19:25:27] <bblack>	 it's there in my copy
[19:25:40] <bblack>	 bblack@haliax:~/repos/puppet$ cat modules/stdlib/types/ip/address/v4/cidr.pp
[19:25:43] <bblack>	 type Stdlib::IP::Address::V4::CIDR = Pattern[/\A([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(\.([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])){3}\/([1-9]|[12][0-9]|3[0-2])?\z/]
[19:26:05] <bblack>	 these are from upstream I guess
[19:26:33] <mutante>	 yea, it's there (now)
[19:26:41] <mutante>	 yea, we updated stdlib from upstream once
[19:27:05] <mutante>	 but that was in 2018
[19:27:29] <bblack>	 I really don't like how they constructed these regexen anyways
[19:27:47] <bblack>	 and then the tree of v6 types that get varianted together, with tons of duplication of over-ugly regexen
[19:27:50] <bblack>	 it's all ewwwww
[19:27:51] <mutante>	 so still weird how it would work in other places.. my next guess is the other places maybe use Stdlib::Ip_address instead of Stdlib::IP::Address
[19:28:06] <mutante>	 i just recently replaced some because John pointed out how the latter is better
[19:28:09] <bblack>	 base::firewalls uses IP::Address
[19:28:25] <bblack>	 but I think nobody else has happened to use a /0 with it
[19:28:45] <sukhe>	 ok back, sorry I was making tea
[19:28:47] <mutante>	 so I am going back to "it's a bug in upstream stdlib, /0 is valid"
[19:29:10] <bblack>	 yes
[19:29:11] <mutante>	 unfortunately you can just skip the mask in the config
[19:29:14] <mutante>	 sukhe already tried
[19:29:21] <mutante>	 can't
[19:29:30] <bblack>	 so, what others have done in a non-array context to work around this (possibly accidentally), is:
[19:29:37] <bblack>	 modules/profile/manifests/openstack/eqiad1/keystone/service.pp:    Stdlib::IP::Address::V4::CIDR $instance_ip_range = lookup('profile::openstack::eqiad1::keystone::instance_ip_range', {default_value => '0.0.0.0/0'}),
[19:29:46] <bblack>	 apparently the default doesn't have to validate heh
[19:29:51] <sukhe>	 ha
[19:29:58] <bblack>	 (which is also kinda dumb, on its own level)
[19:30:24] <mutante>	 (we should not have any default values per style-guide  /me hides) :)
[19:30:55] <mutante>	 I think splitting it into IP and netmask is not so bad
[19:31:13] <bblack>	 or doing what mutante suggested earlier and making a variant against it
[19:31:17] <mutante>	 to work around it .. and then we could make an upstream pull request
[19:31:18] <bblack>	 either way it's string data
[19:31:26] <sukhe>	 bblack: yes, so that was the commit. that is what led me down the path of all these changes I was trying to make
[19:31:30] <mutante>	 or maybe it's fixed in upstream and it's time to upgrade stdlib another time
[19:31:50] <sukhe>	 how do I find out which version of stdlib are we on?
[19:31:58] <sukhe>	 (just out of curiosity)
[19:32:11] <bblack>	 git log on the stdlib directory tells
[19:32:16] <mutante>	 the version that was latest on Nov 16 2018.. hmm
[19:32:27] <mutante>	 4.16.0
[19:32:33] <bblack>	 4.25.1
[19:32:42] <bblack>	 I think it's just ip address didn't change since 4.16
[19:32:46] <mutante>	 oh, heh, i just checked git log of cidr.pp
[19:32:59] <bblack>	 you updated it! :)
[19:33:02] <mutante>	 yep
[19:33:39] <mutante>	 https://github.com/puppetlabs/puppetlabs-stdlib/blob/main/types/ip/address/v4/cidr.pp
[19:33:47] <mutante>	 looks fixed :)
[19:34:12] <sukhe>	 "it's not a compiler^Wstdlib bug till it is"
[19:34:26] <sukhe>	 I honestly thought I was missing something. that's a natural feeling for me when I deal with puppet
[19:35:19] <bblack>	 who knows all the changes that happened from 4.25.1 -> 6.5.0 though
[19:35:31] <mutante>	 I'd just update cidr.pp but mixing versions is also a bad idea, probably. I'll make a change to update them all ..if even just so we can look at the diff.
[19:35:59] <bblack>	 apparently the /0 fix for CIDR was first released with 5.2.0
[19:36:06] <bblack>	 in case that version is more-palatable on other risks
[19:36:30] <mutante>	 or we just copy that file over from stdlib to wmflib, fix it and you use Wmflib::IP::Address ?
[19:36:52] <mutante>	 too many option snow
[19:36:55] <bblack>	 yeah we could do that, too
[19:37:08] * sukhe is silently listening
[19:37:08] <bblack>	 you'd need to copy over that whole tree though (ip/)
[19:38:15] <bblack>	 13 files to define IP address regexes :)
[19:38:57] <bblack>	 https://github.com/puppetlabs/puppetlabs-stdlib/commit/f37d1a46b4f15c95acf05e5d1225a2f44244dc00 has the fix
[19:39:19] <bblack>	 it was just a 1-char change, we could just drop that on our copy in stdlib:: and it will automerge fine if we upgrade the whole thing someday later
[19:39:21] <mutante>	 make it String, add a FIXME/TODO comment "replace type once stdlib has been upgraded" :)
[19:39:37] <bblack>	 I wonder if they ever fixed v6 though, which has to be fixed in several different files
[19:41:10] <bblack>	 yeah they fixed v6 in a separate commit, also pushed in 5.2.0
[19:41:12] <bblack>	 https://github.com/puppetlabs/puppetlabs-stdlib/commit/2f8601286b29fb1336dc8ea90e3a4303cb32b8b2
[19:41:25] <bblack>	 16 lines changed, but they're all 1-digit diffs
[19:42:21] <bblack>	 IMHO, the simplest thing would be to port those two commits onto our stdlib as a one-off desync from the 4.25.1 we're on today, and deal with it later on the next more-necessary upgrade (it will probably auto-resolve anyways on the merge conflict)
[19:52:29] <mutante>	 alright, https://gerrit.wikimedia.org/r/c/operations/puppet/+/631522
[19:53:40] <mutante>	 compiling it on * might be good.. will take about 1.5 h
[19:53:53] <bblack>	 well
[19:54:37] <bblack>	 I think it's comfortable to say that the change (a) can be visually verified as correct (faithful to its intent, I mean) + (b) only adds to the set of things that matches the affected types, does not remove from them
[19:54:45] <bblack>	 therefore, it cannot cause a new failure in existing code
[19:55:17] <bblack>	 up to you if you want the 1.5h safety run, but honestly I think it's fine as-is
[19:56:35] <mutante>	 ok, I'll merge it (and just glance at puppet failures in puppetboard a little while after that)
[19:58:53] <mutante>	 sukhe: new error, yay :)
[19:58:54] <mutante>	 https://puppet-compiler.wmflabs.org/compiler1002/25608/malmok.wikimedia.org/change.malmok.wikimedia.org.err
[19:58:56] <sukhe>	 sorry, I didn't check IRC. fwiw, PS10 is reverted to PS2
[19:59:14] <mutante>	 oh, i just clicked the rebase button
[19:59:30] <sukhe>	 yep, reverted now. building
[19:59:37] <sukhe>	 https://integration.wikimedia.org/ci/job/operations-puppet-catalog-compiler/25609/console
[20:00:08] <sukhe>	 ha same https://puppet-compiler.wmflabs.org/compiler1001/25609/malmok.wikimedia.org/change.malmok.wikimedia.org.err
[20:00:43] <mutante>	 yea, but a different issue now
[20:00:44] <mutante>	 expects Puppet::LookupValue, got Hash[Enum['acl', 'host', 'port'], Any, 3, 3]
[20:01:11] <sukhe>	 yes, this is what I was getting earlier as well https://puppet-compiler.wmflabs.org/compiler1003/25601/malmok.wikimedia.org/change.malmok.wikimedia.org.err
[20:01:39] <mutante>	 which PS was the one we started with ?
[20:02:10] <sukhe>	 PS10 is the current version and was PS2, which should have worked
[20:10:55] <bblack>	 is it possible that the compiler can't deal with the type being newly-defined in the same commit?
[20:12:02] <sukhe>	 wouldn't such a change fall under "Hosts that have no differences (or compile correctly only with the change)" though?
[20:12:30] <sukhe>	 so in PS11 (current), I tried another thing and that also didn't work
[20:12:35] <sukhe>	 we were having issues earlier with this:
[20:12:36] <sukhe>	     Dnsdist::Webserver_config $webserver_config = lookup('profile::wikidough::dnsdist::webserver', {'merge' => hash}),
[20:13:03] <sukhe>	 so I thought maybe the order of merge => hash matters (since api_key and password are private). that also didn't help
[20:13:59] <bblack>	 what I mean is, it's possible your PS is "correct" in practice, but the only way to pass the compiler is to first merge a separate change that just declares the new Dnsdist::Webserver_config type, and then run the rest of this change through a compiler that already knows that type
[20:14:05] <bblack>	 but that's just a blind guess, I could be offbase
[20:14:09] <sukhe>	 ahh that way
[20:15:12] <bblack>	 what's the hash merge for?
[20:15:16] <bblack>	 for the defaults?
[20:16:06] <sukhe>	 the hash merge is for the password and api_key defined in puppetmaster/private
[20:16:37] <bblack>	 ah
[20:17:10] <bblack>	 my things get tricky quickly!
[20:17:12] <cdanis>	 sorry sukhe I wasn't watching before, can you link me to your patch?
[20:17:25] <cdanis>	 on nvm it's https://gerrit.wikimedia.org/r/#/c/631508/
[20:17:25] <bblack>	 sukhe: you might need a fake version of those in the public-private repo?
[20:17:35] <sukhe>	 bblack: also there :)
[20:17:45] <bblack>	 ok :)
[20:17:59] <sukhe>	 cdanis: it's madness but if you want to join sure: https://gerrit.wikimedia.org/r/c/operations/puppet/+/631508/ https://puppet-compiler.wmflabs.org/compiler1002/25611/malmok.wikimedia.org/change.malmok.wikimedia.org.err :D
[20:23:22] <cdanis>	 sukhe: does it work if you relax acl's type to Array[String]?
[20:24:04] <mutante>	 expects Puppet::LookupValue, got Hash[Enum['acl', 'host', 'port'], Any, 3, 3].  The part that it is expecting Puppet::Lookupvalue should mean the lookup() itself doesn't like what it sees. when you google just the "wrong type, expects Puppet::LookupValue," there is stuff like https://tickets.puppetlabs.com/browse/PUP-8084
[20:24:09] <mutante>	 maybe this?  " there is a restriction on hashes that keys are limited to Variant[String, Numeric] because of the limitations in most serialization formats on what constitutes a valid hash key data type."
[20:24:31] <mutante>	 gotta go afk for a bit, bbl
[20:24:45] <cdanis>	 yeah, I think this is a fun, exciting, obscure way for Puppet to say "the looked-up value doesn't validate against the data type"
[20:26:20] <sukhe>	 cdanis: I haven't tried that, no. let me see.
[20:27:13] <bblack>	 yeah but the limitation on keys shouldn't apply to values
[20:27:30] <bblack>	 but it is entirely possible that we've exceeded some basic complexity limit for puppet type checking depth here :)
[20:28:32] <bblack>	 it could also be that the type appears to require all 5 hash keys, and the hieradata only provides 3 in the public copy
[20:28:37] <bblack>	 maybe validation hapens before merging?
[20:28:56] <sukhe>	 https://puppet-compiler.wmflabs.org/compiler1002/25612/malmok.wikimedia.org/index.html using Array[String] seems to have worked
[20:29:15] <bblack>	 or it could be that the "private" ones are missing.  IIRC, long ago it used to be the case that if you put new fake data in the private repo, there was some manual step to get the compiler updated to use the new commit
[20:29:20] <cdanis>	 yeah, so, the IP address value still fails to validate
[20:29:29] <cdanis>	 it's just you get a different error now
[20:29:33] <bblack>	 or that!
[20:29:49] <cdanis>	 bblack: you're speaking of https://wikitech.wikimedia.org/wiki/Nova_Resource:Puppet-diffs#How_to_update_the_compiler's_facts?_(e.g._INFO:_Unable_to_find_facts_for_host_conf2001.codfw.wmnet,_skipping)
[20:29:53] <bblack>	 we just updated the ip address types to accept the value in an earlier commit
[20:29:59] <cdanis>	 oh!
[20:30:04] <cdanis>	 is it merged?
[20:30:19] <bblack>	 it is, but maybe that also takes time for compiler?
[20:30:22] <bblack>	 or some other step?
[20:30:27] <cdanis>	 did sukhe rebase atop it?
[20:30:43] <bblack>	 https://gerrit.wikimedia.org/r/c/operations/puppet/+/631522/
[20:30:49] <bblack>	 it's merged, do we need to?
[20:30:54] <bblack>	 oh, maybe
[20:31:26] <cdanis>	 yeah, sukhe, try going back to using the ipaddress class but rebasing atop current production
[20:31:43] <sukhe>	 on it
[20:31:52] <cdanis>	 right now gerrit says the parent of your patch is https://gerrit.wikimedia.org/g/operations/puppet/+/ed5d327c5e39061660b8adce4d648012b2bc175e which happens to be the parent of the ipaddress fix patch
[20:32:14] <cdanis>	 so, yeah, i think PCC isn't seeing the fix to ipaddress
[20:33:16] <bblack>	 so all this runaround, basically boils down to (1) puppetlabs failed to contemplate /0 for CIDR types until a more-recent stdlib than what we had + (2) compiler does not rebase onto newer production merges for you :)
[20:33:43] <cdanis>	 yeah, and arguably it shoudn't
[20:33:57] <bblack>	 yeah I vaguely remember debating this a long time ago with j.oe
[20:34:13] <cdanis>	 it just does whatever is in your changeset, which I think is reasonable -- your changeset might be part of a stack of patches
[20:34:18] <bblack>	 it's definitely debateable, but I'm not sure I like the current state of affairs either
[20:34:41] <sukhe>	 hmm so I rebased on top production
[20:34:42] <sukhe>	 https://puppet-compiler.wmflabs.org/compiler1001/25613/malmok.wikimedia.org/change.malmok.wikimedia.org.err
[20:34:46] <sukhe>	 same
[20:35:00] <cdanis>	 hm
[20:35:40] <bblack>	 [on that side note, if it's a stack of unmerged patches, I agree that history should be what the compiler uses, but it might still rebase from the last common ancestor onto production's latest tip]
[20:35:57] <bblack>	 [since at the end of the day, that's what matters when it comes time to actually merge the new stuff into production]
[20:36:25] <cdanis>	 well, I think git-rebase (for those who use it) does that or at least can be configured to do that
[20:36:29] <cdanis>	 errrr
[20:36:29] <sukhe>	 (yeah I made sure HEAD^ is mutant.e's commit)
[20:36:30] <cdanis>	 git-review
[20:36:54] <cdanis>	 yeah very strange sukhe
[20:37:08] <bblack>	 so we're still pretty sure it's the acl ip::address that's not validating
[20:37:11] <cdanis>	 personally I would just relax to Array[String] and leave a TODO 🙃
[20:37:34] <sukhe>	 I think we are since Array[String] worked and that has alleviated the concerns I had with other things failing
[20:37:44] <bblack>	 I've seen hieradata IPs in both quoted and unquoted forms
[20:37:53] <bblack>	 I'm not sure when or if the quoting is ever necessary
[20:38:25] <bblack>	 unquoted does seem more-common though
[20:38:48] <cdanis>	 that's... hm
[20:38:58] <cdanis>	 now you have me wondering if YAML does any mangling of IP addresses.
[20:39:11] <bblack>	 I think the unquoted ones are still strings either way
[20:39:31] <sukhe>	 right, as per YAML anyway
[20:39:39] <bblack>	 hmmm
[20:39:40] <cdanis>	 I think so too, but I don't want to express certainty when I'm making any statements involving YAML parsing
[20:40:09] <bblack>	 Yet Another Mistake Language
[20:40:11] <sukhe>	 so in PS1,
[20:40:12] <sukhe>	 https://gerrit.wikimedia.org/r/c/operations/puppet/+/631508/1
[20:40:19] <sukhe>	 I had quoted it and that had also failed
[20:40:40] <bblack>	 right, but back then just the /0 would've caused failure, too
[20:40:42] <sukhe>	 but it's been a while and I should try again (!) especially since https://integration.wikimedia.org/ci/user/ssingh/builds 404s for me
[20:40:45] <sukhe>	 oh right yes
[20:40:46] <sukhe>	 trying
[20:41:44] <bblack>	 we should just write a new data format for our hieradata, it will fix all the things
[20:42:07] <bblack>	 we'll call it 927ML
[20:42:24] <sukhe>	 haha wow, ok
[20:42:28] <sukhe>	 it worked https://puppet-compiler.wmflabs.org/compiler1003/25614/malmok.wikimedia.org/index.html
[20:42:31] <sukhe>	 the quoting worked
[20:42:33] <bblack>	 lol
[20:42:34] <sukhe>	 !
[20:42:35] <cdanis>	 lol
[20:42:38] <cdanis>	 lol lol lol
[20:43:10] <sukhe>	 sigh
[20:43:12] <cdanis>	 if you want to humor me
[20:43:20] <cdanis>	 figure out which one needs to be quoted?
[20:43:28] <bblack>	 the one with colons of course
[20:43:30] <sukhe>	 ha, I have an odd feeling it is ::
[20:43:31] <bblack>	 because key:value
[20:43:32] <cdanis>	 I'm actually suspicious of ::/0 because :: has magic meaning in puppet
[20:43:34] <sukhe>	 yep
[20:43:35] <cdanis>	 OH
[20:43:37] <cdanis>	 ahahah
[20:44:29] <bblack>	 I'm telling you, 927ML will fix all this
[20:45:07] <bblack>	 we'll move all the boundary quote characters out-of-band using the high bits of our special 9-bit bytes
[20:47:25] <bblack>	 then we can port it all back to Multics with 36-bit words on and be done with the "Unix" heresy
[20:48:15] <sukhe>	 while bblack shares his 927ML wishlist, I can report that it was indeed ::/0
[20:49:27] <cdanis>	 hilarious
[20:50:08] <sukhe>	 thanks everyone for the help and indulging in this! <3
[20:50:21] <sukhe>	 yet another puppet wildride
[20:50:39] <sukhe>	 took jbond.42 on one the other day: https://gerrit.wikimedia.org/r/c/operations/puppet/+/607477
[20:50:54] <sukhe>	 (stay away from it)
[20:52:49] <cdanis>	 the Python3 YAML parser doesn't do this, but
[20:52:57] <cdanis>	 >>> str = io.StringIO(' - ::/0')
[20:52:59] <cdanis>	 >>> yaml.load(str)
[20:53:01] <cdanis>	 ['::/0']
[20:53:04] <cdanis>	 who knows, really
[21:02:45] <shdubsh>	 If I'm reading it right, maybe yaml linter with quoted-strings enforcement enabled might have helped?
[21:03:03] <bblack>	 we have tons of unquoted strings in hieradata
[21:03:21] <cdanis>	 it would be a worthy cleanup project
[21:03:55] <bblack>	 the magic of parsing unquoted things is scary
[21:04:11] <bblack>	 clearly it automagically does integers, floats, booleans, etc
[21:04:41] <bblack>	 we do have some unquoted ipv6 with colons that work, if they're inside the numbers
[21:05:33] <bblack>	 it's either the leading colon at the yaml level that throws it off (for this parser), or it really is the :: carrying some magic at the puppet level and ::/0 becomes something like a class lookup on a class named /0
[21:05:53] <shdubsh>	 There's a task about adding yamllint to CI: https://phabricator.wikimedia.org/T236954
[21:06:07] <sukhe>	 at least in this case, the failure was both due /0 not being recognized in PS1 and the missing quotes in subsequent patches
[21:06:30] <bblack>	 whether they should be required in this case is a matter of some debate though
[21:06:39] <bblack>	 the bulk of the ip address data in our hieradata/ is unquoted
[21:06:53] <bblack>	 there's only a few counter-examples
[21:07:20] <bblack>	 but it would perhaps make some things clearer if they were all quoted
[21:07:24] <sukhe>	 right, I added them in PS1 because I forgot but then it failed, so I thought maybe Puppet doesn't want this to be a string since it's an IP address (not that made any sense since YAML doesn't care if it's quoted, but well, Puppet)
[21:07:43] <bblack>	 (make it clearer that the address is a yaml string, and that the stdlib type is just applying some regex rules to a string)
[21:08:15] <bblack>	 which is very different than, say, the possibility that it could be some ruby ip address type/class
[21:09:12] <bblack>	 (which actually is one of the many things that could be going on here under the hood - it could be the ruby yaml parser does recognize unquoted IPs, and then they get auto-stringified for the puppet-level regex check, etc)
[22:10:28] <mutante>	 I tried to make a VM with the cookbook, which now means it also includes regenerating DNS zones.. unfortunately after it was done generating them and showed me the diff... i saw changes that were not by me
[22:10:44] <mutante>	 so i typed "no" instead of "done" 3 times and aborted it