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
