[10:12:55] lunch [13:06:20] greetings [13:10:15] o/ [14:25:42] How would one access this fact in puppet? https://puppetboard.wikimedia.org/fact/java/%7B%27version%27:%20%7B%27full%27:%20%271.8.0_322%27%2C%20%27major%27:%208%7D%7D I'm trying $facts['java']['version']['major'] and it doesn't seem to wrk [14:27:17] trying to set it here: https://github.com/wikimedia/puppet/blob/production/modules/elasticsearch/manifests/instance.pp#L121 [14:27:34] thinking this is because we haven't installed the java package yet? [14:32:12] having a little plumbing emergency to take care of. Will try to make it to planning on time, but there's a chance i'll still be dealing with it [14:32:22] Good luck! [14:37:23] ^^ don't worry too much about that java version stuff, I think we can use OS version instead [14:50:59] inflatador: $facts['java']['version']['major'] is the correct syntax and it works AFAICT in a quick puppet apply test [14:53:17] volans hmm. let me try it one more time then, I was getting undefined errors last wk. If not, nbd, it is compiling fine with os major version [14:53:50] $ sudo puppet apply -e "notice(\$facts['java']['version']['major'])" [14:53:50] Notice: Scope(Class[main]): 8 [14:54:53] are you running that from master or client? [14:54:53] even better with inverted quotes :D 'notice($facts["java"]["version"]["major"])' [14:55:04] I run that on an-airflow1002 for example [14:55:13] that is one of the hosts from your link [14:57:46] thanks, seeing the same thing on cloudelastic host [14:57:56] running it thru CI again, let's see if it likes it https://gerrit.wikimedia.org/r/c/operations/puppet/+/787505 [15:00:24] CI still thinks it's undef: https://integration.wikimedia.org/ci/job/operations-puppet-tests-buster-docker/43594/console , but don't worry too much, think I'll stick to os major version ATM [15:02:24] cbogen_, mpham, dcausse, ejoseph: triage meeting: https://meet.google.com/eki-rafx-cxi [16:01:52] ^^ That's for sure a CI issue, the facts are coming from the CI container, which doesn't have java installed. So we could use a different CI image, or just ignore that java version is undef [16:02:29] * ebernhardson never fully understood how facts and pcc works, some dark magic :P [16:04:44] me neither, but v-olans' cmd above is helpful to have some contrast between CI and prod [17:04:05] Haven't fully confirmed this yet, but I don't see a way to override the docker image used for CI (and that's probably a bad idea anyway). So falling back to OS major version instead of java version [17:04:40] inflatador: oh, those specs. Those are super annoying [17:05:06] inflatador: Inside spec's there are basically no supports in place. If you want to refer to a fact you have to define the fact in the spec. Lemme find an example [17:07:29] inflatador: essentially at the top of elasticsearch_spec.rb we have `let(:facts) { facts }`, those facts came from WMFConfig.test_on which only sets `operatingsystem` and `operatingsystemrelease` facts. [17:09:32] probably needs something like `let(:facts) { facts.merge({'java' => {'version' => {'major' => 11}}}) }` [17:11:52] yeah, we could approach it that way too, I'll take a look [17:12:14] probably better to get the version directly instead of indirectly [17:20:40] lunch/errands, back in ~70m [18:34:17] inflatador: fyi (since I'm talking to ryankemper), sofar we've been planning our capex stuff in https://docs.google.com/spreadsheets/d/1xwdhKSjp0h8WItg_hobmjV6L4l83aiMeMR5S4_P_bE4/edit#gid=132951534 [18:48:23] hmm, .tasks and .ltrstore are pre-elastic 6 indices on some clusters. Will have to see how to transition [19:03:02] back [19:03:07] thanks gehel [20:23:09] if anyone has time to look over this PR, it would help us move towards bullseye https://gerrit.wikimedia.org/r/c/operations/puppet/+/787505 . We might need to fix the spec file, LMK if you think that is necessary [20:27:35] (working on changes e-bernhardson mentioned above) [20:43:48] inflatador: impl seems reasonable enough. For the spec file I'm not sure whats would be appropriate, the test with those lines commented out doesn't test that the gc_log flag turns on some sort of gc logging any more. But changing the test isn't obvious since there isn't any shared content between os versions to look for [20:44:37] ebernhardson: I was looking at that test file as well (https://gerrit.wikimedia.org/r/c/operations/puppet/+/787505/31/modules/elasticsearch/spec/classes/elasticsearch_spec.rb), my guess is it was disabled when we were initially making the stub commit where we switched the gc_logs_flag branching logic backwards just to see what PCC would say [20:44:56] so yeah we prob want to uncomment those back, I will try doing so and see if the tests still pass [20:45:20] the tests will fail, because those flags aren't valid in java 11 [20:46:05] they're set here https://gerrit.wikimedia.org/r/c/operations/puppet/+/787505/30/modules/elasticsearch/manifests/instance.pp line 174 ish [20:46:33] Wouldn't they have been failing previously then? I guess I don't understand what about our change would impact that test [20:47:18] yeah, they were failing previously, but you can re-enable those lines, it's somewhere up in the 20 billion patches I submitted, easy to lose ;) [20:47:42] ryankemper: it's inside the `on_supported_os(WMFConfig.test_on)` section, so i think that assertion will be run for multiple debian versions (9,10,etc). So we can't have one assertion that covers them all [20:47:57] i suppose the hacky answer is vary on facts['operatingsystemmajrelease'] in the test as well [20:48:16] Yeah, that's what I was thinking of too, but now I gotta figure out how to write if statements in ruby ;) [20:48:52] I love me a good hack but I'm not in favor of that one, that means we'd be making the test useless [20:48:57] since we're including the logic we want to test in the test itself [20:49:33] ryankemper: not really, the test is testing 'does gc_log=true add more jvm options that turn it on'. The problem is that the jvm options to check vary in debian 9 vs 11, so we have to check which we are on to know which to check for [20:49:35] Well maybe not, I guess it's more analogous to us just providing a known-good [20:49:46] ebernhardson: yeah on second thought I agree [20:50:53] inflatador: with any luck, same syntax as the `if $gc_log == true {` lines in instance.pp [20:51:03] oh, doh thats puppet. this is ruby [20:51:08] I've just been trolling thru other's spec files looking for an example if statement [20:51:17] i dunno, run `irb` to get a ruby shell and see what works :) [20:51:51] ./vendor_modules/stdlib/spec/spec_helper.rb might be a good one [20:52:02] the only slightly weird thing to remember about ruby is that ifs (actually all blocks) need to be ended with `end` [20:52:38] * inflatador finds it hard to believe that this is the **only** weird thing to remember about ruby =) [20:53:12] :P you have no idea how right you are [20:53:27] * ryankemper has still not forgiven ruby for calling a dictionary (aka hashmap) a "hash" [20:53:43] a hash is a checksum, dammit! [20:55:47] Don't ya love it when they overload already-established tech terms? [20:55:59] * ebernhardson glares at opensearch [21:00:52] ryankemper up at meet.google.com/rds-wmdf-uut if/when you're ready to pair [21:03:19] inflatador: cool, 5 mins for me [21:03:33] ACK [21:06:30] Also, I see your patch passed CI, I'm stumped on that one [21:08:04] I guess the earlier failures weren't related [22:22:18] * ebernhardson wonders what a more-obvious way would be to run a cirrus script for each underlying cluster (eqiad-psi, eqiad-omega, etc) than a hand-picked set of wikis [22:31:35] * ebernhardson realizes after pressing enter why searching email for 'phabricator elasticsearch' isn't going to be very discriminative