-
Notifications
You must be signed in to change notification settings - Fork 31
Jolokia based readiness and liveness probes #170
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing 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 |
@wallrj PR needs rebase |
f68df38
to
dac1734
Compare
/area cassandra |
@wallrj PR needs rebase |
dac1734
to
642475f
Compare
@wallrj PR needs rebase |
642475f
to
1eb1aa3
Compare
f66fd2d
to
af25e99
Compare
* Use the Docker maintained Cassandra 3 image. * Temporarily use TCP connect based readiness and liveness probes because this image doesn't contain a nodetool based `/ready-probe.sh` script. * I found that these TCP connection checks succeed even when the process is SIGSTOPped; so I now simulate a node failure using `nodetool decommission` which makes cassandra stop listening on its CQL port. * Cluster status aware readiness probes will be restored jetstack#170 is merged. Fixes: jetstack#222
Automatic merge from submit-queue. Use the Docker maintained Cassandra 3 image * Use the Docker maintained Cassandra 3 image. * Use TCP connect based readiness and liveness probe because this image doesn't contain a nodetool based `/ready-probe.sh` script. * These TCP connection checks will succeed even though the process is stopped; so I now simulate a node failure using `nodetool decommission` which makes cassandra stop listening on its CQL port. * I can improve on this when #170 is merged. Fixes: #222 **Release note**: ```release-note NONE ```
@wallrj PR needs rebase |
5bb9d33
to
94bf1c5
Compare
94bf1c5
to
ed41f5d
Compare
Host string | ||
ID uuid.UUID | ||
State NodeState | ||
Status NodeStatus |
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.
What's the difference between State and Status? I see they have different values (above), but not sure what each of them mean. A brief comment here would help.
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.
I've added comments in the source code.
} | ||
|
||
type Interface interface { | ||
Status() (NodeMap, error) |
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.
Could we rename Status
to Nodes
? Status is becoming quite an overloaded term in this package.
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 idea, but again, I wanted to try and match nodetool as closely as possible.
This is intended to give a structured version of the nodetool status
output.
Let's leave it as is for now and I can refactor later when I've got a better feeling for what operations we need to perform.
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.
👍
NodeStatusUnknown NodeStatus = "Unknown" | ||
NodeStatusUp NodeStatus = "Up" | ||
NodeStatusDown NodeStatus = "Down" | ||
) |
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.
Will these strings be surfaced via our API (i.e. on a pilot.status field?).
If so, we should earmark them to move into pkg/apis/navigator/v1alpha1
. Not important for merge though as afaict, they are not being used on API types currently.
jolokiaPort, | ||
jolokiaContext, | ||
), | ||
}, |
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 okay for now, but I think this environment variable should be set/managed by Pilot in future. We've got a similar question/issue coming up in the ES controller, as we need to see the heap size (via env var) too.
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.
(line comment went weird - this is specifically around setting the JAVA_OPTS env var)
ed41f5d
to
c8c2dc0
Compare
@wallrj PR needs rebase |
* Load Jolokia agent when Cassandra node starts. * A golang client for getting local Cassandra node status * Linked to the Pilot readiness and liveness health functions.
c8c2dc0
to
a58f226
Compare
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kragniz, munnerz 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 |
/retest |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. |
You can see the source of
nodetool status
, which this replaces, here:Part of #169
Release note: