Fixed race and added support for all option in docker stats #4671
Conversation
cgtexmex
force-pushed
the
del-boca-vista
branch
from
3e5fa1a
to
7308f25
Compare
|
FYI - The integration tests appear out of order since they are named 1-39 and there's currently no 1-38 -- that's because #4339 has integration tests with the 1-38 prefix... |
|
|
| 2. Run Stats with no-stream which will return stats for any running container | ||
| 3. Run Stats with no-stream all which will return stats for all containers | ||
| 4. Verify the API memory output against govc | ||
| 5. Verify the API CPU output against govc |
There was a problem hiding this comment.
- Run stats with no-stream on a container that doesn't exist
- Run stats with no-stream <running container ID>
- Run stats with no-stream <non-running container ID>
There was a problem hiding this comment.
tests look good, can you add them here to the md as well? I am not seeing them...
| #Expected Outcome: | ||
| 1. Fails if two containers are not created | ||
| 2. Return stats for a single container and validate memory -- will fail if there's too | ||
| much variation in the memroy |
|
|
||
| #Possible Problems: | ||
| Stats are created by the ESXi host every 20s -- if there are long pauses between calls | ||
| in a single test the results could be incorrect and a failure could occur. |
There was a problem hiding this comment.
Is this going to cause intermittent failures in the CI?
| Stats No Stream | ||
| ${rc} ${output}= Run And Return Rc And Output docker %{VCH-PARAMS} stats --no-stream | ||
| Should Be Equal As Integers ${rc} 0 | ||
| ${output}= Get Line ${output} 1 |
There was a problem hiding this comment.
Get line -1 would be safer, as I think you want the last line of the output?
There was a problem hiding this comment.
I want the first container -- which should be the last line of output. I'm using Get Line ${output} 1 because I understand it to return a specific array element by ordinal position. Which in all my tests it does.
How would Get Line -1 work or be safer?
There was a problem hiding this comment.
-1 grabs the last line of output. so in case for some reason there was a hangover container or something it would be safer as it would always grab the last line
| # convert to percent and move decimal | ||
| ${percent}= Evaluate (${vmomiMemory}/2048000)*100 | ||
| ${diff}= Evaluate ${percent}-${vicMemory} | ||
| # due to timing we could see some variation, but shouldn't exceed 5 |
There was a problem hiding this comment.
percent -- comment added
cgtexmex
force-pushed
the
del-boca-vista
branch
2 times, most recently
from
20db5d8
to
a2b5126
Compare
| Stats No Stream | ||
| ${rc} ${output}= Run And Return Rc And Output docker %{VCH-PARAMS} stats --no-stream | ||
| Should Be Equal As Integers ${rc} 0 | ||
| ${output}= Get Line ${output} 1 |
There was a problem hiding this comment.
-1 grabs the last line of output. so in case for some reason there was a hangover container or something it would be safer as it would always grab the last line
| Should Be Equal As Integers ${rc} 0 | ||
| ${stress}= Get Container ShortID %{STRESSED} | ||
| ${rc} ${vmomiCPU}= Run Keyword If '%{HOST_TYPE}' == 'ESXi' Run And Return Rc And Output govc metric.sample -json vm/*${stress} cpu.usagemhz.average | jq -r .Sample[].Value[0].Value[] | ||
| ${rc} ${vmomiCPU}= Run Keyword If '%{HOST_TYPE}' == 'VC' Run And Return Rc And Output govc metric.sample -json */%{VCH-NAME}/*${stress} cpu.usagemhz.average | jq -r .Sample[].Value[0].Value[] |
There was a problem hiding this comment.
can't do this var assignment like this, with esxi it will always get overwritten to null.
There was a problem hiding this comment.
easier to just do something like this:
Run keyword if host_type == ESXi Set test variable ${my_path} vm/*${stress}
Run keyword if host_type == VC Set test variable ${my_path} /%{VCH-NAME}/${stress}
Then just a single run and return command with the ${my_path}
There was a problem hiding this comment.
cool..that's what I thought about doing originally...but then punted...thanks!
| Should Be Equal As Integers ${rc} 0 | ||
| ${stress}= Get Container ShortID %{STRESSED} | ||
| ${rc} ${vmomiMemory}= Run Keyword If '%{HOST_TYPE}' == 'ESXi' Run And Return Rc And Output govc metric.sample -n 1 -json vm/*${stress} mem.active.average | jq -r .Sample[].Value[].Value[0] | ||
| ${rc} ${vmomiMemory}= Run Keyword If '%{HOST_TYPE}' == 'VC' Run And Return Rc And Output govc metric.sample -n 1 -json */%{VCH-NAME}/*${stress} mem.active.average | jq -r .Sample[].Value[].Value[0] |
|
|
||
| #Possible Problems: | ||
| Stats are created by the ESXi host every 20s -- if there are long pauses between calls | ||
| in a single test the results could be incorrect and a failure could occur. |
| 2. Run Stats with no-stream which will return stats for any running container | ||
| 3. Run Stats with no-stream all which will return stats for all containers | ||
| 4. Verify the API memory output against govc | ||
| 5. Verify the API CPU output against govc |
There was a problem hiding this comment.
tests look good, can you add them here to the md as well? I am not seeing them...
cgtexmex
force-pushed
the
del-boca-vista
branch
from
a2b5126
to
16fda6e
Compare
In some cases a race could be encountered when executing docker stats. The race would result in an orphaned go routine that would be a memory leak. In addition if stats --no-stream -all was requested the persona would hang on any stopped containers. Stats now will consider the container state when processing these requests. Finally, the unit testing coverage was increased to just over 90% and integration tests were created. Fixes vmware#4549, vmware#4585, vmware#4421
cgtexmex
force-pushed
the
del-boca-vista
branch
from
16fda6e
to
3a12638
Compare
In some cases a race could be encountered when executing docker
stats. The race would result in an orphaned go routine that
would be a memory leak.
In addition if stats --no-stream -all was requested the persona
would hang on any stopped containers. Stats now will consider
the container state when processing these requests.
Finally, the unit testing coverage was increased to just over
90% and integration tests were created.
Fixes #4549, #4585, #4421