[07:25:42] A couple talks look interesting https://inog.net/live/ [07:43:54] (pad is already up, thanks Faidon!) [07:45:20] XioNoX: indeed! nice [10:01:41] is there a recommended way to implement sth like "feature flags" for profiles? case in point I'd like to have a flag for "turn on upload of data to thanos" for prometheus profiles, when on such flag would need to lookup() a bunch of values to pass to various modules [10:02:23] using if (flag) $var = lookup() in the profile itself is technically against the guidelines [10:02:54] I could pass each and every variable to the profile and lookup accordingly, plus the flag I guess [10:04:28] godog: i would create a profile for "turn on upload of data to thanos" and then use the feature flag in the prometheus profile to determin if you want to include the new profile [10:06:25] in this case when the feature is on some parameters to modules change, e.g. passing the username/password to use to the sidecar, as opposed to the case where upload is off [10:06:36] how would I implement sth like that in this case ? [10:09:01] godog: do you just have additional parameters or some would change if the flag is on/off? [10:09:50] godog: would something like this work https://phabricator.wikimedia.org/P11302 [10:09:54] yeah the parameters would change, e.g. in the account case it'd go from username == undef to being defined [10:11:28] jbond42: s/include/require or direct class call/ [10:11:56] jbond42: I think it would yeah, how the feature class would look like ? two parameters $sidecar_password and $sidecar_username that get lookup()'ed ? would they be in scope ? [10:14:35] volans: why not include? godog difficult to think about without seeing the code, if you mock something happy to take a look [10:14:54] jbond42: style guide docet :D [10:15:17] jbond42: fair enough! I'll give it a spin and let you know the review [10:15:47] volans: you can include other profiles in a profile just not base modules [10:15:57] godog: sounds good [10:16:11] jbond42: cit.: 3. No resource should be added to a profile using the include class method, but with explicit class instantiations. Only very specific exceptions are allowed, like global classes like the network::constants class. [10:16:16] https://wikitech.wikimedia.org/wiki/Puppet_coding#Profiles [10:16:58] ofc the guideline can be re-evaluated ;) [10:16:59] oh ok, i dont think wmf_styleguide catches that one [10:17:40] +1 to reevaluating the guideline every now and then [10:18:27] godog: so, if I get it correctly, you have a user/pass that will be shared between the client (prometheus to send data) and the server [10:19:17] that can be defined in both places in hiera, one for real (the server I'd say) and the other with an alias [10:19:27] it should be cleaner than using a global keys [10:20:07] beside that the client part does it need to lookup other parameters that justify another profile or that could just be a module class? [10:29:36] mmhh so when upload needs enabling at least two things need to happen: thanos sidecar needs to know how to access object storage, and prometheus needs to have the local compaction disabled [10:29:58] https://gerrit.wikimedia.org/r/c/operations/puppet/+/598712 is the first try, haven't run pcc yet [10:31:34] and $objstore_account where is defined? [10:31:55] sorry [10:32:22] found it at the end [10:32:34] dunno, I would probably just add them in the main profile at this point [10:34:05] yeah that's fair, probably for two variables it isn't a whole lot of difference [10:34:31] so the two variables plus the feature flag (?) [10:35:13] I guess, yes [10:36:41] mmhh ok trying that in the next PS, although I'm curious how we'd do it if there were more variables involved [10:36:55] i guess the question is would you need objstore_account in any other profile (other then profile::prometheus::k8s::staging). if so its better to have them defined in there own profile so other profiles can require profile::thanos::upload and then use the variables with $profile::thanos::upload::objstore_account [10:38:57] yeah there's several profiles, one of each prometheus instance and they will be sharing the account/password [10:39:25] mmhh ok, I'll try that approach too [10:39:51] it feels like we're back to "params class" which for some reason were verboten [10:52:33] godog: i would be tempted to pass the enable_thanos_upload flag to profile::thanos::sidecar [10:54:28] yeah that's fair, and then require the profile within that class [10:54:34] ok I'll change it [10:55:01] also template() gets evaluated for content in file resources regardless of ensure => absent doesn't it, sigh [10:55:18] yes i think so [10:59:31] godog: something like this is what i was thinking https://gerrit.wikimedia.org/r/c/operations/puppet/+/598719 [11:01:41] jbond42: aye, thanks! trying that approach now [11:13:33] ok PS4 of https://gerrit.wikimedia.org/r/c/operations/puppet/+/598712 seems to work as expected, there's still the question in my mind of whether to have the extra profile class or not [11:13:38] gotta go to lunch [11:24:14] i thought ptrofile::thanos::upload was going to do more then just profice the paramters, as its empty i agree better to role those params to profile::thanos::sidcar, i have commented on the CR [12:33:46] kk, will update the review with the suggestions, thanks jbond42 volans ! appreciate it [12:39:07] anytime :) [12:40:40] np :) [14:07:44] ghghghgh the lookup of profile::thanos::objstore_account from within profile::thanos::sidecar trips wmf-style [14:07:47] 14:51:29 modules/profile/manifests/thanos/sidecar.pp:15 wmf-style: Found hiera call in defined type 'profile::thanos::sidecar' for 'profile::thanos::objstore_account' [14:07:51] 14:51:29 modules/profile/manifests/thanos/sidecar.pp:16 wmf-style: Found hiera call in defined type 'profile::thanos::sidecar' for 'profile::thanos::objstore_password' [14:09:52] ok I don't think I want to duplicate/alias those keys to another location [15:02:17] godog: can you just have objstore_account and objstore_passwords as paramters to profile::thanos::sidecar [15:14:42] jbond42: yeah that's what I did in https://gerrit.wikimedia.org/r/c/operations/puppet/+/597071/12/modules/profile/manifests/thanos/sidecar.pp [15:15:03] maybe a bug ? not sure [15:15:28] one sec ill take a quick look [15:15:35] sure, thanks! [15:33:31] I think it has to do with class vs define, e.g. this is fine: https://gerrit.wikimedia.org/r/c/operations/puppet/+/597072/13/modules/profile/manifests/thanos/compact.pp [15:37:06] godog: i this is becaus profile::thanos::sidecar is a defined type not a class, sorry i didn;t notice that before. In this case i would probably do something like this https://phabricator.wikimedia.org/P11304 [15:37:17] sorry got destracted half way through typing that [15:39:11] thats obvioulsy a lot of refactoring so in this case i would be tempted to just move the $objstore_account = lookup('profile::thanos::objstore_account') (and the password) so its inside the main block and lint::ignore. maybe create a task and TODO to refacter simlar to above [15:41:27] jbond42: thanks a lot! yeah I'll opt for the simpler option for now [15:41:56] will send out a new PS shortly with that change, then it is likely enough puppet for me for the day! [15:42:57] :)