fix(clustering): data-plane node id keeps changing after restarted in hybrid mode

[vm-001]

Summary

Kong creates a new node_id every time it restarts. In hybrid mode, CP will insert a new row to the clustering_data_planes table after DP restart because it provides a new node_id. If DPs restart enough times in a period of time which is defined by cluster_data_plane_purge_delay, clustering_data_planes rows will increase a lot. This most likely fills up the prometheus_metrics shared dict.

Full changelog

• Avoid repeatedly generating different node_id after kong restarted by writing node_id to file

Issue reference

FTI-4168

[flrgh]

Great idea @vm-001 😀 I am surprised this was not done before.

Mind adding an integration test for this?

[vm-001]

Great idea @vm-001 😀 I am surprised this was not done before.

Mind adding an integration test for this?

Added.

[vm-001]

Sorry for leaving this PR for some time since I have some high-priority tasks that need to work on.

After some digging, I found that if Kong uses a single file to store the node_id and share with both http and stream mode. This will cause some critical issues in cluster event, that is the cluster events that are published by the http mode instance will be ignored in the stream mode instance as they shared the same node id in the filesystem, which is wrong.

So I decide to use different file to store the node id for the different ngx mode.

• ${prefix}/node.id/http for http mode.
• ${prefix}/node.id/stream for stream mode.

[flrgh]

1. rebased onto master
2. addressed most of the code review cleanup/consistency feedback
3. added more test coverage

[flrgh]

I also addressed @dndx's requests to relocate each local knode = kong and kong.node or require "kong.pdk.node".new() expression. The tests are passing with this change having been made, so I think it's okay, but @vm-001 will know better if there are any issues to be aware of in regard to this.

[flrgh]

It has tests and is simple enough of a change such that I'm confident any potential issues can be corrected before code freeze. I will merge once the latest CI run has concluded.

@vm-001 thanks for enduring the long road to get this merged in. Can you please give all the review comments another read to see if there are any follow-up items that need another PR? Also, please look at the additional integration tests I've added to make sure that the correct behavior is reflected. I had to mark one test as pending because of some order-of-operation oddities, and I wasn't sure if I'd break something else by trying to fix it.

[flrgh]

I can't help but feel like all of the logic should go into kong.pdk.private.node instead. As it is, it's still hard to follow the order of operations to know if there's a chicken/egg situation.

You have spent more time working through the problem, so maybe I'm just missing something?

Here's how I picture the call stack:

kong.init()
 -> kong.global.init_pdk(self, kong_config)
  -> kong.pdk.new(kong_config, self)
    -> kong.pdk.node.new(self)
      -> kong.pdk.private.node.init(self)

...and kong.pdk.node.private.init() could look something like this:

-- init() is a no-op if called more than once
if already_initialized() then
  return
end

-- check shm first, since this might have been a reload
local shm_id = shm:get(MY_ID_KEY)

-- the simple case
if self.configuration.role ~= "data_plane" then
  if not shm_id then
    shm_id = generate_uuid()
    store_to_shm(shm_id)
  end
  return
end

local fs_id = read_id_from_filesystem()

if shm_id and not fs_id then
  store_to_filesystem(shm_id)

elseif fs_id and not shm_id then
  store_to_shm(fs_id)

elseif not fs_id and not shm_id then
  local id = generate_uuid()
  store_to_filesystem(id)
  store_to_shm(id)

elseif fs_id ~= shm_id then
  -- uh oh, need to pick a winner
  -- probably should keep shm_id in this case because if it exists,
  -- that means we're a reload, so it's already being used by the
  -- current process group
  log("uh oh this is weird...")
  store_to_filesystem(shm_id)

else
  -- no action needed if this is the case
  assert(fs_id == shm_id)
end

At this point the kong.node.get_id() PDK function just needs to be updated to not attempt to create the node_id itself:

  function _NODE.get_id()
    if node_id then
      return node_id
    end

    local shm = ngx.shared.kong

-    local ok, err = shm:safe_add(NODE_ID_KEY, utils.uuid())
-    if not ok and err ~= "exists" then
-      error("failed to set 'node_id' in shm: " .. err)
-    end

    node_id, err = shm:get(NODE_ID_KEY)
    if err then
      error("failed to get 'node_id' in shm: " .. err)
    end

    if not node_id then
      error("no 'node_id' set in shm")
    end

    return node_id
  end

[vm-001]

@flrgh This is a cleaner approach. But there may have an issue:

kong.init()
 -> kong.global.init_pdk(self, kong_config)
  -> kong.pdk.new(kong_config, self)
    -> kong.pdk.node.new(self)
      -> kong.pdk.private.node.init(self)

Kong calls kong.global.init_pdk in the cmd stage, and the store_to_shm function won't work as ngx.shared API is not available in the stage.

[vm-001]

It has tests and is simple enough of a change such that I'm confident any potential issues can be corrected before code freeze. I will merge once the latest CI run has concluded.

@vm-001 thanks for enduring the long road to get this merged in. Can you please give all the review comments another read to see if there are any follow-up items that need another PR? Also, please look at the additional integration tests I've added to make sure that the correct behavior is reflected. I had to mark one test as pending because of some order-of-operation oddities, and I wasn't sure if I'd break something else by trying to fix it.

@flrgh Thank you for your help. And the fix PR is here: #9790

[vm-001]

I appreciate everyone who spent time involved in this PR.