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

validate cpus according to actual number of node cpus #2057

Merged
merged 3 commits into from
Sep 25, 2023
Merged

validate cpus according to actual number of node cpus #2057

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
575f520
validate cpus according to actual number of nodes cpus
rawdaGastan Sep 17, 2023
03d48f6
fix typos
rawdaGastan Sep 17, 2023
f5d7c9d
rename n to cpus
rawdaGastan Sep 25, 2023
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
2 changes: 1 addition & 1 deletion pkg/gridtypes/zos/zmachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func (v ZMachine) Valid(getter gridtypes.WorkloadGetter) error {
}
}
if v.ComputeCapacity.CPU == 0 {
return fmt.Errorf("cpu capcity can't be 0")
return fmt.Errorf("cpu capacity can't be 0")
}
if v.ComputeCapacity.Memory < 250*gridtypes.Megabyte {
return fmt.Errorf("mem capacity can't be less that 250M")
Expand Down
17 changes: 13 additions & 4 deletions pkg/vm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 •
edited
Loading

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 {
Expand Down Expand Up @@ -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 grouped as private (wireguard + yggdrasil), and public (public Ips)
type MachineMetric struct {
Private NetMetric
Public NetMetric
Expand Down