-
Notifications
You must be signed in to change notification settings - Fork 332
Helm chart redesign for Quarkus-based runtimes #626
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
Conversation
09657d1 to
f66d623
Compare
0ac047b to
434a847
Compare
68cbdfc to
acee0c9
Compare
helm/polaris/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is automatically generated by helm-docs.
.github/workflows/helm.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With https://github.com/apache/polaris/pull/912/files, we will need only the original command without filter out the warning messages due to symbolic link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I'm just waiting for #912 to be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#912 is merged! I removed the workaround.
helm/polaris/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above if we should merge https://github.com/apache/polaris/pull/912/files first then remove the filter part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EclipseLink uses a default of 32 connections for connection pooling. For local testing, this is more than enough. If people want to use it for some load testing, this will suffer when there are many concurrent client connection. Do you think we may want to add an example or add the default value here? So for people who may not be familiar with EclipseLink, they can just change the max value etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I added the following:
<property name="eclipselink.connection-pool.default.initial" value="1" />
<property name="eclipselink.connection-pool.default.min" value="1" />
<property name="eclipselink.connection-pool.default.max" value="1" />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits: extra space between "list $k"
helm/polaris/values.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a similar comment here for the file listing issue (#907)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done
4d36036 to
ac643dc
Compare
|
cc @gh-yzou |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that mean by default we will have a connection pool with size 1? maybe we should mention this in the read me, and along with some instruction about how to adjust the size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only for CI tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does the private key and public key coming from ? will we be able to generate it on fly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just for tests. They were randomly generated and do not contain any sensitive information. I will add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that two ports one for icebergRestCatalog service, and another one for OAuth service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is just for tests and contains "bogus" data that does not necessarily reflect how extraServices will be used in real life, because in these tests we cannot create load balancers, ingresses, etc.
The typical use case for extraServices is:
- User wants port 8181 to be exposed to internal clients (in the same namespace) using a
ClusterIPservice. - User also wants to expose port 8181 using a
LoadBalancerservice, for consumption by external clients (potentially with TLS termination, etc.)
This can be achieved by simply leaving service and managementService untouched, and adding the following config:
extraServices:
- nameSuffix: "ext"
type: LoadBalancer
ports:
- name: polaris-http
port: 8181Assuming the release name is my-release, the above will generate the following services:
my-releaseof type ClusterIP for port 8181;my-release-extof type LoadBalancer for port 8181;my-release-mgmtservice of type ClusterIP (headless) for port 8182.
|
@MonkeyCanCode @gh-yzou while answering a question from @gh-yzou I realized that I forgot to set |
|
@MonkeyCanCode @gh-yzou should we move forward with this? I would suggest to merge unless you see any blockers, then we can always keep improving the chart after the merge. It's not like we are going to do a release of the chart any time soon 😄 |
|
@adutra i don't have other further questions, but i am not familiar with this part, so I will let @MonkeyCanCode and @collado-mike to do the final approval on this. You don't have to wait for me to approve this. |
Thought I clicked on approved earlier. Approved. Very well done. |
|
Heads up: I am going to merge this now to unblock people looking for node port support, see #982. |
This is a major redesign of the Helm chart to make it compliant with Quarkus, but also improving a lot of aspects of the existing chart, such as the persistence secret management and the bootstrap jobs, to name a few.
Summary of changes
Persistence
Thanks to the improvement brought by #613, it's not necessary anymore to have an init container create a conf.jar. From now on, the user provides a secret with their persistence.xml, and that file is mounted on each Polaris pod directly.
Enhanced services
The services for Polaris now are separated in 3 categories:
Each service section has the same structure and allows to configure one or more ports.
Observability
New sections were added for logging, tracing, metrics, with also the ability to create a
ServiceMonitor.Bootstrap Job
The
bootstrapsection now configures the bootstrap job. Thanks to the Polaris Admin Tool introduced in #605, the jobs now use the tool to bootstrap realms.I am not convinced personally that this bootstrap job has a huge value, compared to just running the admin tool directly. But I didn't want to remove it.
Advanced Config
A new
advancedConfigsection can be used to customize Polaris deployments in any possible way, even when the Helm chart does not expose the desired setting.