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

feat(balancer) use lua-resty-dns-client healthThreshold attrib #5206

Merged
merged 1 commit into from
Nov 11, 2019

Conversation

locao
Copy link
Contributor

@locao locao commented Nov 6, 2019

  • added new field threshold to healthchecks in upstreams entities.
  • added new query parameter balancer_health for the endpoint /upstreams/:upstream_id/health. When balancer_health=1, the endpoint returns the balancer health, instead of the targets' health.

kong/runloop/balancer.lua Outdated Show resolved Hide resolved
@Tieske
Copy link
Member

Tieske commented Nov 6, 2019

instead of the query argument, cannot we add the balancer health in the top elelements of the response as shown here: https://docs.konghq.com/1.4.x/admin-api/#show-upstream-health-for-node

{
    "total": 2,
    "node_id": "cbb297c0-14a9-46bc-ad91-1d0ef9b42df9",
    "status": "HEALTHY",                                        --------> this one added
    "data": [
        {
            "created_at": 1485524883980,
            "id": "18c0ad90-f942-4098-88db-bbee3e43b27f",
            "health": "HEALTHY",
            "target": "127.0.0.1:20000",
            "upstream_id": "07131005-ba30-4204-a29f-0927d53257b4",
            "weight": 100
        },
        {
            "created_at": 1485524914883,
            "id": "6c6f34eb-e6c3-4c1f-ac58-4060e5bca890",
            "health": "UNHEALTHY",
            "target": "127.0.0.1:20002",
            "upstream_id": "07131005-ba30-4204-a29f-0927d53257b4",
            "weight": 200
        }
    ]
}

@locao locao force-pushed the feat/balancer_health_threshold branch from 5d7b4ba to 4e136e9 Compare November 6, 2019 16:41
@hishamhm
Copy link
Contributor

hishamhm commented Nov 6, 2019

instead of the query argument, cannot we add the balancer health in the top elelements

@Tieske I discussed this with @locao and I suggested the query argument as a way to add the feature without incurring in a breaking change for the format (and subsequent adjustments to any tooling built around Kong's health info, etc.) and we agreed this was a good way to go.

The rationale was that the configuration and behavior of healthchecks are already pretty complicated as is, and given that the default for health_threshold is 0, most users shouldn't need to deal with three nested levels of health status variables (your JSON example above didn't include the addresses nested array)

@hishamhm
Copy link
Contributor

hishamhm commented Nov 6, 2019

@locao Missing autodoc data for field upstreams.healthchecks.threshold ;)

@locao locao force-pushed the feat/balancer_health_threshold branch 2 times, most recently from 2883547 to 5c006d2 Compare November 7, 2019 15:37
kikito
kikito previously requested changes Nov 7, 2019
kong/api/routes/upstreams.lua Outdated Show resolved Hide resolved
kong/runloop/balancer.lua Outdated Show resolved Hide resolved
@locao locao force-pushed the feat/balancer_health_threshold branch 2 times, most recently from fb8f036 to 767ff41 Compare November 8, 2019 16:18
@locao locao requested a review from kikito November 8, 2019 17:30
@hishamhm
Copy link
Contributor

hishamhm commented Nov 8, 2019

@locao I noticed a small detail before hitting merge: the query-arg for the endpoint needs to be documented in the Admin API autodocs as well (grep for request_query in autodoc/data/admin-api.lua for example on documenting query args — or see #5210 😁 )

added unit test healthchecks.threshold param validation
@locao locao force-pushed the feat/balancer_health_threshold branch from 767ff41 to 655e2fc Compare November 8, 2019 20:21
@locao
Copy link
Contributor Author

locao commented Nov 8, 2019

@locao the query-arg for the endpoint needs to be documented in the Admin API autodocs as well

Good catch @hishamhm, thank you! Docs updated

@hishamhm hishamhm dismissed kikito’s stale review November 8, 2019 20:25

changes addressed

@locao locao merged commit e3a0e0d into next Nov 11, 2019
@locao locao deleted the feat/balancer_health_threshold branch November 11, 2019 16:36
hutchic pushed a commit that referenced this pull request Nov 21, 2019
added unit test healthchecks.threshold param validation
@hishamhm hishamhm added this to the 2.0.0 milestone Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants