Skip to content

Conversation

@bduffany
Copy link
Contributor

@bduffany bduffany commented Jul 1, 2023

  • Pull in upstream swagger definitions for SnapshotLoadParams and MemoryBackend
  • Run go generate
  • Update Snapshot config to allow setting MemBackend
  • Forward the MemBackend through to the Firecracker client

@bduffany bduffany requested a review from a team as a code owner July 1, 2023 16:47
@bduffany bduffany force-pushed the support-uffd-backend branch from b4c1912 to fbc3f74 Compare July 1, 2023 16:53
@bduffany bduffany marked this pull request as draft July 1, 2023 18:43
@bduffany bduffany force-pushed the support-uffd-backend branch 2 times, most recently from 63818b6 to 3a80903 Compare July 1, 2023 19:24
- Pull in upstream swagger definitions for SnapshotLoadParams and MemoryBackend
- Run `go generate`
- Update Snapshot config to allow setting MemBackend
- Forward the MemBackend through to the Firecracker client

Signed-off-by: Brandon Duffany <[email protected]>
@bduffany bduffany force-pushed the support-uffd-backend branch from 3a80903 to 62f8e17 Compare July 2, 2023 17:31
@bduffany bduffany marked this pull request as ready for review July 20, 2023 18:43
@bduffany
Copy link
Contributor Author

bduffany commented Jul 21, 2023

Friendly ping @kzys - we have gotten this patch working successfully over in https://github.com/buildbuddy-io/buildbuddy. I'm not sure why the CI tests are failing though - the failed test doesn't seem to show any output.

Copy link
Contributor

@fangn2 fangn2 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

Overall looks good to me, I left some comments.

Regarding CI failure, rebase main should resolve it(had some changes on CI recently).

Copy link
Contributor

@fangn2 fangn2 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

Overall looks good to me, I left some comments.

Regarding CI failure, rebase main should resolve it(had some changes on CI recently).

@bduffany bduffany force-pushed the support-uffd-backend branch from e5334b9 to 7b904b1 Compare July 28, 2023 23:28
Signed-off-by: Brandon Duffany <[email protected]>
@bduffany
Copy link
Contributor Author

bduffany commented Jul 29, 2023

I went ahead and bumped the firecracker version to v1.4.0 since MemoryBackend is not supported in v1.0.0. I am not sure if the version bump will break anything else, though.

Copy link
Contributor

@fangn2 fangn2 left a comment

Choose a reason for hiding this comment

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

CI is failing due to "go: command not found".

Seems to me your fork is out of sync somehow, at least PR #498 is not in your fork.

Signed-off-by: Brandon Duffany <[email protected]>
Copy link
Contributor Author

@bduffany bduffany left a comment

Choose a reason for hiding this comment

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

CI is failing due to "go: command not found".

I merged main into this branch - hopefully that fixes CI. Let me know if I should have rebased, or if there's anything else I should do to clean up the commits. (I assumed you will squash commits when merging into main.)

@fangn2
Copy link
Contributor

fangn2 commented Aug 3, 2023

I merged main into this branch - hopefully that fixes CI. Let me know if I should have rebased, or if there's anything else I should do to clean up the commits. (I assumed you will squash commits when merging into main.)

CI is working as expected.
Test TestLoadSnapshot is failing though.

Signed-off-by: Brandon Duffany <[email protected]>
@bduffany
Copy link
Contributor Author

bduffany commented Aug 5, 2023

Test TestLoadSnapshot is failing though.

Fixed with c51a40b - this assertion was unnecessary, I think.

@fangn2 fangn2 merged commit 2e09014 into firecracker-microvm:main Aug 7, 2023
@swagatbora90
Copy link
Contributor

Please clean up the commits before merging. I think we should keep the commit to upgrade firecracker ver to v1.4.0 as is and squash the rest

@swagatbora90
Copy link
Contributor

nvm, looks like its already merged

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