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-controller: add spec.cpuScalingMode #1104

Closed
wants to merge 1 commit into from
Closed

neonvm-controller: add spec.cpuScalingMode #1104

wants to merge 1 commit into from

Conversation

mikhail-sakhnov
Copy link
Contributor

Add the new field to the VirtualMachine spec, and change the VM-controller to explicitly update a VM with the default value of the field.
Add temporary e2e test to ensure that VM without spec.cpuScalingMode is updated as expected.

Part of the preparations for #1082

@mikhail-sakhnov mikhail-sakhnov force-pushed the misha/add-vm-spec-field-cpu-scaling-mode branch from e860c64 to 5212dc9 Compare October 11, 2024 11:02
@mikhail-sakhnov mikhail-sakhnov marked this pull request as ready for review October 11, 2024 11:36
sharnoff
sharnoff reviewed Oct 11, 2024
View reviewed changes
neonvm/apis/neonvm/v1/virtualmachine_types.go Outdated
Comment on lines 58 to 62
CpuScalingModeQMP string = "qmp_scaling"

// CpuScalingModeCpuSysfsState is the value of the VirtualMachineSpec.CpuScalingMode field that
// indicates that the VM should use the CPU sysfs state interface to scale CPUs.
CpuScalingModeCpuSysfsState string = "cpu_sysfs_state_scaling"
Copy link
Member

Choose a reason for hiding this comment

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

Let's use upper camel case here instead of snake case, but otherwise lgtm

@mikhail-sakhnov mikhail-sakhnov force-pushed the misha/add-vm-spec-field-cpu-scaling-mode branch 3 times, most recently from 0ecb11d to 5184a2a Compare October 14, 2024 12:19
Omrigan
Omrigan approved these changes Oct 14, 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.

Suggested simpler names; otherwise LGTM.

neonvm/apis/neonvm/v1/virtualmachine_types.go Outdated Show resolved Hide resolved
@mikhail-sakhnov mikhail-sakhnov mentioned this pull request Oct 14, 2024
Merged
sharnoff
sharnoff reviewed Oct 14, 2024
View reviewed changes
Copy link
Member

@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.

One last thought: Does this PR get us anything without #1111? If no: should it be combined into that PR? if yes: what?

(asking genuinely, not for any particular outcome)

@mikhail-sakhnov mikhail-sakhnov mentioned this pull request Oct 14, 2024
Closed
@mikhail-sakhnov
Copy link
Contributor Author

@sharnoff this PR is required for #1111, but we first need to have a controller version in place that overwrites current VM instances to have defaulted at the time value (qmp based scaling), as we discussed so we could enable sysfs-based scaling only for new instances when we start rolling it out. I think that aligns well with what we discussed last week about the way to update CRDs?

@mikhail-sakhnov
neonvm-controller: add spec.cpuScalingMode, change vm-controller to e…
be2ad48 
…xplicitly default the field value if it is not set

Signed-off-by: Misha Sakhnov <[email protected]>
@mikhail-sakhnov mikhail-sakhnov force-pushed the misha/add-vm-spec-field-cpu-scaling-mode branch from 5184a2a to be2ad48 Compare October 14, 2024 14:37
@github-actions GitHub Actions
Copy link

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

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/neondatabase/autoscaling/neonvm/apis/neonvm/v1 0.56% (-0.00%) 👎
github.com/neondatabase/autoscaling/pkg/neonvm/controllers 11.73% (+0.22%) 👍
Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/neondatabase/autoscaling/neonvm/apis/neonvm/v1/virtualmachine_types.go 1.92% (ø) 52 1 51
github.com/neondatabase/autoscaling/neonvm/apis/neonvm/v1/zz_generated.deepcopy.go 0.00% (ø) 427 (+4) 0 427 (+4)
github.com/neondatabase/autoscaling/pkg/neonvm/controllers/vm_controller.go 25.60% (+0.37%) 668 (+6) 171 (+4) 497 (+2) 👍

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.

Changed unit test files

  • github.com/neondatabase/autoscaling/pkg/neonvm/controllers/vm_controller_unit_test.go

HTML Report

Click to open

@mikhail-sakhnov
Copy link
Contributor Author

close in favor of #1111, which already has the changes for the current PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharnoff sharnoff sharnoff 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
@mikhail-sakhnov @Omrigan @sharnoff