Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

rkt-monitor: bunch of improvements #3093

Merged
merged 1 commit into from Sep 3, 2016

Conversation

mzylowski
Copy link
Contributor

  • Container start time is measured by reading output from app
  • Using rkt stop instead of killing all processes
  • Using rkt gc when container is in exit state
  • Using milliseconds instead of nanoseconds
  • Added test image name to output csv filename
  • Option for build all test images by one command
  • Readme updated
  • Minior fixes

In the context of #3019

@jonboulle
Copy link
Contributor

Looks like gofmt is failing..

@mzylowski
Copy link
Contributor Author

Ehh.. My stupid IDE. Rebased.

acbuild --debug copy build-rkt-1.13.0+git/target/bin/"${1}"-stresser /worker
acbuild --debug set-exec -- /worker
acbuild --debug write --overwrite "${1}"-stresser.aci
acbuildEnd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you still need to trap this in case one of the acbuild steps fails?

Copy link
Contributor Author

@mzylowski mzylowski Aug 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was to use acbuildEnd instead of trap.
In my function we looking for acbuild directory. If it exist we can finish old build by acbuild --debug end. So if there was any unfinished job before running the script we end it immediately and safe build our images.
Did I miss something in my solution? ;]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set -e makes the script exit when an acbuild command fails, and the trap makes the script run acbuild end when the script is about to exit. This means in the event of an error acbuild is able to clean up after itself.

Removing the trap will prevent the script from performing cleanup on error, so I'd highly recommend putting that back.

@jonboulle
Copy link
Contributor

I see a new commit was pushed, but some of my comments still stand :-)

@mzylowski
Copy link
Contributor Author

mzylowski commented Aug 22, 2016

There was some work in progress (it is way there was an extra commit). We probably discover new issue related with lkvm and memory test, but it still need to be investigate. For now I added output message if something goes wrong with stopping the container.

@mzylowski mzylowski force-pushed the mzylowski/rkt-benchmarking branch 3 times, most recently from 3ea985d to 69cd58d Compare August 23, 2016 09:21
@jonboulle
Copy link
Contributor

punting to @dgonyeo when he's available

@mzylowski
Copy link
Contributor Author

@iaguis Maybe you also are interested ?

func main() {
fmt.Print("APP-STARTED!\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and after reading more of this PR it is. Nevermind.

@cgonyeo
Copy link
Member

cgonyeo commented Aug 29, 2016

Great work! Had a couple comments, other than those LGTM.

@mzylowski
Copy link
Contributor Author

@dgonyeo Suggestions are applied now ;]

@mzylowski mzylowski force-pushed the mzylowski/rkt-benchmarking branch 2 times, most recently from dfae1c2 to 313071d Compare August 30, 2016 13:32
@cgonyeo
Copy link
Member

cgonyeo commented Aug 31, 2016

Well TestFetch flaked so I restarted semaphore. All green now, and LGTM.

@lucab
Copy link
Member

lucab commented Sep 1, 2016

@mzylowski this needs a refresh to land, can you please rebase against master?

* Container start time is measured by reading output from app
* Using rkt stop instead of killing all processes
* Using rkt gc when container is in exit state
* Using milliseconds instead of nanoseconds
* Added test image name to output csv filename
* Option for build all test images by one command
* Readme updated
* Minor fixes
@mzylowski
Copy link
Contributor Author

@lucab Done ;]

@lucab lucab merged commit 002e7cd into rkt:master Sep 3, 2016
@mzylowski mzylowski deleted the mzylowski/rkt-benchmarking branch September 6, 2016 08:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants