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.

Sign up for GitHub

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

Jump to bottom

neonvm: Add new neonvm-daemon binary skeleton #1090

Merged
merged 7 commits into from
Oct 14, 2024
Merged

neonvm: Add new neonvm-daemon binary skeleton #1090

merged 7 commits into from
Oct 14, 2024

Conversation

sharnoff
Copy link
Member

Extracted the minimal version from #1052, because it's clear that this daemon is required in multiple places, and we don't know which PR will merge first.

Notes: Not expected to pass CI! (images won't be in the right place). Haven't tested it since extracting from #1052.

cc @mikhail-sakhnov, as discussed :)

@sharnoff sharnoff changed the title neonvm: Add new skeleton neonvm-daemon binary neonvm: Add new neonvm-daemon binary skeleton Sep 26, 2024
This was referenced Sep 27, 2024
Closed
Closed
@mikhail-sakhnov
Copy link
Contributor

Fixed CI. Let's merge it and I use it in the #1082

@mikhail-sakhnov mikhail-sakhnov force-pushed the sharnoff/neonvm-daemon branch 2 times, most recently from 1551aa2 to 2dbf278 Compare September 30, 2024 10:52
@sharnoff sharnoff marked this pull request as ready for review September 30, 2024 21:16
@sharnoff sharnoff marked this pull request as draft September 30, 2024 21:19
@sharnoff
Copy link
Member Author

sharnoff commented Sep 30, 2024 •
edited
Loading

Looks like my original commit accidentally reverted some of the fixes from #1088 (and also has a bunch of cgroups-related stuff from #1052). Fixing that...

edit: Also it seems like CI still won't work correctly because we have racing image building?

@sharnoff sharnoff force-pushed the sharnoff/neonvm-daemon branch from 2dbf278 to ed15dc0 Compare September 30, 2024 21:49
@mikhail-sakhnov mikhail-sakhnov force-pushed the sharnoff/neonvm-daemon branch 3 times, most recently from b0d9d5d to 836094e Compare October 1, 2024 08:58
@Omrigan Omrigan marked this pull request as ready for review October 1, 2024 15:05
@Omrigan Omrigan self-requested a review October 1, 2024 15:05
sharnoff
sharnoff commented Oct 1, 2024
View reviewed changes
neonvm/daemon/Dockerfile Outdated Show resolved Hide resolved
@mikhail-sakhnov mikhail-sakhnov mentioned this pull request Oct 8, 2024
Open
@mikhail-sakhnov mikhail-sakhnov force-pushed the sharnoff/neonvm-daemon branch 5 times, most recently from 59de31c to 0524ce1 Compare October 9, 2024 10:08
@mikhail-sakhnov
Copy link
Contributor

@sharnoff I resolved highlighted issues, dropped neonvmd mentions from the build-image, kept it only in the build-test-vm workflow. neonvmd doesn't use go-chef anymore. I also moved neonvm/daemon to neonvm-daemon to comply with new flat structure. I first thought to keep it where it was (neonvm/daemon) because it is a component which runs in the VM, but at the same time, vm-runner also runs in the VM but looks like it belongs to neonvm-runner now.

@sharnoff @Omrigan could you take a look at the PR?

sharnoff
sharnoff commented Oct 9, 2024
View reviewed changes
Copy link
Member Author

@sharnoff sharnoff left a comment

Choose a reason for hiding this comment

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

Some comments, otherwise LGTM. I can't approve this since it's originally my PR, but you should be able to approve & merge it yourself 😄

.github/workflows/build-test-vm.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
neonvm-daemon/Dockerfile Outdated Show resolved Hide resolved
vm-builder/files/Dockerfile.img Outdated Show resolved Hide resolved
vm-builder/files/Dockerfile.img Outdated Show resolved Hide resolved
vm-builder/files/Dockerfile.img Outdated Show resolved Hide resolved
Omrigan
Omrigan approved these changes Oct 10, 2024 •
edited
Loading
View reviewed changes
Omrigan
Omrigan approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@Omrigan Omrigan left a comment

Choose a reason for hiding this comment

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

LGTM! (with all comments resolved)

