Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(core) using new events library #8715

Closed
wants to merge 225 commits into from
Closed

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Apr 22, 2022

Summary

Now we have a new event libaray, let's try to introduce it to replace lua-resty-worker-events.

This PR is closed, we have an equivalent PR #8890, please check it.

See also:

https://github.com/Kong/lua-resty-events
Kong/kong-build-tools#460

Full changelog

  • Modify init_worker_events in global.lua, switch to new events library.
  • Modify templates/nginx_kong.lua, add unix listening
  • Modify templates/kong_defaults.lua, add config legacy_worker_events
  • Add a temporary kong/resty/healthcheck.lua to use resty.events.compat
  • Modify test cases to pass ci, wrap them with wait_until()

Notice

  • Temporary kong/resty/healthcheck.lua should be replaced by a formal version.
  • Flaky test 02-integration/05-proxy/10-balancer/01-healthchecks_spec.lua now is fixed.

@chronolaw chronolaw added this to the 3.0 milestone May 12, 2022
@@ -31,7 +31,7 @@ jobs:
with:
repository: Kong/kong-build-tools
path: kong-build-tools
ref: master
ref: feat/new_resty_events
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will almost certainly have to be changed before we can merge it, so leaving this comment as a reminder.

.requirements Outdated
@@ -7,6 +7,7 @@ RESTY_LUAROCKS_VERSION=3.8.0
RESTY_OPENSSL_VERSION=1.1.1n
RESTY_PCRE_VERSION=8.45
RESTY_LMDB_VERSION=master
RESTY_EVENTS_VERSION=dev
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: we'll have to make a resty events release and change this before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it is main

@@ -484,6 +484,9 @@ build = {
["kong.plugins.azure-functions.handler"] = "kong/plugins/azure-functions/handler.lua",
["kong.plugins.azure-functions.schema"] = "kong/plugins/azure-functions/schema.lua",

-- XXX: test only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, remove before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Vini is working on new version health check library.

lmdb_environment_path = { typ = "string" },
lmdb_map_size = { typ = "string" },

opentelemetry_tracing = { typ = "array" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that opentelemetry_tracing_* was included by mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it just for solving the merge conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8890 fix it.

wait_max = 0.5, -- max wait time before discarding event
}

worker_events = require "resty.worker.events"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather have the two requires at the top of the file, even if that means having to use some temp var names here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worker_events will not work correctly if required at the top of the file, It must be required right here, inside the init function

Comment on lines +186 to +187
opentelemetry_tracing = off
opentelemetry_tracing_sampling_rate = 1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should opentelemetry_tracing* be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for solving the merge conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8890 fix it.

@@ -351,7 +352,8 @@ describe("kong start/stop #" .. strategy, function()
path = "/hello",
})
assert.res_status(404, res) -- no Route configured
assert(helpers.stop_kong(helpers.test_conf.prefix))
--assert(helpers.stop_kong(helpers.test_conf.prefix))
assert(helpers.kong_exec("quit --prefix " .. helpers.test_conf.prefix))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should change stop_kong using quit.

Likely a bit slower, but it might help with flakyness (less dangling requests).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe in another PR, not this?

@@ -129,6 +129,7 @@ for _, strategy in helpers.each_strategy() do
cluster_control_plane = "127.0.0.1:9005",
proxy_listen = "0.0.0.0:9002",
}))
ngx.sleep(0.2) -- wait for 'pids/nginx.pid'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be done with a wait_until that checks that the pid file exists.

@@ -43,7 +43,8 @@ describe("Proxy interface listeners", function()
proxy_listen = "off",
admin_listen = "0.0.0.0:9001",
}))
assert.equals(1, count_server_blocks(helpers.test_conf.nginx_kong_conf))
assert.equals(2, count_server_blocks(helpers.test_conf.nginx_kong_conf))
--assert.equals(1, count_server_blocks(helpers.test_conf.nginx_kong_conf))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the old lines as comments doesn't make it more clear to me, I suggest removing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -212,6 +212,8 @@ for _, strategy in helpers.each_strategy() do
nginx_http_proxy_ssl_trusted_certificate = "../spec/fixtures/kong_spec.crt",
})

ngx.sleep(0.01)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some time to wait for event broker/client to establish unix domain socket connect.

@chronolaw
Copy link
Contributor Author

The master branch has too many changes, it is hard to rebase to this PR.

I create a new PR #8890, let's talk there.

@chronolaw chronolaw closed this Jun 2, 2022
@mayocream mayocream removed this from the 3.0 milestone Jun 7, 2022
@hbagdi hbagdi deleted the feat/new_events branch October 26, 2022 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Not part of the core functionality of kong, but still needed core/balancer core/configuration core/templates size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants