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

tests(hybrid): enable cluster sync in test case 11-status_spec.lua #13985

Closed
wants to merge 10 commits into from

Conversation

chobits
Copy link
Contributor

@chobits chobits commented Dec 6, 2024

Summary

Currently, we are verifying that the /ready/status endpoint with an empty config in cluster sync mode returns a 503.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix KAG-5974

@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Dec 6, 2024
@chronolaw chronolaw changed the title test(hybrid): enable incremental sync test in 11-status_spec.lua tests(hybrid): enable incremental sync test in 11-status_spec.lua Dec 6, 2024
@@ -119,7 +116,10 @@ for _, strategy in helpers.each_strategy() do

local status = res and res.status
http_client:close()
if status == 200 then

if (inc_sync == "on" and status == 503) or
Copy link
Contributor

Choose a reason for hiding this comment

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

It could work but may not the best solution, since the case title is "should return 200 on data plane after configuring", and the status code is 503 when enabling inc_sync.

@@ -138,7 +138,9 @@ for _, strategy in helpers.each_strategy() do

local status = res and res.status
http_client:close()
if status == 200 then
if (inc_sync == "on" and status == 503) or
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@chobits chobits Dec 9, 2024

Choose a reason for hiding this comment

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

before inserting an entity, it will return 503.
After inserting an entity, which makes the configuration non-empty, it will return 200.

The inserting-an-entity case is not added to this case currently.

@chobits
Copy link
Contributor Author

chobits commented Dec 9, 2024

It also needs to add a test case that, when we insert a service, the DP status will become ready, like the following, but after CP restarts, inserted entity could not notify DP with RPC concentrator since there's a concentrator connection timeout bug: https://konghq.atlassian.net/browse/KAG-5994

         end, 10)

+        -- insert one entity to make dp ready for incremental sync
+        if inc_sync == "on" then
+          ngx.sleep(1)
+          print("+++++ post /services")
+          local admin_client = helpers.admin_client(10000)
+          local res = assert(admin_client:post("/services", {
+            body = { name = "service-001", url = "https://127.0.0.1:15556/request", },
+            headers = {["Content-Type"] = "application/json"}
+          }))
+          assert.res_status(201, res)
+          admin_client:close()
+
+          print("++++++= sleep")
+          --ngx.sleep(1000000)
+          helpers.wait_until(function()
+            local http_client = helpers.http_client('127.0.0.1', dp_status_port)
+
+            local res = http_client:send({
+              method = "GET",
+              path = "/status/ready",
+            })
+
+            local status = res and res.status
+            http_client:close()
+--            print("++++++++++" .. status)
+
+            if status == 200 then
+              return true
+            end
+          end, 10)
+        end
       end)
     end)

@chobits chobits marked this pull request as draft December 9, 2024 07:44
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 11, 2024
@chobits chobits changed the title tests(hybrid): enable incremental sync test in 11-status_spec.lua tests(hybrid): enable cluster sync in test case 11-status_spec.lua Dec 17, 2024
@chobits
Copy link
Contributor Author

chobits commented Dec 25, 2024

closed, @lhanjian has another pr to fix this

@chobits chobits closed this Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants