Skip to content
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

Add Close/WaitCtx to UtilityVM & System #1876

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Aug 22, 2023

Add CloseCtx and WaitCtx methods to UtilityVM and System, which accept a context parameter and return if the context is canceled.

This is intended to allow benchmark iterations to time out and prevent them from spending the majority of their time waiting.

However, the added benefit is that tracing information (trace and span ID) will now be passed along to the Wait and Close logs (and underlying HCS call spans).

Additionally, fix a bug in (*UtilityVM).Close, where, if the uVM was created but not started, then the (*UtilityVM).Wait call will
hang indefinitely.
Fix is to wait initially on the system to close, close the IO output
handler, and then wait on the uVM.

Combine LCOW uVM benchmarks together (similar to LCOW container) to simplify benchmark name formatting.

@helsaawy helsaawy marked this pull request as ready for review September 6, 2023 15:44
@helsaawy helsaawy requested a review from a team as a code owner September 6, 2023 15:44
internal/uvm/wait.go Outdated Show resolved Hide resolved
@@ -18,3 +21,26 @@ func CleanName(n string) string {
}
return strings.TrimSpace(strings.Map(mapper, n))
}

// RandNameSuffix concants the provided paramters and appends a random 4 byte sequence as as hex string.
Copy link
Contributor

Choose a reason for hiding this comment

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

nits:
*concats,
as as,
*This is to ensure

Copy link
Contributor

@ambarve ambarve left a comment

Choose a reason for hiding this comment

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

Minor comments: otherwise LGTM!

Add `CloseCtx` and `WaitCtx` methods to `UtilityVM` and `System`, which
accept a context parameter and return if the context is canceled.

This is intended to allow benchmark iterations to time out and prevent
them from spending the majority of their time waiting.

However, the added benefit is that tracing information (trace and span
ID) will now be passed along to the `Wait` and `Close` logs (and
underlying HCS call spans).

Additionally, fix a bug in `(*UtilityVM).Close`, where, if the uVM was
created but not started, then the `(*UtilityVM).Wait` call will
hang indefinitely.
Fix is to wait initially on the system to close, close the IO output
handler, and then wait on the uVM.

Combine LCOW uVM benchmarks together (similar to LCOW container) to
simplify benchmark name formatting.

Relies on microsoft#1875

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy helsaawy merged commit a02b3b2 into microsoft:main Oct 24, 2023
15 of 16 checks passed
@helsaawy helsaawy deleted the uvm-wait-context branch October 24, 2023 16:19
darracott pushed a commit to KenGordon/hcsshim that referenced this pull request Oct 30, 2023
* Add `Close`/`WaitCtx` to `UtilityVM` & `System`

Add `CloseCtx` and `WaitCtx` methods to `UtilityVM` and `System`, which
accept a context parameter and return if the context is canceled.

This is intended to allow benchmark iterations to time out and prevent
them from spending the majority of their time waiting.

However, the added benefit is that tracing information (trace and span
ID) will now be passed along to the `Wait` and `Close` logs (and
underlying HCS call spans).

Additionally, fix a bug in `(*UtilityVM).Close`, where, if the uVM was
created but not started, then the `(*UtilityVM).Wait` call will
hang indefinitely.
Fix is to wait initially on the system to close, close the IO output
handler, and then wait on the uVM.

Combine LCOW uVM benchmarks together (similar to LCOW container) to
simplify benchmark name formatting.

Relies on microsoft#1875

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

* PR: uvm.Wait err handling; doc comments

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

---------

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Signed-off-by: Joe Powell <joepowell@microsoft.com>
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
* Add `Close`/`WaitCtx` to `UtilityVM` & `System`

Add `CloseCtx` and `WaitCtx` methods to `UtilityVM` and `System`, which
accept a context parameter and return if the context is canceled.

This is intended to allow benchmark iterations to time out and prevent
them from spending the majority of their time waiting.

However, the added benefit is that tracing information (trace and span
ID) will now be passed along to the `Wait` and `Close` logs (and
underlying HCS call spans).

Additionally, fix a bug in `(*UtilityVM).Close`, where, if the uVM was
created but not started, then the `(*UtilityVM).Wait` call will
hang indefinitely.
Fix is to wait initially on the system to close, close the IO output
handler, and then wait on the uVM.

Combine LCOW uVM benchmarks together (similar to LCOW container) to
simplify benchmark name formatting.

Relies on microsoft#1875

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

* PR: uvm.Wait err handling; doc comments

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

---------

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants