Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Require explicit call to async Expand::machine_id() #1468

Merged
merged 20 commits into from
Dec 2, 2021

Conversation

hayleycall
Copy link
Contributor

@hayleycall hayleycall commented Nov 19, 2021

Summary of the Pull Request

This PR address an issue with a panic occurring due to nested Tokio runtimes when expanding {machine_id} to machine identifier. This issue is documented in #1351. The fix was to update the Expand API by modifying the function machine_id that got and set the machine identifier by making it async so that it would not need to use another Tokio runtime. Now this function runs at the time Expand is constructed rather than at the time of expanding the particular {machine_id} value when expanding each value in target_options.

PR Checklist

  • Applies to work item: Expanding machine_id crashes OneFuzz agent #1351
  • CLA signed. If not, go over here and sign the CLI.
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

Modified machine_id function, called this function at appropriate constructions of Expand. Closes #1351

Validation Steps Performed

Modified test_expand_machine_id to be async to properly test that modified function expands {machine_id} without panic.

@ranweiler ranweiler changed the title Update machine_id function to be async. Call this function in Expand constructors in tasks. Require explicit call to async Expand::machine_id() Nov 19, 2021
Copy link
Member

@ranweiler ranweiler left a comment

Choose a reason for hiding this comment

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

Looking good! Added a few documentation suggestions.

Also, two remaining questions:

  • Are there uses of Expand elsewhere that also need to be updated, such as in the onefuzz library crate?
  • Have we manually tested this with a job that tries to expand {machine_id}?

src/agent/onefuzz/src/expand.rs Show resolved Hide resolved
src/agent/onefuzz/src/expand.rs Show resolved Hide resolved
hayleycall and others added 3 commits November 19, 2021 12:01
Add comment to indicate proper use of machine_id function

Co-authored-by: Joe Ranweiler <joe@lemma.co>
Add comment to indicate proper use of machine_id function

Co-authored-by: Joe Ranweiler <joe@lemma.co>
@hayleycall hayleycall merged commit e4e4671 into microsoft:main Dec 2, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expanding machine_id crashes OneFuzz agent
4 participants