Fix cAdvisor constantly polls data that has been disabled #2900
Conversation
|
/ok-to-test |
|
/retest |
Creatone
force-pushed
the
creatone/disable-metrics
branch
from
2397163
to
aed51d3
Compare
Creatone
force-pushed
the
creatone/disable-metrics
branch
3 times, most recently
from
c552073
to
6260164
Compare
build/integration.sh
Outdated
| @@ -26,7 +26,8 @@ printf "" # Refresh sudo credentials if necessary. | |||
| function start { | |||
| set +e # We want to handle errors if cAdvisor crashes. | |||
| echo ">> starting cAdvisor locally" | |||
| GORACE="halt_on_error=1" ./cadvisor --docker_env_metadata_whitelist=TEST_VAR --v=6 --logtostderr $CADVISOR_ARGS &> "$log_file" | |||
| # This cpu, cpuset, percpu, app, memory, disk, diskIO, network, perf_event metrics should be enabled. | |||
There was a problem hiding this comment.
This -> The ?
Btw. If this PR would depend on PR#2902, it would be enough to change "disable_metrics" for "enable_metrics" option, to document what metrics the testing actually needs.
Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
Creatone
force-pushed
the
creatone/disable-metrics
branch
from
6260164
to
dbf633f
Compare
Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
Creatone
force-pushed
the
creatone/disable-metrics
branch
from
dbf633f
to
de4c2e2
Compare
| @@ -26,7 +26,8 @@ printf "" # Refresh sudo credentials if necessary. | |||
| function start { | |||
| set +e # We want to handle errors if cAdvisor crashes. | |||
| echo ">> starting cAdvisor locally" | |||
| GORACE="halt_on_error=1" ./cadvisor --docker_env_metadata_whitelist=TEST_VAR --v=6 --logtostderr $CADVISOR_ARGS &> "$log_file" | |||
| # This cpuset, percpu, memory, disk, diskIO, network, perf_event metrics should be enabled. | |||
There was a problem hiding this comment.
Now that -enable_metrics is used, this could say e.g. "enable only metrics required by the test", or be even removed.
There was a problem hiding this comment.
In my opinion, that comment is more accurate than "enable only metrics required by the test".
There was a problem hiding this comment.
Comment metrics list just duplicates -enable_metrics argument, so it's redundant / can get out of sync later. Comment is a bit weirdly at the moment too ("This" -> "The"?). But feel free to ignore my comment "bike-shedding", it's not really important, like rest of this PR.
|
Btw. Do integration tests need also "app" and "cpu" metrics? Currently they are always enabled, as those two (are only ones that) are not in the ignoreWhitelist, but that is going to change with PR #2910. |
|
@bobbypage, @iwankgb, @dims, @mrunalp PTAL |
|
Thanks @Creatone for the fix. LGTM! |
Fixes #2897