12:00:12 <dviroel> #startmeeting watcher
12:00:12 <opendevmeet> Meeting started Thu Jul 24 12:00:12 2025 UTC and is due to finish in 60 minutes.  The chair is dviroel. Information about MeetBot at http://wiki.debian.org/MeetBot.
12:00:12 <opendevmeet> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
12:00:12 <opendevmeet> The meeting name has been set to 'watcher'
12:00:35 <dviroel> hi all, who's around today?
12:00:50 <amoralej_> o/
12:01:12 <dviroel> courtesy ping: sean-k-mooney chandankumar morenod rlandy
12:02:43 <dviroel> let's start with today's meeting agenda
12:02:49 <rlandy> o/
12:02:55 <dviroel> #link https://etherpad.opendev.org/p/openstack-watcher-irc-meeting#L26 (Meeting agenda)
12:03:14 <dviroel> feel free to add your own topics to the agenda
12:03:37 <dviroel> first one
12:03:48 <dviroel> #topic Eventlet Removal
12:03:54 <chandankumar> o/
12:04:11 <dviroel> as usual, this topic is to cover the progress on eventlet removal patches
12:04:28 <dviroel> this week, there isn't too much updates
12:04:40 <dviroel> #link https://etherpad.opendev.org/p/watcher-eventlet-removal (watcher evenlet removal etherpad)
12:05:03 <dviroel> at the beginning of the etherpad there is a list of changes open for review
12:05:11 <dviroel> one patch recently merged
12:05:33 <dviroel> the one that merge dec-engine services
12:05:39 <dviroel> #link https://review.opendev.org/c/openstack/watcher/+/952499
12:05:58 <dviroel> the others are open for review
12:06:01 <sean-k-mooney> are you plannign to respine https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/954264
12:06:38 <dviroel> sean-k-mooney: yeah, i was thinking yes, but didn't get into it yet
12:06:47 <dviroel> or maybe provide a follow up to improve it
12:07:05 <sean-k-mooney> ack i had a look at it and you seaid you would fix one of the commend in the next patchset 9 days ago
12:07:12 <sean-k-mooney> so i was partly waiting for you to do that
12:08:15 <dviroel> ack, I can work on it in until the end of the week
12:08:54 <dviroel> i was busy with other patches, which i should also propose soon
12:09:10 <sean-k-mooney> ack no worreis
12:09:17 <dviroel> ack, we can wait for a new ps on the plugin
12:09:21 <sean-k-mooney> i aslo kind of agree with alfredo about chaning how the test works
12:09:30 <dviroel> yeah me too
12:10:06 <sean-k-mooney> i think it might be useful to have a dummy sleep stagey that we could use instead
12:10:31 <sean-k-mooney> i..e one that will generate sleep actions
12:10:45 <amoralej> chandankumar is working in another tempest test for continuous
12:10:48 <sean-k-mooney> noop is also ok
12:10:58 <sean-k-mooney> there are pros and cons to both aproches
12:11:10 <amoralej> there is a dummy strategy which creates a testplan with two nop and one sleep
12:11:36 <sean-k-mooney> yes so i think the main continue audit test shoudl use that or somethign eimilar
12:11:37 <amoralej> i proposed to use it in https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/955472
12:11:42 <sean-k-mooney> not the zone migration stragy
12:11:57 <sean-k-mooney> and i dont think we shoudl have a continues version of each stragey
12:12:09 <amoralej> but it's still good to have one as dviroel one to test stuff related to model, which has shown to be problematic
12:12:30 <dviroel> yeah, unless there is a reason, we can use the dummy instead
12:12:32 <amoralej> nop or sleep actions will not validate access to model
12:12:39 <sean-k-mooney> yes and no
12:12:59 <sean-k-mooney> test of the continue audit fucntionalty shoudl be isolated form testing the logic of the stragies
12:13:14 <sean-k-mooney> they should be tested independtly
12:13:39 <sean-k-mooney> model updates are independent of the functioning of the continous audit
12:13:40 <amoralej> yes, i think that's good, that's why i proposed to use dummy for the a full continuous cycle audit
12:13:47 <sean-k-mooney> +1
12:14:17 <sean-k-mooney> just as an aside.
12:14:26 <amoralej> but as dviroel found, continuous audits are sensitive to issues related to access to model
12:14:27 <sean-k-mooney> if you are adding coment for multipel steps in a test
12:14:46 <dviroel> agree, the scenario that I am testing is a corner case with threading mode
12:14:59 <sean-k-mooney> that is generaly an indication that you are not propelry breaking up the test into the seprate parts or should at least condier pulling the part out into helper funcitons
12:15:06 <amoralej> so there is space for both tests
12:15:15 <chandankumar> I was also working on testing continous audit with cron format here https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/955472 got feedback from amoralej to use nop strategy . just wanted to highlight here to avoid duplicy
12:15:32 <sean-k-mooney> the corner case shoudl be tested in the unit/functional suite not in the integration tests in general
12:16:21 <dviroel> i can check how it would be possible to do that
12:16:29 <chandankumar> ah amoralej already spoke about the same.
12:16:56 <sean-k-mooney> chandankumar: yep we can loop back to the topic of cron format supprot
12:17:43 <amoralej> it's interesting to test that kind of synchronization issues with unit tests ..
12:18:30 <sean-k-mooney> unit and functional test are best positoin to test that
12:18:40 <sean-k-mooney> tempest is very bad at testing race condtions
12:18:51 <sean-k-mooney> we generally avoid that if possible
12:20:01 <sean-k-mooney> but i dont know the detail of the issue dviroel found with threading as there isnt a bug report for it that expains it properly.
12:20:34 <dviroel> it is not really a race issue, but an initialization problem, of the continuous audit service, which only happens in threading mode
12:21:01 <sean-k-mooney> well you talks baout runnign the threads in diffent processes
12:21:14 <sean-k-mooney> which woudl not change with or witherh trehadign
12:21:53 <sean-k-mooney> and 2 thread can share a single data stucrue in memory
12:22:20 <sean-k-mooney> so if they are indeepently creatign there own in memory model that is a bug even with eventlet
12:23:50 <sean-k-mooney> a tempest test is not the correct way to prevent regressing that type of but a unit and preferably functional test is
12:23:57 <sean-k-mooney> but perhaps we shoudl move on
12:24:06 <sean-k-mooney> we can dicsuss that outside the meeting
12:24:25 <dviroel> ack, i can check how I can reproduce the issue with funcional tests perhaps
12:25:00 <dviroel> yes, we can move on
12:25:11 <dviroel> thanks for the discussions on that topic
12:25:45 <dviroel> that's what I have for the evenlet
12:25:50 <sean-k-mooney> before we move on we shoudl try and create  a bug report for this
12:26:05 <sean-k-mooney> and likely we want to fix this indepenetly of the eventlet removal work
12:26:25 <sean-k-mooney> or at least in a way we woudl be comfortabel backporting
12:26:36 <sean-k-mooney> as it stand we would need to split https://review.opendev.org/c/openstack/watcher/+/952257/9
12:26:42 <sean-k-mooney> if we were to backport
12:27:03 <dviroel> ack, I can file a Bug and try to reproduce it in a evenlet env
12:27:19 <sean-k-mooney> you should be able to use id()
12:27:25 <sean-k-mooney> or `is`
12:27:47 <sean-k-mooney> to assert the refence to the data model is the same in the backgroud schduler as it is in the main data model i think
12:28:08 <dviroel> ack
12:28:15 <dviroel> will give a try on that
12:28:47 <dviroel> tks sean-k-mooney
12:28:53 <dviroel> moving to the next topic then
12:29:10 <dviroel> #topic Skip actions feature update https://blueprints.launchpad.net/watcher/+spec/add-skip-actions
12:29:28 <amoralej> i added that one
12:30:01 <amoralej> i've sent some patches and I'd like to share the progress and how I did the split of the feature
12:30:14 <dviroel> so you have an spec update too
12:30:14 <amoralej> #link https://review.opendev.org/c/openstack/watcher-specs/+/954718
12:30:35 <amoralej> yes, that's just adding the same field status_message to the audit for convenience
12:30:57 <amoralej> as it can be useful at some point, i think it'd be good to be covered by the same microversion
12:31:18 <dviroel> ack, as discussed previously here in the channel I think
12:31:22 <amoralej> exacgtly
12:31:37 <amoralej> #link https://review.opendev.org/c/openstack/watcher/+/954745
12:31:38 <sean-k-mooney> yes it was
12:31:53 <sean-k-mooney> i left a minor comment on the spec update but i htink it good in general
12:32:13 <amoralej> that's the first implementation one, it just adds the status_messages to the database, object, notification and adds api microverson
12:32:32 <sean-k-mooney> first feedback on that is your doing a lot in one patch
12:32:38 <amoralej> i will fix that sean-k-mooney , thanks
12:33:03 <sean-k-mooney> noramly you woudl do the object change in one patch db in anothe rand the api change gos a the end of the serise wehn the entire feature is functional
12:33:57 <dviroel> +1
12:34:25 <amoralej> so, that one should be three patches? 1st db, 2nd, object, 3rd api change?
12:35:00 <amoralej> note that's just status_message, nothing related to the SKIPPED yet
12:35:23 <sean-k-mooney> yes can you split this in 3 , db schema changes in first patch, object and notificaiton changes in secodn and api and apiref changes in the final pathc whihc shoudl go after the other 2 you have already proposed
12:36:05 <amoralej> so, i could join both api changes (status_message and patch action) in the same patch and api microviersion ?
12:36:21 <sean-k-mooney> yes you can do that and proably should
12:36:42 <sean-k-mooney> do you have anything that is setting the status message to a value in the first patch
12:36:53 <amoralej> nop, but in the second
12:36:58 <amoralej> when doing automatic SKIPPED
12:37:10 <sean-k-mooney> right so we do not make the api changes before the code that uses the new fields
12:37:12 <amoralej> i'm setting status_message
12:37:33 <sean-k-mooney> the api change alwasy comes at the end after the feature is fully working.
12:37:59 <amoralej> ok, let's move on to the next patches, and at the end we can agree to the entire sequence of patches
12:38:22 <amoralej> #link https://review.opendev.org/c/openstack/watcher/+/954746/
12:38:32 <amoralej> that's the automatic skipped based on pre_condition
12:39:46 <dviroel> ack, so this one is using the  fields from previous patch
12:39:53 <amoralej> yes
12:40:19 <amoralej> in this one, i'm extending the nop action to ease testing different situations https://review.opendev.org/c/openstack/watcher/+/954746/4/watcher/applier/actions/nop.py
12:40:35 <amoralej> i hope i don't need an spec to modify the nop action :)
12:40:54 <sean-k-mooney> well technialy you do
12:41:10 <sean-k-mooney> beacuse your modifyint eh api schema for the parmaters
12:41:23 <amoralej> yes, but it's just testing action ....
12:41:27 <sean-k-mooney> i think case i think we are likely ok with this as part of the larger work
12:41:34 <sean-k-mooney> there is no just
12:41:41 <sean-k-mooney> it follow the same rules as the rest
12:41:56 <sean-k-mooney> but you r right that its not going to break real uscases
12:42:26 <sean-k-mooney> i think this is ok but can you split it into its own commit
12:42:43 <sean-k-mooney> we might want to backprot that for other testing usecases in the future
12:42:51 <amoralej> the problem is that, for that i need the new exception i'm creating
12:42:54 <sean-k-mooney> so having it as its own commit will make that simpeler
12:42:59 <amoralej> so we have chicken-and-egg
12:43:19 <sean-k-mooney> well you dont you can extend it twice
12:43:37 <sean-k-mooney> once to add the fail cases
12:43:42 <amoralej> ok, i can create one commit for the nop action for "everything" but the skip part
12:43:45 <sean-k-mooney> and then again for the skip part
12:43:52 <amoralej> ok
12:43:52 <sean-k-mooney> exactly
12:44:11 <sean-k-mooney> that can go really early in the series for what its worth
12:44:21 <sean-k-mooney> although the order is up to you
12:44:23 <amoralej> yep, actually, would not depend on anything else
12:44:38 <sean-k-mooney> right that was my thinking as well
12:44:44 <amoralej> #link https://review.opendev.org/c/openstack/watcher/+/955753/
12:44:58 <amoralej> that's the new PATCH /actions/<id> part
12:45:09 <amoralej> includes its own microversion
12:45:39 <sean-k-mooney> ya again i would split that in 2, one for the non api part first and hten combine the othe rapi change form before into the second patch and do all the api changes at once
12:46:17 <amoralej> i think there is no non-api part
12:46:21 <amoralej> lemme double check
12:46:37 <sean-k-mooney> you hav changes to the applier
12:46:50 <sean-k-mooney> but your right its mostly api and docs
12:47:15 <dviroel> only applier piece yes
12:47:17 <amoralej> it's very closely related to the api change
12:47:33 <amoralej> actually, i'd may do that when adding the SKIPPED state
12:47:43 <amoralej> it'd be more correct probably
12:47:54 <sean-k-mooney> that might be better its not actully related to the api changes it related to the object changes
12:48:04 <amoralej> yes, i will do that, it makes sense
12:48:27 <sean-k-mooney> so this 3 patch serise will likely be 5-7 after the bits are split and re arranged
12:48:32 <amoralej> somethink to remark, i'm adding https://review.opendev.org/c/openstack/watcher/+/955753/1/watcher/api/controllers/v1/types.py
12:48:42 <sean-k-mooney> and the nop change can be merged indepentely of the rest
12:48:49 <amoralej> a new way to validate patch calls based on allowed_attributes only
12:49:06 <amoralej> in addition to internal_attributes
12:49:20 <amoralej> i think that may be useful for others
12:49:31 <amoralej> it's a more restrictive way
12:49:36 <sean-k-mooney> if you think it might be it should be its on patch
12:49:43 <sean-k-mooney> but alos we shoudl liekly discuss that seperatly
12:49:56 <sean-k-mooney> thanks for highligting it however
12:50:11 <amoralej> implementing here adds a use case to discuss it, which is useful i think
12:50:15 <amoralej> but i can do depends on
12:50:17 <sean-k-mooney> i want to dicsson oru api implation a tthe ptg
12:50:37 <sean-k-mooney> * cant type right now
12:51:18 <sean-k-mooney> im generallly nnot super happy with how we do api validation and the technologies/libs watcher is cureently useing
12:51:30 <sean-k-mooney> but i think any large refactorign of that shoudl be indepenent of your series
12:52:06 <amoralej> so, should i avoid doing that?
12:52:31 <amoralej> tbh, i felt it was an improvement but there may be a better way
12:52:33 <sean-k-mooney> was it discussed in the spec as part of the feature
12:53:05 <amoralej> nop, but that's just an internal implementation detail, do not affect users
12:53:05 <sean-k-mooney> we can discuss it and perhaps still do it this cycle but its not part of the skipped feature
12:53:23 <sean-k-mooney> it changes api validation and there for could
12:53:31 <sean-k-mooney> so its boarderline
12:53:37 <amoralej> note i'm not applying to any other call
12:53:49 <amoralej> default behavior is keeping the existing behavior
12:53:59 <sean-k-mooney> and it skips if its empty so its backward compatible
12:54:05 <amoralej> exactly
12:54:13 <sean-k-mooney> but again it should be in a diffent commit
12:54:19 <amoralej> ok, i will
12:54:30 <sean-k-mooney> if we ever need that fuctionalty to fix a bug we woudl want to be able to cleanly backport it
12:54:42 <sean-k-mooney> instead of extracting it form this change
12:55:25 <dviroel> amoralej: any other patch that you want to highlight?
12:55:36 <amoralej> 1. nop action change 2. change in validation 3. db change 4. object + notification 5. adds SKIPPED state 6. api change with microversion
12:55:58 <amoralej> #link https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/955775
12:56:04 <amoralej> last one, that's the tempest test
12:56:36 <amoralej> ah, i need to do the skip for previous versioins
12:57:17 <amoralej> so, that's it
12:58:06 <dviroel> altight, thanks for all the proposed changes, I will take some time to review then after you submit again
12:58:08 <amoralej> thanks for your feedback, it's very helpful
12:59:06 <dviroel> we have only 2 min left, so i will just skip the bug triage,  but there are 2 new bugs there, if you want to take a look
12:59:29 <dviroel> chandankumar: i believe that you might want to discuss more about yours?
12:59:51 <dviroel> we are out of time, if needed, we can continue discussing afterwards in the channel
13:00:15 <dviroel> the other one is a doc bug only, with a patch proposed, we can chat about next week
13:00:16 <chandankumar> dviroel: we can revisit next time
13:00:23 <dviroel> #topic Volunteers to chair next meeting
13:00:33 <chandankumar> dviroel: I will chair for next meeting
13:00:33 <dviroel> someone wants to chair?
13:00:38 <dviroel> thanks chandankumar
13:01:01 <dviroel> let's wrap up for today
13:01:09 <dviroel> we will meet again next week
13:01:14 <dviroel> thank you all for participating
13:01:19 <dviroel> #endmeeting