@mikhail-sakhnov mikhail-sakhnov force-pushed the sharnoff/neonvm-daemon branch 3 times, most recently from e8cd908 to 1e3c8a8 Compare October 11, 2024 10:11
sharnoff and others added 7 commits October 14, 2024 12:40
@sharnoff @mikhail-sakhnov
neonvm: Add new skeleton neonvm-daemon binary
02083ad 
Extracted the minimal version from a PR, because it's clear that this
daemon is required in multiple places, and we don't know which PR will
merge first.
@mikhail-sakhnov
neonvm: apply ci fixes for extracted control daemon
0eafcab 
Signed-off-by: Misha Sakhnov <[email protected]>
@mikhail-sakhnov
neonvm: add smoke test to check that neonvmd is running in the vm
ba95e37 
Signed-off-by: Misha Sakhnov <[email protected]>
@mikhail-sakhnov
neonvmd: explicitly depend bin/vm-builder on neonvmd makefile target …
fd49afb 
…to prevent having the race in CI

Signed-off-by: Misha Sakhnov <[email protected]>
@mikhail-sakhnov @sharnoff
Update .github/workflows/build-images.yaml
9743e68 
Co-authored-by: Em Sharnoff <[email protected]>
@mikhail-sakhnov
neonvmd: build neonvmd image in the build-test-vm workflow to prevent…
5b59f8a 
… races with build-images workflow

Signed-off-by: Misha Sakhnov <[email protected]>
@mikhail-sakhnov
neonvmd: moved neonvm/daemon -> neonvm-daemon to comply with new repo…
b1fb1b0 
… flatten structure

Signed-off-by: Misha Sakhnov <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 14, 2024 •
edited
Loading

Merging this branch will not change overall coverage

Impacted Packages Coverage Δ 🤖
github.com/neondatabase/autoscaling/neonvm-daemon 0.00% (ø)
github.com/neondatabase/autoscaling/vm-builder 0.00% (ø)
Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/neondatabase/autoscaling/neonvm-daemon/main.go 0.00% (ø) 29 (+29) 0 29 (+29)
github.com/neondatabase/autoscaling/vm-builder/main.go 0.00% (ø) 215 (+7) 0 215 (+7)

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

HTML Report

Click to open

@mikhail-sakhnov mikhail-sakhnov merged commit 4156e39 into main Oct 14, 2024
22 checks passed
@mikhail-sakhnov mikhail-sakhnov deleted the sharnoff/neonvm-daemon branch October 14, 2024 11:08
sharnoff added a commit that referenced this pull request Oct 14, 2024
@sharnoff
neonvm-daemon: Move main.go into cmd subdir
b85fe8b 
Must've missed this in #1090 -- after #961, the common structure is that
main.go sits inside a cmd subdir for the component.
@sharnoff sharnoff mentioned this pull request Oct 14, 2024
Merged
sharnoff added a commit that referenced this pull request Oct 14, 2024
@sharnoff
neonvm-daemon: Move main.go into cmd subdir (#1112)
89f0882 
Must've missed this in #1090 — after #961, the common structure is that
main.go sits inside a cmd subdir for the component.
sharnoff added a commit that referenced this pull request Dec 3, 2024
@sharnoff
neonvm-daemon: Use go-chef in Dockerfile
9e43bd0 
Probably an inadvertent merge conflict between #1090 and #989 meaning we
accidentally weren't using go-chef for neonvm-daemon.

Noticed this while developing locally and saw that it was re-downloading
all of the dependencies for neonvm-daemon every time, even though I was
making changes in the scheduler and the dependencies hadn't changed.
sharnoff added a commit that referenced this pull request Dec 3, 2024
@sharnoff
neonvm-daemon: Use go-chef in Dockerfile
f363389 
Probably an inadvertent merge conflict between #1090 and #989 meaning we
accidentally weren't using go-chef for neonvm-daemon.

Noticed this while developing locally and saw that it was re-downloading
all of the dependencies for neonvm-daemon every time, even though I was
making changes in the scheduler and the dependencies hadn't changed.
@sharnoff sharnoff mentioned this pull request Dec 3, 2024
Merged
sharnoff added a commit that referenced this pull request Dec 4, 2024
@sharnoff
neonvm-daemon: Use go-chef in Dockerfile (#1165)
a0b1409 
Probably an inadvertent merge conflict between #1090 and #989 meaning we
accidentally weren't using go-chef for neonvm-daemon.

Noticed this while working on #1163 locally and saw that it was
re-downloading all of the dependencies for neonvm-daemon every time,
even though I was making changes in the scheduler and the dependencies
hadn't changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikhail-sakhnov mikhail-sakhnov mikhail-sakhnov left review comments

@Omrigan Omrigan Omrigan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sharnoff @mikhail-sakhnov @Omrigan