-
Notifications
You must be signed in to change notification settings - Fork 90
Scrape more information from _cluster/health es end point #130
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
Scrape more information from _cluster/health es end point #130
Conversation
|
requires #122 to be merged in first |
9d7a991 to
66fcdd1
Compare
|
/test e2e-aws |
|
@jcantrill should i also update the manifest to expose these status fields for the console? |
| if allocationString, ok := value.(string); ok { | ||
| allocation = allocationString | ||
| } | ||
| allocation := parseString("transient.cluster.routing.allocation.enable", payload.ResponseBody) |
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.
Any reason not to reuse an existing library over writing our own?
Ref:
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.
not really... less to vendor in for a single file?
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.
If we are comfortable it won't come back to bite us and we can support given our needs, its probably fine.
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.
we can make it a tech debt item to investigate further
338c925 to
aa7d457
Compare
richm
left 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.
lgtm - @jcantrill
6edf08b to
9b40d05
Compare
portante
left 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.
Looks good; not sure I understand all the reason for the function renames.
controller-manifests/elasticsearch-operator.v4.2.0.clusterserviceversion.yaml
Show resolved
Hide resolved
controller-manifests/elasticsearch-operator.v4.2.0.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
| path: nodeSpec.resources | ||
| x-descriptors: | ||
| - 'urn:alm:descriptor:com.tectonic.ui:resourceRequirements' | ||
| statusDescriptors: |
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.
Seems like you have a mix of partial Elasticsearch cluster health data with other kube level stats. Perhaps any way to group these in the file so that in the future folks know that the ES cluster health stats are together?
And are you going to add the rest of them?
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 simply information that gets exposed at the console level. this will configure it to know where to scrape it from on the CR and what to label it as.
4bf3cc0 to
a423daf
Compare
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.annotations['olm.targetNamespaces'] | ||
| - name: POD_NAME |
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 isn't necessary for 4.1.0 -- required for 4.2 as part of the sdk bump
a423daf to
6529784
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ewolinetz, josefkarasek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Addresses https://jira.coreos.com/browse/LOG-377
compared to: