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

Add active health checks to TransportServer #1384

Merged
merged 9 commits into from
Feb 25, 2021
Merged

Conversation

LorcanMcVeigh
Copy link
Contributor

Proposed changes

This PR adds active health checks support to Transport Servers for NGINX Plus. It adds a health check field to yaml and fields for better configuring the health checks.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

internal/configs/version2/stream.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver_test.go Outdated Show resolved Hide resolved
docs-web/configuration/transportserver-resource.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Hi @LorcanMcVeigh

Thanks for addressing feedback. I got a few more comments/suggestion related to the new code.

docs-web/configuration/transportserver-resource.md Outdated Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/version2/nginx-plus.transportserver.tmpl Outdated Show resolved Hide resolved
internal/configs/version2/stream.go Show resolved Hide resolved
internal/configs/transportserver_test.go Outdated Show resolved Hide resolved
internal/configs/transportserver_test.go Outdated Show resolved Hide resolved
internal/configs/transportserver_test.go Show resolved Hide resolved
internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver_test.go Outdated Show resolved Hide resolved
@lucacome lucacome added the enhancement Pull requests for new features/feature enhancements label Feb 19, 2021
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Feb 22, 2021
var upstreams []version2.StreamUpstream
var hc *version2.StreamHealthCheck

for _, u := range transportServerEx.TransportServer.Spec.Upstreams {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the health check generation from this method?

At the moment, it appears to create a healthcheck for each upstream, but there is only one pointer. I think the value is overridden so it only returns the healthcheck for the last upstream, is this correct?

How about a new function, which takes the upstreams (transportServerEx.TransportServer.Spec.Upstreams ?) and returns the health checks for each one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, it appears to create a healthcheck for each upstream, but there is only one pointer. I think the value is overridden so it only returns the healthcheck for the last upstream, is this correct?

Yeah that's right, at the moment we don't support multiple health checks, so I'll refactor so that the only health check that's generated is the one that referenced the right upstream

@@ -4,6 +4,7 @@ import (
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned previously about the pointer override, assuming this is not intended behaviour, can we add a test to confirm that two upstreams generates two health checks ? I think there's multiple health checks in the tests below, but some of them are invalid ?

internal/configs/transportserver.go Outdated Show resolved Hide resolved
internal/configs/transportserver_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@soneillf5 soneillf5 left a comment

Choose a reason for hiding this comment

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

Looks great!

@LorcanMcVeigh LorcanMcVeigh merged commit 8e39b6f into master Feb 25, 2021
@LorcanMcVeigh LorcanMcVeigh deleted the health-checks-ts branch February 25, 2021 11:22
@pleshakov pleshakov changed the title Add health checks to transport server Add active health checks to TransportServer Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants