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

validate cpus according to actual number of node cpus #2057

Merged
merged 3 commits into from
Sep 25, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions pkg/vm.go
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@ import (
"net"
"path/filepath"

"github.com/shirou/gopsutil/cpu"
"github.com/threefoldtech/zos/pkg/gridtypes"
"github.com/threefoldtech/zos/pkg/gridtypes/zos"
)
@@ -126,7 +127,7 @@ type VM struct {
Network VMNetworkInfo
// KernelImage path to uncompressed linux kernel ELF
KernelImage string
// InitrdImage (optiona) path to initrd disk
// InitrdImage (optional) path to initrd disk
InitrdImage string
// KernelArgs to override the default kernel arguments. (default: "ro console=ttyS0 noapic reboot=k panic=1 pci=off nomodules")
KernelArgs KernelArgs
@@ -175,8 +176,16 @@ func (vm *VM) Validate() error {
return fmt.Errorf("invalid memory must not be less than 250M")
}

if vm.CPU == 0 || vm.CPU > 32 {
return fmt.Errorf("invalid cpu must be between 1 and 32")
n, err := cpu.Counts(true)
Copy link
Collaborator

@xmonader xmonader Sep 17, 2023

Choose a reason for hiding this comment

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

can't we use runtime.NumCPU? https://pkg.go.dev/runtime#NumCPU, if so, let's not introduce a new dependency like gopsutil

Copy link
Collaborator

Choose a reason for hiding this comment

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

n -> numCpus or ncpus or cpus

Copy link
Member

Choose a reason for hiding this comment

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

can't we use runtime.NumCPU? https://pkg.go.dev/runtime#NumCPU, if so, let's not introduce a new dependency like gopsutil

this is not the same, according to the docs, the runtime.NumCPU is logical number of cpus that can be used by the current process. this is a totally Go runtime thing, not related to the total number of CPUs available on the system

Copy link
Member

Choose a reason for hiding this comment

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

But should definitely rename the variable from n to something else

if err != nil {
return fmt.Errorf("failed to get count of cpus")
}

if vm.CPU == 0 || vm.CPU > uint8(n) {
if n == 1 {
return fmt.Errorf("invalid cpu must be 1")
}
return fmt.Errorf("invalid cpu must be between 1 and %d", n)
}

for _, shared := range vm.Shared {
@@ -234,7 +243,7 @@ func (n *NetMetric) Nu() float64 {
}

// MachineMetric is a container for metrics from multiple networks
// currently only groped as private (wiregaurd + yggdrasil), and public (public Ips)
// currently only groped as private (wireguard + yggdrasil), and public (public Ips)
rawdaGastan marked this conversation as resolved.
Show resolved Hide resolved
type MachineMetric struct {
Private NetMetric
Public NetMetric