feat(clustering) config export on demand

[StarlightIbuki]

fix FT-3308

[bungle]

This reminds me of this:
#9082

[StarlightIbuki]

This reminds me of this: #9082

yeah, we reduced 4 times of the process to 2, and now it should be 1

[bungle]

This also reminds me of this, @suika-kong please read carefully all the comments there:
#8840

[bungle]

While I think on demand is good, it does not solve the double problem. A lot of code will still be double and executed double when you have both types of clients connected. I think common code should be common and executed only once.

That is:

1. there is no reason both wrpc and old do export
2. there is no reason both calculate hashes
3. there is no reason both json encode config
4. there is no reason both compress the config

[StarlightIbuki]

While I think on demand is good, it does not solve the double problem. A lot of code will still be double and executed double when you have both types of clients connected. I think common code should be common and executed only once.

That would be hard. We've discussed that earlier: the format of the 2 protocols are very different, and the most we could do here is to share the config table exported. I think that was the reason why we did not do it that time.

[bungle]

That would be hard. We've discussed that earlier: the format of the 2 protocols are very different, and the most we could do here is to share the config table exported. I think that was the reason why we did not do it that time.

Last time I checked it was not that hard. The wRPC is no different, it just wraps the above in its own protocol. What I said above is common, AFAIK. It means though a lot of code needs to be moved.

[bungle]

While I think on demand is good, it does not solve the double problem. A lot of code will still be double and executed double when you have both types of clients connected. I think common code should be common and executed only once.

That is:

1. there is no reason both wrpc and old do export
2. there is no reason both calculate hashes
3. there is no reason both json encode config
4. there is no reason both compress the config

I stand corrected, only the 1. and 2. are easily shareable. The 3. and 4. are not.

[fffonion]

@suika-kong if remove the duplicate code is too much work to do, let's not it do it in this PR (but sure to have a ticket to record it). As CP mostly takes to DP in either of the protocol, making it "on demand" should solve the CP 2x latency issue in most cases, let's focus on ship that and also solve the caching issue Aapo mentioned in 8840.

[flrgh]

if ok is truth-y but self.clients is empty, this branch will be followed because err will be nil and generate a confusing error log entry (I think).

Maybe it would be more straightforward to put the self.clients check into push_config?

function _M:push_config()
  if not next(self.clients) then
    return true
  end

  local start = ngx_now()

  local payload, err = self:export_deflated_reconfigure_payload()
  if not payload then
  -- ...
end

...otherwise I think this needs to be changed to if err and err ~= "timeout" then.

[flrgh]

Alternatively, maybe the self.clients check could be pushed to the event handler? This would be simple for both control_plane and wrpc_control_plane:

  kong.worker_events.register(function(_)
    if next(self.clients) and push_config_semaphore:count() <= 0 then
      -- the following line always executes immediately after the `if` check
      -- because `:count` will never yield, end result is that the semaphore
      -- count is guaranteed to not exceed 1
      push_config_semaphore:post()
    end
  end, "clustering", "push_config")

[dndx]

@flrgh The first approach has some issues, if we return true inside push_config, then the timer of push_config_loop is still reset which means that loop will wait for db_update_frequency before even pushing out another updated config.

The second issue might work (I did not spot immediate issue with it), but it seems to be a bit more dangerous as there are a lot of asynchronous going on between worker events and the semaphore. It's a bit harder to argue the correctness.

My thought:

If we change the push_config_loop function to do the following:

while not exiting() do
    local ok, err = push_config_semaphore:wait(1)
    if exiting() then
      return
    end

    if ok then
      if next(self.clients) do
        ok, err = pcall(self.push_config, self)
        ...

Does this also work? It seems to be simpler than the current approach and does not affects the coalescing timer. Basically, we consume the semaphore, but pretends nothing happened. The current code does the same thing, it just seems to be more complex on the error handling part.

[flrgh]

Suggested change

[dndx]

Suggested change

[dndx]

Suggested change

	end
	end

[StarlightIbuki]

Closing for @dndx 's PR. #9368