Hawkular support for the Initial Resources #15476
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
k8s-github-robot
added
the
size/L
|
Labelling this PR as size/L |
| m := make([]metrics.Modifier, len(self.modifiers), 2+len(self.modifiers)) | ||
| copy(m, self.modifiers) | ||
|
|
||
| if namespace != "" { |
There was a problem hiding this comment.
use kapi.NamespaceAll constant
|
Took an initial pass, would also appreciate @piosz input. |
| descriptorTag string = "descriptor_name" | ||
| separator string = "/" | ||
|
|
||
| defaultServiceAccountFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" |
There was a problem hiding this comment.
Is anyone else making this assumption? shouldn't this be a flag?
There was a problem hiding this comment.
Yea, I've seen this assumption elsewhere. I added serviceAccountFile param to override this path.
|
Is there a corresponding PR that discusses how to configure Hawkular with Kubernetes? |
|
I have a concern on this PR: With this PR in, any changes to our monitoring schema in the future might cause the breakage to this integration, which makes test / rollout / upgrade much harder by adding another metric. Can we help with our metric plumbing proposal: https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/metrics-plumbing.md so that we have a standard way to access stats? cc/ @vishh |
|
@dchen1107 is it fair to say that your comment extends to the other initial resources data source types - gcm and influxdb - as well? |
|
@ncdc: Yes. I have raised similar concerns in the past while introducing the other two data sources. My intention is not to derail this PR, but highlight the fact that rolling out any schema changes will be difficult with the current approach. |
|
@burmanm thanks for the change! Overall LGTM but I'm not familiarized with Hawkular at all. I assume @derekwaynecarr and @ncdc reviewed implementation details. |
|
@dchen1107 @vishh |
burmanm
force-pushed
the
hawkular_save
branch
from
5bfa682 to
76a7d10
Compare
|
Squashed the commits and rebased from master. |
|
@burmanm - trying to focus on a way to proceed for this PR. I think its fair to advertise the feature as experimental. As a result, user's need to know how to experiment with the feature to even make it worthwhile to integrate in the code. Can you add an example to |
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
GCE e2e test build/test passed for commit 048f9a25dc9ac93657987fdd9ebbcb3e5a948b48. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test failed for commit 048f9a25dc9ac93657987fdd9ebbcb3e5a948b48. |
|
Trying rebasing and then running hack/update-all.sh, then push that commit and see if the tests pass |
burmanm
force-pushed
the
hawkular_save
branch
from
048f9a2 to
6ec1c4d
Compare
|
PR changed after LGTM, removing LGTM. |
k8s-github-robot
removed
the
lgtm
|
GCE e2e test build/test passed for commit 6ec1c4d3fad9e071a73432994f624864d1197a4c. |
davidopp
added
the
lgtm
burmanm
force-pushed
the
hawkular_save
branch
from
6ec1c4d to
3cfb9ec
Compare
|
PR changed after LGTM, removing LGTM. |
k8s-github-robot
removed
the
lgtm
|
GCE e2e build/test failed for commit 3cfb9ecf16aa63f4cb0765ffa6c8ed522cb77b90. |
burmanm
force-pushed
the
hawkular_save
branch
from
3cfb9ec to
528be97
Compare
|
GCE e2e test build/test passed for commit 528be97. |
|
Now the integration test is failing on something like persistent_volumes_test.go, out of my hands. |
|
@burmanm when something fails in a test and it's not related to code that you changed in your PR, it typically means that it's a test flake - something that passes most of the time, but also sometimes fails. The process in this case is to find an existing issue or create a new one, and then ask the bot to retest, referencing the flake issue. Like this (I found the issue): |
davidopp
added
the
lgtm
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
GCE e2e build/test failed for commit 528be97. |
|
@k8s-bot test this issue: #IGNORE Tests have been pending for 24 hours |
|
GCE e2e test build/test passed for commit 528be97. |
|
GCE e2e test build/test passed for commit 528be97. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 528be97. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
Adds support for using Hawkular as the datasource for initial resources plugin. Follows the configuration of Heapster sink for easier migration to Heapster in the future.
fixes #13873
cc @piosz