fix: persist node id for reuse

[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

[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]

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.

Hmm. Is there any different way we can address the cluster events problem? I do realize this may greatly increase the scope of your efforts, but in my opinion having two distinct "node" IDs is not an optimal solution and could lead to more confusion further down the line.

The maintenance of the node ID file should be in one lua file and share the file name creation logic.

Agree. Putting all the logic in kong.global is one idea. Putting it in a submodule in the kong.pdk.private namespace is another idea, assuming this doesn't put us in a chicken<->egg situation with regards to instantiating the PDK.

[vm-001]

Hmm. Is there any different way we can address the cluster events problem? I do realize this may greatly increase the scope of your efforts, but in my opinion having two distinct "node" IDs is not an optimal solution and could lead to more confusion further down the line.

The alternative solution is probably to use one node_id to share both http and stream mode, but the sub_system may have to be stored in the database to distinguish which mode it was for later use in the /cluster_events/init.lua#process_event(). The tricky part is not adding a new field in the cluster_events table, but the migration for the old event -- how do we determine which mode for the old existing cluster events?

From my understanding. http and stream mode both invoke Kong.init() to initialize an independent Kong instance. I think using a different node_id for different Kong instances is a good&simple solution.

[vm-001]

@hanshuebner @flrgh Yeah. Using a single file to do persist and load logic does improve the maintainability. Since the PDK module is required by global.lua, I prefer to add a file under /pdk/private to do such logic.

[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.

[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: https://github.com/Kong/kong/pull/9790

[vm-001]

I appreciate everyone who spent time involved in this PR.