| *** mhen_ is now known as mhen | 01:33 | |
| opendevreview | Merged openstack/devstack master: lib/neutron: Deploy under uWSGI by default https://review.opendev.org/c/openstack/devstack/+/932199 | 05:07 |
|---|---|---|
| opendevreview | Maxim Sava proposed openstack/tempest master: Add test for parallel image upload https://review.opendev.org/c/openstack/tempest/+/945320 | 07:33 |
| opendevreview | Maxim Sava proposed openstack/tempest master: Add SRBAC alt manager persona to dynamic credentials https://review.opendev.org/c/openstack/tempest/+/822318 | 09:44 |
| opendevreview | Maxim Sava proposed openstack/tempest master: Add test for parallel image upload https://review.opendev.org/c/openstack/tempest/+/945320 | 13:46 |
| opendevreview | Merged openstack/devstack-plugin-nfs master: Update CI jobs for Flamingo https://review.opendev.org/c/openstack/devstack-plugin-nfs/+/950317 | 14:20 |
| opendevreview | Takashi Kajinami proposed openstack/grenade master: Remove transition code for old python-openstackclient release https://review.opendev.org/c/openstack/grenade/+/936107 | 14:20 |
| opendevreview | Luigi Toscano proposed openstack/devstack-plugin-nfs master: zuul: run cinder-tempest-plugin tests too https://review.opendev.org/c/openstack/devstack-plugin-nfs/+/898965 | 14:54 |
| *** ralonsoh is now known as ralonsoh_out | 15:08 | |
| opendevreview | Merged openstack/devstack master: lib/neutron: Remove NEUTRON_DEPLOY_MOD_WSGI https://review.opendev.org/c/openstack/devstack/+/932203 | 17:03 |
| opendevreview | Eric Harney proposed openstack/devstack master: Silence SyntaxWarnings in outfilter.py https://review.opendev.org/c/openstack/devstack/+/950449 | 17:19 |
| opendevreview | Fernando Ferraz proposed openstack/devstack-plugin-nfs master: zuul: run cinder-tempest-plugin tests too https://review.opendev.org/c/openstack/devstack-plugin-nfs/+/898965 | 17:25 |
| opendevreview | Fernando Ferraz proposed openstack/devstack-plugin-nfs master: [DNM] Run Glance with Cinder as backend https://review.opendev.org/c/openstack/devstack-plugin-nfs/+/940482 | 17:25 |
| opendevreview | Maxim Sava proposed openstack/tempest master: Add test for parallel image upload https://review.opendev.org/c/openstack/tempest/+/945320 | 18:12 |
| clarkb | gibi: sean-k-mooney I left a question on https://review.opendev.org/c/openstack/devstack/+/948436 | 21:07 |
| sean-k-mooney | clarkb: the motivation to do it this way is so that we could be very explcit in speicific jobs which service have this set. and specificly we wantted to do this onece and have it work for all plugins or existign service without code change | 21:39 |
| clarkb | right, the problem with that is 6 months from now making it a default is painful | 21:40 |
| sean-k-mooney | well this is for the eventlet transition | 21:40 |
| clarkb | because now you have to merge bash arrays and prefer one side or another. If you have a normal var like PLACEMENT_THREADS then you just change its default from false to true | 21:41 |
| sean-k-mooney | so when we make ti the default we will change the defualt in nova | 21:41 |
| sean-k-mooney | what is the other method you were suiggesting | 21:41 |
| sean-k-mooney | by the way to me this is identical to how the git repos and branches work | 21:42 |
| clarkb | sean-k-mooney: typically it is easier to manage defaults and make them human readable/manageable if you have a var like PLACEMENT_RUNTIME={$PLACEMENT_RUNTIME:-eventlet} that you override and then just pass that to run_process' env var list | 21:42 |
| sean-k-mooney | https://github.com/openstack/devstack/blob/master/stackrc#L322-L323 | 21:42 |
| clarkb | the difference is the array | 21:43 |
| clarkb | the array can contain arbitrary env vars so merging them / changing defaults later is much more painful | 21:43 |
| sean-k-mooney | right but having ti be unrestircted was kind of the point | 21:43 |
| sean-k-mooney | so tha twe dont have to keep going back for each service and adding a spcific value | 21:44 |
| clarkb | right you're basically punting the maintenance bruden into the future | 21:44 |
| clarkb | and when you get to the point where you have to manage that outside of specific jobs it will be painful | 21:44 |
| sean-k-mooney | i dont see this as a mantance burden honestly | 21:44 |
| clarkb | if we're more explicit about the settings now that means we can manage them directly and set defaulst that make sense over time | 21:45 |
| clarkb | with this proposal you're going to have to curate this at a job level and if you ever need to manage it within devstack proper it will be annoying to merge the values | 21:45 |
| sean-k-mooney | i dont see why we cant with thsi apprch | 21:45 |
| sean-k-mooney | clarkb: actully my orginal propsal was to directly render systemd drop in files to set it | 21:46 |
| clarkb | because array[service] is pointing to a string of vars. If I want to set an additional var, or modify one of those vars in the string, etc I have to jump through more hoops | 21:46 |
| sean-k-mooney | gibi tought following the patten we have for the git repos would be cleaner | 21:46 |
| clarkb | the difference with git repos is the values are singletons | 21:46 |
| clarkb | there is one repo location and one repo branch | 21:46 |
| sean-k-mooney | so your object is to thte fact you can have multiple vars | 21:46 |
| clarkb | env vars are a list. Its still doable | 21:46 |
| sean-k-mooney | the problem is the name of the var will be diffent for each project | 21:47 |
| clarkb | but much more painful to manage if you're merging values or overriding things for defaults etc | 21:47 |
| clarkb | sean-k-mooney: that seems like a bug | 21:47 |
| clarkb | sean-k-mooney: it could just be OSLO_RUNTIME and be consistent everywhere? | 21:47 |
| sean-k-mooney | that intentional | 21:47 |
| clarkb | then you set it to thread or eventlet or async or whatever but then operators don't need to know the different var names | 21:47 |
| sean-k-mooney | we could revisit it but we didnt want to confirue this form the oslo side | 21:48 |
| clarkb | you don't have to configure it from the oslo side | 21:48 |
| clarkb | but the input is configuring oslo | 21:48 |
| clarkb | (or at least that is what the blog post I read implied) | 21:48 |
| sean-k-mooney | clarkb: i mean have the env var be somethign that oslo provides | 21:48 |
| sean-k-mooney | i dont recall what the explcit reason was to not do that | 21:49 |
| clarkb | right I don't think you have to do that. Just give it a consistent name that refers to what it does (whcih is configure oslo service's runtime between different operational modes) | 21:49 |
| sean-k-mooney | althogh it may have been so we can make sure you cant opt into an unsupproted mode | 21:49 |
| clarkb | anyway I'm not going to say no to any of this. I just want to call out this adds a second mode of setting unit file env vars and one that is far more tricky to manage down the road when you want to set multiple vars and change defaults and manage them directly rather than set once in a job definition | 21:49 |
| sean-k-mooney | so i dont really expect devstack or the tempets team to manage these | 21:50 |
| sean-k-mooney | but perhasp there is a reason for them to | 21:50 |
| sean-k-mooney | i was not expecting neutron for exampel to want to test with novas schduler runnign without event let | 21:51 |
| clarkb | no but in 6 months that will probably be the case | 21:51 |
| clarkb | (or the inverse) | 21:52 |
| sean-k-mooney | maybe. my orginal propsoal was each nova service woould only supprot one mode in any release | 21:52 |
| clarkb | thinking about it more maybe a way to mitigate my concern is to flip the order of the vars | 21:52 |
| sean-k-mooney | dan ask to supprot both so we had to come up with a way to swap without using config files | 21:52 |
| clarkb | so that the internal state overrides the external state if that makes sense | 21:52 |
| clarkb | that way if devstack grwos the ability to configure things directly it will override the one off state in the top level job def and you'll have to set the vars the devstack way | 21:53 |
| clarkb | then you don't even need to worry about merging the array in properly | 21:53 |
| sean-k-mooney | you mean when we are generating the file | 21:53 |
| clarkb | https://review.opendev.org/c/openstack/devstack/+/948436/2/functions-common yes on line 1649 | 21:53 |
| clarkb | flip the order | 21:53 |
| sean-k-mooney | so ya we could do that i guess | 21:54 |
| clarkb | basically as long as devstack has no need to worry about an env var doing it this way is unaffected. As soon as devstack does have a need to manage things directly you must use the devstack method | 21:54 |
| clarkb | but that avoid needing to do string parsing into var lists and so on | 21:54 |
| sean-k-mooney | though i htink the current order make more sense since we want to overried the defualt state | 21:54 |
| clarkb | but there is no default state according to the above | 21:54 |
| clarkb | not in devstack anyway. And thats my point if devstack did become away of any of these vars you should manage it with devstack | 21:55 |
| clarkb | but if it isn't aware then using this array won't hurt anything | 21:55 |
| sean-k-mooney | clarkb: honestly i have jsut beeing think in my head woudl it be better to move the nova install into novas plugin so that devsnck never has to manage it | 21:56 |
| clarkb | that doesn't chagne the problem I'm trying to describe | 21:56 |
| clarkb | whether its devstack or the nova devstack plugin you still have this configuration layer UX that gets processed through sane defaults and emits a localrc configuration. That configuration then feeds into the configuration of services | 21:57 |
| sean-k-mooney | clarkb: we didnt really want to allow operator to choose how the service run to the idea that devstack or other proejct was not a consideratoin for this approch at leat not by me | 21:57 |
| clarkb | sean-k-mooney: right but lets ignore the specific eventlet vs threads problem for now | 21:57 |
| clarkb | sean-k-mooney: this chagne adds a new feature to devstack that allows you to do a complete end around that configuration layer UX | 21:58 |
| clarkb | that is the problem | 21:58 |
| clarkb | and I think its ok to do as long as the devstack internals win if they care about specific options | 21:58 |
| sean-k-mooney | ya im conflicted. | 21:59 |
| sean-k-mooney | i see where your coming form | 21:59 |
| sean-k-mooney | in this specific sate we are modifying nova internal state that we didtn want to expose to anyoen, we have to for testing but thats it. but in the generic case i see why you would want to keep the defaulting in devstacck | 22:00 |
| clarkb | the vast majory of my interaction with devstack these days is "my job exploded I don't understand why" and the vast majority of times the answer is "because you cowboyed some weird configuration like forcing ipv6 addresses when there is no ipv6" | 22:00 |
| sean-k-mooney | clarkb: woudl you prefer if we added a varible per servcie and by service i mean per binary so nova would need 1 for the schduler another for the api | 22:00 |
| clarkb | features like this make those problems worse not better | 22:00 |
| clarkb | sean-k-mooney: that was my initial thought (note the env var that goes into the unit file may be the same name for each service then you control its value per service). But I think I'ev managed to convince myself that this new feature of env var array stuff is ok as long as the internal env values override the matrix array values | 22:02 |
| clarkb | since that will force ouy to use the internal env var values if they exist | 22:02 |
| sean-k-mooney | i guess that works if your happy with it | 22:03 |
| sean-k-mooney | i know enough of the internal of devstack ot be able to bypass and of the protectin sif i need to so i can cobowy in somehting if i need to test someitn in a DNM later :) | 22:03 |
| clarkb | theoretically devstack exists as a shim between human intention and service configuration | 22:04 |
| sean-k-mooney | clarkb: my two intila tough were pre-run playbook to drop in a systemd env file or use the fact we can write any file we like with the local.conf :) | 22:04 |
| clarkb | we're removing that shim here. I think that is ok as long as the shim can take over when it needs to | 22:04 |
| sean-k-mooney | it does but i tought we wanted to move aware from acros | 22:05 |
| clarkb | I'm not sure I understand that statement | 22:05 |
| sean-k-mooney | we can totally follwo the patther or the *_MOD_WSGI* vars we are removing if that is preferable too | 22:05 |
| clarkb | (and really most of this is selfish I don't want to have to debug something really crazy because someone discovers this feature to start setting random env vars on random services) | 22:06 |
| clarkb | basically the more devstack hand holds these things the less likely people are to randomly footgun | 22:06 |
| sean-k-mooney | ya thats fiar. i kind of saw that as a feature not a bug because it ment i did not need ot bother you when we hit a road block | 22:06 |
| clarkb | and I understand that in this particular case hand holding all the various services taht need stuff set to toggle eventlet on and off is annoying so maybe we allow that feature in just with some borders | 22:07 |
| sean-k-mooney | clarkb: i think i have jsut used devstack too long to be able to see it as handloding | 22:07 |
| sean-k-mooney | to me its not a black box it somehting if have read most of the code of at this point several times over | 22:08 |
| clarkb | ya but thats not true for most people using it | 22:08 |
| sean-k-mooney | right. | 22:08 |
| sean-k-mooney | so to summerise for gibi | 22:08 |
| sean-k-mooney | if we invert the precidence and maybe add a comment to say this is not intened as a generic feature | 22:09 |
| sean-k-mooney | then that would be more suprotabel and allow devstack to manage the defaults over time | 22:09 |
| clarkb | ya I'm not even sure we need the comment warning since there is zero documentation of the feature in the current patchset :) but I don't think the comment hurts either | 22:09 |
| sean-k-mooney | would it help or hurt ot hide it behiand a function like enable_plugin ? | 22:10 |
| sean-k-mooney | https://github.com/openstack/devstack/blob/master/functions-common#L1737-L1748 | 22:10 |
| sean-k-mooney | i.e. set_service_var n-cpu whatever 42 | 22:11 |
| clarkb | the existing env var manipulation is currently hiding behind run_process I think | 22:11 |
| clarkb | but its not that specific either its just collecting the env list and passing it along | 22:12 |
| sean-k-mooney | right but that not mutable via local.conf | 22:12 |
| sean-k-mooney | that was part of the goal have it trivally setable via local.conf since that makes it simple to do locally or in ci | 22:12 |
| sean-k-mooney | clarkb: i have a related quesiont regarding enable_plugin by the way. but its a slightly differnt topic | 22:13 |
| sean-k-mooney | clarkb: before askign it i was going to wait and see fi you wanted to discuss this more. | 22:16 |
| clarkb | I think we've reached a reasonable conclusion. Basically keep the power user feature but allow devstack to retain control for any env vars it may choose to control later | 22:17 |
| sean-k-mooney | ack, so my other question is also about defaultign but in this case its defintly a bug | 22:17 |
| sean-k-mooney | or well unintiutive behavior | 22:17 |
| sean-k-mooney | so enable_plugin take a name for the directory under $DEST i.e. under /opt/stack | 22:18 |
| sean-k-mooney | a url for the plugin repo | 22:18 |
| sean-k-mooney | and an optional branch which defaults to master | 22:19 |
| sean-k-mooney | https://github.com/openstack/devstack/blob/master/functions-common#L1740 | 22:19 |
| sean-k-mooney | then assumeing the directoy does not exist and error on clone is not enabled | 22:19 |
| sean-k-mooney | we evnetully clone it here https://github.com/openstack/devstack/blob/master/functions-common#L634-L636 | 22:19 |
| sean-k-mooney | git_timed clone --no-checkout $git_clone_flags $git_remote $git_dest | 22:19 |
| sean-k-mooney | cd $git_dest | 22:19 |
| sean-k-mooney | git_timed fetch $git_clone_flags origin $git_ref | 22:19 |
| sean-k-mooney | so fun fact | 22:20 |
| sean-k-mooney | after the first two command your in the cloned repo checked out to the drfault branch | 22:20 |
| sean-k-mooney | if that is main an you forgot to pass that then the fetch fails assuming the repo does not have a master braanch | 22:20 |
| sean-k-mooney | so if you unstack fix your local.conf and stack again | 22:21 |
| sean-k-mooney | you install runs to completion but does not actully checkout main | 22:21 |
| sean-k-mooney | so your plugin is never run | 22:21 |
| sean-k-mooney | and the service is not enabled | 22:21 |
| clarkb | right the clone will checkout the upstream default branch HEAD | 22:21 |
| sean-k-mooney | because the git repo exist with no work tree chekced out | 22:21 |
| clarkb | and you're relying on the fetch && checkout FETCH_HEAD to switch to the requested branch | 22:22 |
| clarkb | an easy fix here may be to delete the git repo if the fetch or checkout FETCH_HEAD fail | 22:22 |
| sean-k-mooney | ya so two questions 1 can i set the default barnch in enabled plugins to a sential | 22:22 |
| sean-k-mooney | and then move the defaulting to git_clone? | 22:22 |
| clarkb | no I mean you'd run the first time and fail and it would clean up the git repo so after you fix the config and rerun it will clone and fetch again | 22:23 |
| sean-k-mooney | and the second question is there a better way to have it default to the default branch if none is passsed | 22:23 |
| clarkb | the problem is that it isn't recloning, fetching and checking out the second pass I think beacuse the repo already exists | 22:23 |
| clarkb | but now that you mention having git clone do it I wonder if that is possible | 22:23 |
| sean-k-mooney | well we only need to do the fetuch if your fetching the non default branch | 22:24 |
| sean-k-mooney | like if we remove the --no-checkout | 22:25 |
| clarkb | looks like git clone --branch $branch would do it | 22:25 |
| clarkb | but your second question would be defeated by ^ I think | 22:25 |
| clarkb | you basically want to rely on the upstream default if there is no default set locally | 22:25 |
| clarkb | and if a default is set locally it should handle errors better | 22:25 |
| sean-k-mooney | right | 22:25 |
| sean-k-mooney | it didnt take long to figure out what was broken once i went looking | 22:26 |
| sean-k-mooney | most of our repos obvisouly use master | 22:26 |
| sean-k-mooney | so this is normally not a problem | 22:26 |
| clarkb | idea: https://github.com/openstack/devstack/blob/master/functions-common#L1740 make the default there empty string. Then test if empty string drop the --no-checkout. If not the empty string replace --no-checkout with --branch $thatvalue | 22:26 |
| sean-k-mooney | ya i was wondering if empty string would be a good sentinal | 22:27 |
| clarkb | I think something along ^ those lines would work. It make break plugins that are in a weird situation where the default branch isn't master but master is present | 22:27 |
| clarkb | I doubt there are many cases or any of that though | 22:27 |
| sean-k-mooney | hum well not in ci but it might | 22:28 |
| sean-k-mooney | so it will break one usecase i had but that you didnt want to supprot | 22:28 |
| sean-k-mooney | well perhaps it was not you i shoudl say devstack | 22:28 |
| sean-k-mooney | so fun fact you can set git_ref to a branch or /ref/for/wahtever or a tag or a git commit | 22:29 |
| sean-k-mooney | but only if you have reclone=true or the repo does nto exist | 22:29 |
| sean-k-mooney | commit shas dont work i belive in general | 22:29 |
| sean-k-mooney | if we use --branch | 22:30 |
| sean-k-mooney | that may or may not impact which of those 4 options work | 22:30 |
| sean-k-mooney | i assume --barnch actully needs to be a branch | 22:30 |
| sean-k-mooney | clarkb: i can push up 1 or 2 patches to compare the options and see what peopel think | 22:33 |
| sean-k-mooney | clarkb: the reason i used to care about commits is the two plugins i used to maintian to complie "ovs with dpdk" or libvirt+qemu | 22:35 |
| sean-k-mooney | but supproted checking out specific commitn and then applying patches form the relevnet mailing list (via patchwork) | 22:35 |
| sean-k-mooney | so i used to maintain my onw similar version to add that functionatliy | 22:36 |
| sean-k-mooney | https://opendev.org/x/devstack-plugin-libvirt-qemu/src/branch/master/devstack/libs/libvirt#L10-L69 | 22:36 |
| clarkb | ya I think --branch may break you arbitrary ref thing | 22:38 |
| clarkb | which is maybe why it does no checkout then checkout after | 22:38 |
| clarkb | I think we can preserve that with better error handling and cleanup though | 22:38 |
| clarkb | basically don't use --branch. Handle empty default to mean use the default (so drop the --no-checkout in that case). If tehre is a value set then do no-checkout, then fetch if the fetch fails delete the git repo. if the fetch succeeds check it out | 22:39 |
| sean-k-mooney | ya. well honestly what i care more about woudl be making sure gerrit refs still work, but that is its own branch https://github.com/openstack/devstack/blob/master/functions-common#L614 | 22:39 |
| sean-k-mooney | so it shoud proably be fine | 22:40 |
| sean-k-mooney | clarkb: ya that sound like it should work and is unlike to regress anything | 22:40 |
| sean-k-mooney | clarkb: when i test that and submit a patch should i file a bug for it our just give it a good commit messange and sane topic | 22:43 |
| sean-k-mooney | im not sure how much devstack lauchpad bug trackker is actully used | 22:44 |
| clarkb | I'm always a fan of making things easy. If it were me I'd probably not bother with a separate issue since this something that should be able to eb captured in a commit message | 22:44 |
| sean-k-mooney | ack | 22:44 |
| clarkb | if this required updates across projects then its probably worth tracking in launchpad | 22:44 |
| clarkb | but it shouldn't | 22:45 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!