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

Pass through service spec values #271

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

willmostly
Copy link
Contributor

@willmostly willmostly commented Dec 3, 2024

Kubernetes supports several types of services. This change passes through arbitrary keys under service.spec, which allows specifying type-specific keys such as nodePort. The port and targetPort are autoconfigured to match the port specified in the gateway http-server configuration, and the selector is autoconfigured to match the deployment.

Suggested release note:

  • Set custom service attributes. Breaking change: the service node no longer uses type and port as a subfields. Define the service.spec instead.

@cla-bot cla-bot bot added the cla-signed label Dec 3, 2024
@willmostly willmostly force-pushed the will/arbitrary-service branch from 08aa1cd to aa770ff Compare December 3, 2024 21:42
@nineinchnick nineinchnick added the enhancement New feature or request label Dec 4, 2024
charts/gateway/values.yaml Outdated Show resolved Hide resolved
charts/gateway/templates/service.yaml Outdated Show resolved Hide resolved
charts/gateway/values.yaml Outdated Show resolved Hide resolved
charts/gateway/values.yaml Outdated Show resolved Hide resolved
@willmostly willmostly force-pushed the will/arbitrary-service branch 4 times, most recently from 10895a4 to 3ef3a48 Compare December 11, 2024 22:44
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

I like adding these new test suites! Just a bunch of nitpicks remaining.

charts/gateway/values.yaml Outdated Show resolved Hide resolved
charts/gateway/values.yaml Outdated Show resolved Hide resolved
tests/gateway/test-https.yaml Show resolved Hide resolved
tests/gateway/test-nodeport.yaml Outdated Show resolved Hide resolved
tests/gateway/test-nodeport.yaml Outdated Show resolved Hide resolved
@willmostly willmostly force-pushed the will/arbitrary-service branch from 2019727 to f747b79 Compare December 13, 2024 21:12
charts/gateway/values.yaml Outdated Show resolved Hide resolved
charts/gateway/templates/tests/test-nodeport.yaml Outdated Show resolved Hide resolved
charts/gateway/templates/tests/test-connection.yaml Outdated Show resolved Hide resolved
charts/gateway/templates/deployment.yaml Outdated Show resolved Hide resolved
tests/gateway/test-nodeport.yaml Outdated Show resolved Hide resolved
@willmostly willmostly force-pushed the will/arbitrary-service branch 2 times, most recently from 2020597 to 36b728f Compare December 18, 2024 06:09
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

Almost there, just two nitpicks about testing.

charts/gateway/templates/tests/test-connection.yaml Outdated Show resolved Hide resolved
tests/gateway/test-nodeport.yaml Show resolved Hide resolved
@nineinchnick
Copy link
Member

@willmostly are you going to continue working on this?

@willmostly
Copy link
Contributor Author

@willmostly are you going to continue working on this?

Yes just getting back into the swing of things...

@willmostly willmostly force-pushed the will/arbitrary-service branch from 36b728f to 5d7c71c Compare January 10, 2025 03:06
@willmostly
Copy link
Contributor Author

Tests are succeeding individually but intermittently failing when run as a group. Looks like there is some flakiness

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

Almost there

charts/gateway/values.yaml Outdated Show resolved Hide resolved
charts/gateway/values.yaml Outdated Show resolved Hide resolved
charts/gateway/README.md Outdated Show resolved Hide resolved
charts/gateway/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/gateway/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/gateway/templates/deployment.yaml Outdated Show resolved Hide resolved
@nineinchnick
Copy link
Member

Tests are succeeding individually but intermittently failing when run as a group. Looks like there is some flakiness

I see the https test failed, but it timed out on the 8080 port, and the values have this: http-server.http.enabled: false

@willmostly willmostly force-pushed the will/arbitrary-service branch from 687c6ca to 0cafc91 Compare January 10, 2025 15:37
@willmostly
Copy link
Contributor Author

Tests are succeeding individually but intermittently failing when run as a group. Looks like there is some flakiness

I see the https test failed, but it timed out on the 8080 port, and the values have this: http-server.http.enabled: false

The kubectl logs command is printing logs for containers in previous releases, I added --prefix=true because it is confusing for debugging

@willmostly willmostly force-pushed the will/arbitrary-service branch 2 times, most recently from b5c5d21 to f88f47f Compare January 10, 2025 23:02
@willmostly
Copy link
Contributor Author

connection-test has been updated to retry connections up to 20 times to prevent flaky failures. Some key observations while debugging were that

  • All tests succeed when run individually
  • All tests failed at various times during the first run except for the first in the sequence, complete_values
  • When a connection times out, there is no corresponding entry in the gateway's http-request.log
  • Only connections through the internal service time out, nodePort does not
  • The nodeport test could successfully connect to 30443 and 30080 at NODE_IP, and https://trino-gateway:443, then timeout to http://trino-gateway:8080

With all this in mind, I suspect some network issues in kind

charts/gateway/README.md Outdated Show resolved Hide resolved
@willmostly
Copy link
Contributor Author

willmostly commented Jan 13, 2025

The root of my test failures was DNS caching. In the course of making these improvements the service name was changed from {{ include "trino-gateway.fullname" . }} to trino-gateway. Since each test case creates a new deployment, the service was recreated with the same name, but is assigned a different IP address, and CoreDNS would sometimes return the old IP. The extended retries gave the cache a chance to update. This is fixed by making the service name settable and including the test case name in the service name to ensure it is not reused.

Support  healthchecks for gateway configurations without http enabled.

Add tests for https and NodePort service with both https and http exposed.

Add retries to conection tests to improve test resiliency

Co-authored-by: Jan Waś <[email protected]>
@willmostly willmostly force-pushed the will/arbitrary-service branch from f88f47f to 0f7cbd4 Compare January 13, 2025 19:50
@nineinchnick nineinchnick merged commit d6291d1 into trinodb:main Jan 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants