shuffle domains before renewal, to remedy the process being interrupted #154
Conversation
There was a problem hiding this comment.
@luto: Thanks! I think this is a good idea, but I left an inline comment about the shuffling process probably not being randomized (and sorry, this is probably far more rambling details about Lua random numbers than you wanted to know. ;).
Let me know if any of that doesn't make sense, or you have any other thoughts. If pulling in that function based on resty.random to seed the random numbers sounds good, feel free to copy that code, or I can also pull that code in and merge this (probably later this week).
Thanks again!
lib/resty/auto-ssl/jobs/renewal.lua
Outdated
| @@ -134,13 +134,22 @@ local function renew_check_cert(auto_ssl_instance, storage, domain) | |||
| renew_check_cert_unlock(domain, storage, local_lock, distributed_lock_value) | |||
| end | |||
|
|
|||
| local function shuffle(tbl) | |||
| for i = #tbl, 1, -1 do | |||
| local rand = math.random(i) | |||
There was a problem hiding this comment.
Unless you're calling math.randomseed somewhere else in your Lua code, this shuffle function might not actually be random. This is definitely an annoying pitfall of Lua, since math.random will consistently generate the same sequence of "random" numbers unless it's seeded first:
luajit -e "print(math.random())" && luajit -e "print(math.random())" && sleep 2 && luajit -e "print(math.random())"
0.79420629243124
0.79420629243124
0.79420629243124And while seeding with os.time() is a common suggestion, that still may not be effective if you need different random numbers in different nginx workers, since os.time() only provides resolution to the time in seconds, so processes started within the same second still get the same sequence of random numbers:
luajit -e "math.randomseed(os.time()); print(math.random())" && luajit -e "math.randomseed(os.time()); print(math.random())"
0.44159042313168
0.44159042313168
# Seeding with time is only effective, if enough time has passed between seeding:
luajit -e "math.randomseed(os.time()); print(math.random())" && sleep 2 && luajit -e "math.randomseed(os.time()); print(math.random())"
0.30533206441205
0.73120898168943I'm not entirely sure we need to worry about nginx workers having their own random seeds for this specific context (since only one worker is executing this code at a time), but if we have lua-resty-auto-ssl perform a math.randomseed, I'd want to be careful not to potentially overwrite a better random seed other Lua code may have already performed.
Here's an example function I've written that we could pull into lua-resty-auto-ssl that should more effectively seed the random number generator: https://github.com/NREL/api-umbrella/blob/9dced845723c38c63d9ad54dcfe15837a460ca7f/src/api-umbrella/utils/random_seed.lua and https://github.com/NREL/api-umbrella/blob/9dced845723c38c63d9ad54dcfe15837a460ca7f/src/api-umbrella/proxy/hooks/init_worker.lua#L9-L12 (the double seeding in both the init and init_worker phases may not be necessary in our case here if we don't need random numbers in the init phase, although it may not be a bad idea, just to ensure any future code doesn't run into this pitfall).
Or alternatively, if we don't want lua-resty-auto-ssl to perform the math.randomseed for users, we could just document this and leave it up to the user to perform this themselves in the case they care about this renewal ordering issue. Technically math.randomseed is global and lua-resty-auto-ssl setting it could affect other Lua code running in the same nginx process, but if we seed it with a good random number (like from the lua-resty-string's resty.random), then it seems like this should be a safe thing to do (and would save users the headache of having to figure all this silliness out).
So I guess I'm leaning more towards pulling in some code to perform the seeding during our init_worker setup.
While "random.number" would be simpler, it's only available from the third party bungle/lua-resty-random module. This implementation leverages the "resty.random" implementation that's built into OpenResty, which only provides a random chars function.
|
This is finally released as part of v0.13.0. Thank you! |
Our resty processes get reloaded quite often, which stops running renewal processes. This makes the order of domain renewals random, so we don't try the same, too short list over and over.