Add kubernetes-cassandra seed provider #245
Conversation
jetstack-bot
added
size/M
release-note
and removed
do-not-merge/release-note-label-needed
labels
|
Based on this comment, support for configuring the seed provider via enviroment variables isn't going to happen in library/cassandra. Currently I configure this by doing a find+replace on the config file in the pilot, but this feels pretty hacky. Where should similar config tweaks go? |
There was a problem hiding this comment.
Thanks @kragniz
- I can't tell why the e2e tests have failed.
- We don't seem to have dumped the container log files.
- But I suspect it might be that cassandra is crashing on startup.
I left one or two comments below. Please address or answer those before merging.
Dockerfile.pilot-cassandra
Outdated
| @@ -2,6 +2,11 @@ FROM alpine:3.6 | |||
|
|
|||
| ADD http://search.maven.org/remotecontent?filepath=org/jolokia/jolokia-jvm/1.4.0/jolokia-jvm-1.4.0-agent.jar /jolokia.jar | |||
| RUN chmod a+r /jolokia.jar && touch /jolokia.jar | |||
|
|
|||
| # note: temporarily hosted on gist until we find a better place to put it | |||
| ADD https://gist.github.com/kragniz/2e03465746a26e2391773ee4f760fcc1/raw/0e31706c35824f2019f4d3892d1a499c8ad6db61/kubernetes-cassandra.jar /kubernetes-cassandra.jar | |||
There was a problem hiding this comment.
Were there any changes to the Java source code in the end?
If so, we should put the code in a repo. Either:
- make a branch and a PR for https://github.com/kubernetes/examples and add the re-compiled jar there.
- or create a new Jetstack repo.
If we haven't made any changes to the source code,can we download the jar file from https://github.com/kubernetes/examples/raw/master/cassandra/image/files/kubernetes-cassandra.jar instead?
There was a problem hiding this comment.
No modifications were needed, I've changed it to pull from upstream
pkg/pilot/cassandra/v3/pilot.go
Outdated
| @@ -39,6 +41,22 @@ func NewPilot(opts *PilotOptions) (*Pilot, error) { | |||
| nodeTool: opts.nodeTool, | |||
| } | |||
|
|
|||
| // hack to test the seedprovider works in k8s, this should be moved somewhere better | |||
There was a problem hiding this comment.
Create an issue for this and link to it below the comment.
|
|
||
| newContents := strings.Replace(string(read), | ||
| "org.apache.cassandra.locator.SimpleSeedProvider", | ||
| "io.k8s.cassandra.KubernetesSeedProvider", -1) |
There was a problem hiding this comment.
What if the config file doesn't contain org.apache.cassandra.locator.SimpleSeedProvider ?
Might be better to use yaml tools to set this variable even if it doesn't already exist.
There was a problem hiding this comment.
This is a good idea, otherwise we're relying on the config file from the cassandra container being in a certain state when we start making these modifications. Maybe we should address this in #251, though?
There was a problem hiding this comment.
Maybe we should address this in #251, though?
That's fine by me.
kragniz
force-pushed
the
k8s-seed-provider
branch
from
84fbc50
to
bb296ef
Compare
This adds and configures the seed provider from kubernetes/examples.
kragniz
force-pushed
the
k8s-seed-provider
branch
from
bb296ef
to
185eb8b
Compare
kragniz
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wallrj The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
What this PR does / why we need it:
This adds and configures the seed provider from kubernetes/examples.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #223Special notes for your reviewer:
Release note: