feat: add tars discovery #6599

zhixiongdu027 · 2022-03-13T07:44:55Z

What this PR does / why we need it:

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

#6570

Hello everyone:

This is a PR for the tars discovery module, which is part of "tars-proxy".

Work on "tars-proxy" continues, but maybe we can finish tars discovery first

spacewander · 2022-03-13T13:20:03Z

t/tars/discovery/tars.t

+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('debug');


Is it necessary to use debug level?

spacewander · 2022-03-13T13:20:36Z

t/tars/discovery/tars.t

+
+=== TEST 1: create initial server and servant
+--- timeout: 12
+--- yaml_config eval: $::yaml_config


We already set the yaml_config?

spacewander · 2022-03-13T13:23:34Z

t/tars/discovery/tars.t

+    "DONE\n",
+    "{ 1 1 1 }\n",
+]
+--- no_error_log


Let's save the no_error_log like

apisix/t/plugin/gzip.t

Line 40 in 1ac7166

if ((!defined $block->error_log) && (!defined $block->no_error_log)) {

spacewander · 2022-03-13T13:31:09Z

apisix/discovery/tars/init.lua

+
+function _M.init_worker()
+    -- TODO: maybe we can read dict name from discovery config
+    endpoint_dict = ngx.shared.discovery


Let's use a new shdict instead of hijacking authz-keycloak's

spacewander · 2022-03-13T13:33:33Z

apisix/discovery/tars/init.lua

+    end
+
+    local conf = local_conf.discovery.tars
+    default_weight = local_conf.discovery.tars.default_weight or 100


Suggested change

default_weight = local_conf.discovery.tars.default_weight or 100

default_weight = conf.default_weight

The default weight is set already.

spacewander · 2022-03-13T13:36:10Z

apisix/discovery/tars/init.lua

+
+    if not last_fetch_error then
+        db_cli:set_keepalive(120 * 1000, 1)
+    end


Need close if err?

Need to handle this.

Need close if err?

Sorry, I think this place doesn't actively call db:close() should be acceptable,
Here provides agentzh's view

spacewander · 2022-03-13T13:40:35Z

apisix/discovery/tars/init.lua

+        return nil
+    end
+
+    local endpoint = core.json.decode(endpoint_content)


Let's get the err and put it in the msg

spacewander · 2022-03-13T13:42:45Z

apisix/discovery/tars/init.lua

+local activated_buffer = core.table.new(10, 0)
+local nodes_buffer = core.table.new(0, 5)
+
+local endpoints_pattern = core.table.concat(


Could you show us the data which is matched by this regex?

spacewander · 2022-03-13T13:43:55Z

apisix/discovery/tars/init.lua

+end
+
+
+local function create_endpoint_lrucache(servant)


Suggested change

local function create_endpoint_lrucache(servant)

local function add_endpoint_to_lrucache(servant)

spacewander · 2022-03-13T13:44:55Z

apisix/discovery/tars/init.lua

+
+
+local function get_endpoint(servant)
+    local endpoint_version = endpoint_dict:get_stale(servant .. "#version")


Could you tell me why get_statle is used instead of get?

zhixiongdu027 · 2022-03-16T01:46:36Z

@spacewander
Hi, all comment issues have been taken care of. please help review.
Many TKS.

zhixiongdu027 · 2022-03-16T02:50:19Z

@spacewander @membphis

Sorry, I don't understand how this failure happened.
Please give a hand. TKS

spacewander · 2022-03-16T03:04:29Z

apisix/discovery/tars/init.lua

+
+    if not last_fetch_error then
+        db_cli:set_keepalive(120 * 1000, 1)
+    end


Need to handle this.

spacewander · 2022-03-16T03:05:59Z

conf/config-default.yaml

@@ -266,6 +266,7 @@ nginx_config:                     # config for render the template to generate n
      introspection: 10m
      access-tokens: 1m
      ext-plugin: 1m
+      tars: 1m


Let's make it configurable in the next PR.

Need to handle this.

Sorry，I did not get your point
Your means we can keep using this db hanlder event get last_db_error ?

We need to close the handler when getting last_db_error?

You mean we need to decide whether to close or not based on the value of error ?

Currently the case of error=='again' has been handled
But So Sorry for I don't know what other error values do not need to be closed.

According to the example, we need to close if we don't keepalive?
https://github.com/openresty/lua-resty-mysql#synopsis

-- put it into the connection pool of size 100, -- with 10 seconds max idle timeout local ok, err = db:set_keepalive(10000, 100) if not ok then ngx.say("failed to set keepalive: ", err) return end -- or just close the connection right away: -- local ok, err = db:close() -- if not ok then -- ngx.say("failed to close: ", err) -- return -- end

Sorry, I think this place doesn't actively call db:close() should be acceptable,
this link provides agentzh's view
@spacewander

spacewander · 2022-03-16T03:06:42Z

apisix/discovery/tars/init.lua

+
+
+local function get_endpoint(servant)
+    local endpoint_version, err = endpoint_dict:get_stale(servant .. "#version")


Why use get_stale when we don't have an expiration time?

Fitst:
All servant:nodes pairs are from SQL queries and have no expiration time in the cache sense

Next:
When we read the results of fectch_full the logic is as follows:

1. endpoint_dict:flush_all() 2. for (result : results ) { endpoint_dict[servant]=nodes } 3. endpoint_dict:flush_expired()

get_endpoint may happen at 2, we have to use endpoint_dict:get_stale

@spacewander

We don't set any expiration for the shdict's data, right? So why do we need to use get_stale & flush_expired?

We don't set any expiration for the shdict's data, right?

We call endpoint_dict:flush_all() to marks all the existing items as expired before read full query results.

Why:

Consider the following situation: If the current endpoint is as follows: k1:v1, k2:v2, k3:v3 Then we full query get the following results k1:v1, k4:v4, k5:v5 At this time, we need 1: add k4:v4, k5:v5 2: delete k2:v2, k3:v3 flush_all and flush_expired can achieve this

Thanks for your explanation. Let's enrich the comment in the code.

@zhixiongdu027
Please add some comment about the expiration operation, thanks.

Sorry, I add comment in function fetch_full and forget function get_endpoint()

I will update

spacewander · 2022-03-16T03:07:34Z

t/tars/discovery/tars.t

+      database: db_tars
+      user: root
+      password: tars2022
+    full_fetch_interval: 90


Could we use a shorter interval instead of waiting > 90 in the test?

this value Is check by schema ,
can we use an fake schema.lua int test ?

Maybe we can reduce the min in the schema? A sane user may not use a low value.

A sane user may not use a low value.

Yes, I agree with this

But I think the meaning of scheame's existence is to ensure that this value is within a reasonable range,

Is it not appropriate to modify the schema if it is only for testing .

What about rewriting schema in the test?
Like

apisix/t/plugin/skywalking.t

Lines 34 to 35 in 9d450d7

local client = require("skywalking.client")

client.backendTimerDelay = 0.5

ok, I will try

spacewander · 2022-03-16T03:07:42Z

t/tars/discovery/tars.t

+              local core = require("apisix.core")
+              local d = require("apisix.discovery.tars")
+
+              ngx.sleep(11)


zhixiongdu027 · 2022-03-17T03:20:47Z

@spacewander

Hi, I still have some questions to ask, please take a look at the comments I wrote.
thank you very much.

apisix/discovery/tars/init.lua

zhixiongdu027 · 2022-03-19T13:25:08Z

@spacewander @tzssangglass
Hi , I have submitted a new submission,
Please help to review, thank you very much

spacewander · 2022-03-20T12:13:57Z

apisix/discovery/tars/init.lua

+            return err
+        end
+        extract_endpoint(res)
+    end


For style, let's add return nil at the end

spacewander · 2022-03-20T12:16:43Z

apisix/discovery/tars/init.lua

+
+
+local function get_endpoint(servant)
+    local endpoint_version, err = endpoint_dict:get_stale(servant .. "#version")


@zhixiongdu027
Please add some comment about the expiration operation, thanks.

spacewander · 2022-03-20T12:21:58Z

apisix/discovery/tars/init.lua

+        end
+        extract_endpoint(res)
+    end
+    endpoint_dict:flush_expired()


For style, let's add return nil at the end

Please add some comment about the expiration operation, thanks.

Sorry, I add comment in function fetch_full and forget function get_endpoint()

I will update

tzssangglass · 2022-03-21T02:27:06Z

hi @zhixiongdu027 , file conflict, you need to merge master branch to your PR.

zhixiongdu027 · 2022-03-21T04:25:20Z

hi @zhixiongdu027 , file conflict, you need to merge master branch to your PR.

Done

zhixiongdu027 added 3 commits March 13, 2022 16:47

feat: add tars discovery

a7a49d5

feat: add tars discovery

e725066

feat: add tars discovery

ea629e8

spacewander reviewed Mar 13, 2022

View reviewed changes

feat: add tars discovery

4973fd5

zhixiongdu027 requested a review from spacewander March 16, 2022 02:50

spacewander reviewed Mar 16, 2022

View reviewed changes

zhixiongdu027 requested a review from spacewander March 16, 2022 06:42

tzssangglass reviewed Mar 18, 2022

View reviewed changes

zhixiongdu027 requested a review from tzssangglass March 18, 2022 08:45

feat: add tars discovery

b901877

spacewander reviewed Mar 20, 2022

View reviewed changes

zhixiongdu027 added 2 commits March 21, 2022 00:09

feat: add tars discovery

78773f2

feat: add tars discovery

f86fc26

zhixiongdu027 requested a review from spacewander March 21, 2022 02:05

zhixiongdu027 added 2 commits March 21, 2022 12:17

Merge branch 'master' into tars-proxy

f5dea5b

Update ngx_tpl.lua

0980e61

spacewander approved these changes Mar 21, 2022

View reviewed changes

tzssangglass approved these changes Mar 21, 2022

View reviewed changes

spacewander merged commit 5b21d31 into apache:master Mar 21, 2022

zhixiongdu027 mentioned this pull request Sep 20, 2022

test: fix config error in test cases #7946

Merged

5 tasks

	default_weight = local_conf.discovery.tars.default_weight or 100
	default_weight = conf.default_weight

	local function create_endpoint_lrucache(servant)
	local function add_endpoint_to_lrucache(servant)



		local function get_endpoint(servant)
		local endpoint_version = endpoint_dict:get_stale(servant .. "#version")



		local function get_endpoint(servant)
		local endpoint_version, err = endpoint_dict:get_stale(servant .. "#version")

	local client = require("skywalking.client")
	client.backendTimerDelay = 0.5

feat: add tars discovery #6599

feat: add tars discovery #6599

Conversation

zhixiongdu027 commented Mar 13, 2022

What this PR does / why we need it:

Pre-submission checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 commented Mar 16, 2022

zhixiongdu027 commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhixiongdu027 commented Mar 17, 2022

zhixiongdu027 commented Mar 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzssangglass commented Mar 21, 2022

zhixiongdu027 commented Mar 21, 2022

zhixiongdu027 Mar 20, 2022 •

edited

Loading

zhixiongdu027 Mar 20, 2022 •

edited

Loading

zhixiongdu027 Mar 16, 2022 •

edited

Loading

zhixiongdu027 Mar 17, 2022 •

edited

Loading

zhixiongdu027 Mar 17, 2022 •

edited

Loading