Re: [PATCH] MINOR: cli: add option to modify close-spread-time
Hi Abhijeet, On Mon, Apr 15, 2024 at 09:48:25PM -0700, Abhijeet Rastogi wrote: > Hi Willy, > > Thank you for your patience with my questions. You're welcome! > > It happens that the global struct is only changed during startup > > I used cli_parse_set_maxconn_global as a reference for my patch and my > understanding is, it does have a race as I do not see > thread_isolate(). > > https://github.com/haproxy/haproxy/blob/fa5c4cc6ced5d8cac2132593ed6b10cf3a8a4660/src/cli.c#L1762 Ah I see. I didn't remember we could change it at run time and it predates threads. In practice there's no race since it writes only a single value. It should theoretically use an atomic write here to do things more cleanly but in reality all the platforms we support are 32-bit and none will make non-atomic writes to an aligned 32-bit location. In your case the difference is that you need to set both a value and a flag and need to make them appear consistently to observers, which is why you'd need this isolation. > >That's easier than it seems if you set an ormask, andnotmask and the new > >value from a local variable. > > Correct, however, I wasn't sure if it's okay to read the mask first, > then get out of the critical section to do other stuff. (in case it's > modified by other threads) What I meant was: - parse the cmd line to local variables (value, ormask, andnotmask) - thread_isolate() - apply these local variables to global ones - thread_release() > Also, it felt natural to minimize the use of thread_isolate(), hence, > I went with a solution where we only use that once per execution. I > will fix this if it doesn't make sense. I'd also want to see only one :-) But the way you've done it is fine, a single one is executed for each "if" branch and the code remains clean enough. > >it would be better to call this "set global close-spread-time" > > Good point, I've done it in the latest iteration. Thanks. However you should not reindent the whole table like this in the same patch, it makes the review way more complicated, and even later if for any reason we need to recheck that patch, this part is horrendous. The right approach when you feel like you need to reindent a table due to some longer keywords, function names or so, is to make a preliminary "cleanup" patch that only does the reindent and promises in the commit message that nothing else was touched, and do you feature patch after that one. This way the first one doesn't deserve any analysis and the second one is clean. I'm seeing that an entry is missing for "doc/management.txt" regarding this new command, please write a few lines about it. Otherwise it's generally OK for me. Thank you! Willy
Re: [PATCH] MINOR: cli: add option to modify close-spread-time
Hi Willy, Thank you for your patience with my questions. > It happens that the global struct is only changed during startup I used cli_parse_set_maxconn_global as a reference for my patch and my understanding is, it does have a race as I do not see thread_isolate(). https://github.com/haproxy/haproxy/blob/fa5c4cc6ced5d8cac2132593ed6b10cf3a8a4660/src/cli.c#L1762 >That's easier than it seems if you set an ormask, andnotmask and the new value >from a local variable. Correct, however, I wasn't sure if it's okay to read the mask first, then get out of the critical section to do other stuff. (in case it's modified by other threads) Also, it felt natural to minimize the use of thread_isolate(), hence, I went with a solution where we only use that once per execution. I will fix this if it doesn't make sense. >it would be better to call this "set global close-spread-time" Good point, I've done it in the latest iteration. On Sun, Apr 14, 2024 at 11:56 PM Willy Tarreau wrote: > > Hi Abhijeet, > > On Mon, Apr 08, 2024 at 08:11:28PM -0700, Abhijeet Rastogi wrote: > > Hi HAproxy community, > > > > Let's assume that HAproxy starts with non-zero values for close-spread-time > > and hard-stop-after, and soft-stop is used to initiate the shutdown during > > deployments. > > There are times where we need to modify these values, eg, in case the > > operator needs to restart HAproxy faster, but still use the soft-stop > > workflow. > > > > So, it will be nice to have the ability to modify these values via CLI. > > > > RFC:- > > - Does this seem like a fair use-case? > > - If this is good, I can also work on the patch for hard-stop-after. > > Yes, I think that both totally make sense from a production perspective. > I hadn't thought about that before, but it's obvious that when you've > deployed with a wrong setting or are observing a bad behavior, you'd > really like the old process to quit much faster. > > > Patch questions:- > > - I've added reg-tests under a new folder "cli" because I was unable to > > find a better match. Let me know if that should be moved. > > I think that's fine. From time to time we move them around when better > classification can be found. > > > - If that's a concern, there is code duplication b/w proxy.c [1] and this > > cli.c. I am unable to find a file where we can create a utility function. > > I'm not seeing anything wrong with what you've done. > > > Mostly, the concern is to modify "global.tune.options" whenever > > "global.close_spread_time" changes. > > Ah, good point! > > > - I noticed global struct is accessed without any locks, is that like a > > "known" race condition for these simple operations? I don't primarily > > develop C code, and this was confusing to me. > > You're totally right, and then you're indeed introducing a bug :-) > > It happens that the global struct is only changed during startup, hence > it's single-threaded at this point. After that it's only read. But for > such rare operations that consist in changing global shared stuff, we > have a secret weapon. We can temporarily pause all other threads during > the change, to guarantee exclusive access to everything. This is done > by issuing: "thread_isolate();" before the critical code, and > "thread_release();" at the end (i.e. around your manipulation of > tune.options and close_spread_time. This means you should rearrange > the parsing function to group the changes together. That's easier than > it seems if you set an ormask, andnotmask and the new value from a > local variable. This way you don't have to care about threads. > > I was also thinking about something, I wouldn't be surprised if we start > to see more commands to change some global ones like this. As such I > think it would be better to call this "set global close-spread-time" so > that is more commands affecting the global section happen later, they'll > already be grouped together and will be easier to find. > > Thanks! > Willy -- Cheers, Abhijeet (https://abhi.host) 0001-MINOR-cli-add-option-to-modify-close-spread-time.patch Description: Binary data
Re: [PATCH] MINOR: cli: add option to modify close-spread-time
Hi Abhijeet, On Mon, Apr 08, 2024 at 08:11:28PM -0700, Abhijeet Rastogi wrote: > Hi HAproxy community, > > Let's assume that HAproxy starts with non-zero values for close-spread-time > and hard-stop-after, and soft-stop is used to initiate the shutdown during > deployments. > There are times where we need to modify these values, eg, in case the > operator needs to restart HAproxy faster, but still use the soft-stop > workflow. > > So, it will be nice to have the ability to modify these values via CLI. > > RFC:- > - Does this seem like a fair use-case? > - If this is good, I can also work on the patch for hard-stop-after. Yes, I think that both totally make sense from a production perspective. I hadn't thought about that before, but it's obvious that when you've deployed with a wrong setting or are observing a bad behavior, you'd really like the old process to quit much faster. > Patch questions:- > - I've added reg-tests under a new folder "cli" because I was unable to > find a better match. Let me know if that should be moved. I think that's fine. From time to time we move them around when better classification can be found. > - If that's a concern, there is code duplication b/w proxy.c [1] and this > cli.c. I am unable to find a file where we can create a utility function. I'm not seeing anything wrong with what you've done. > Mostly, the concern is to modify "global.tune.options" whenever > "global.close_spread_time" changes. Ah, good point! > - I noticed global struct is accessed without any locks, is that like a > "known" race condition for these simple operations? I don't primarily > develop C code, and this was confusing to me. You're totally right, and then you're indeed introducing a bug :-) It happens that the global struct is only changed during startup, hence it's single-threaded at this point. After that it's only read. But for such rare operations that consist in changing global shared stuff, we have a secret weapon. We can temporarily pause all other threads during the change, to guarantee exclusive access to everything. This is done by issuing: "thread_isolate();" before the critical code, and "thread_release();" at the end (i.e. around your manipulation of tune.options and close_spread_time. This means you should rearrange the parsing function to group the changes together. That's easier than it seems if you set an ormask, andnotmask and the new value from a local variable. This way you don't have to care about threads. I was also thinking about something, I wouldn't be surprised if we start to see more commands to change some global ones like this. As such I think it would be better to call this "set global close-spread-time" so that is more commands affecting the global section happen later, they'll already be grouped together and will be easier to find. Thanks! Willy
