Skip to content

Commit

Permalink
refactor : Use strongunits.MiB / strongunits.GiB to store memory …
Browse files Browse the repository at this point in the history
…size (crc-org#1642)

Currently we're using memory values in `uint64` variable. This works but sometimes it can be confusing to figure out what unit we're using if variable is not named correctly.

Instead, we can rely on strongunits types for byte/MegaByte and GigaByte for these values.

Signed-off-by: Rohan Kumar <[email protected]>
  • Loading branch information
rohanKanojia committed Jan 7, 2025
1 parent a697b9e commit fe4daab
Show file tree
Hide file tree
Showing 17 changed files with 156 additions and 144 deletions.
13 changes: 7 additions & 6 deletions cmd/crc/cmd/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"github.com/containers/common/pkg/strongunits"
"io"
"os"
"runtime"
Expand Down Expand Up @@ -38,8 +39,8 @@ func init() {
flagSet.StringP(crcConfig.Bundle, "b", constants.GetDefaultBundlePath(crcConfig.GetPreset(config)), crcConfig.BundleHelpMsg(config))
flagSet.StringP(crcConfig.PullSecretFile, "p", "", fmt.Sprintf("File path of image pull secret (download from %s)", constants.CrcLandingPageURL))
flagSet.UintP(crcConfig.CPUs, "c", constants.GetDefaultCPUs(crcConfig.GetPreset(config)), "Number of CPU cores to allocate to the instance")
flagSet.UintP(crcConfig.Memory, "m", constants.GetDefaultMemory(crcConfig.GetPreset(config)), "MiB of memory to allocate to the instance")
flagSet.UintP(crcConfig.DiskSize, "d", constants.DefaultDiskSize, "Total size in GiB of the disk used by the instance")
flagSet.UintP(crcConfig.Memory, "m", uint(constants.GetDefaultMemory(crcConfig.GetPreset(config))), "MiB of memory to allocate to the instance")
flagSet.UintP(crcConfig.DiskSize, "d", uint(constants.DefaultDiskSize), "Total size in GiB of the disk used by the instance")
flagSet.StringP(crcConfig.NameServer, "n", "", "IPv4 address of nameserver to use for the instance")
flagSet.Bool(crcConfig.DisableUpdateCheck, false, "Don't check for update")

Expand Down Expand Up @@ -69,8 +70,8 @@ func runStart(ctx context.Context) (*types.StartResult, error) {

startConfig := types.StartConfig{
BundlePath: config.Get(crcConfig.Bundle).AsString(),
Memory: config.Get(crcConfig.Memory).AsUInt(),
DiskSize: config.Get(crcConfig.DiskSize).AsUInt(),
Memory: strongunits.MiB(config.Get(crcConfig.Memory).AsUInt()),
DiskSize: strongunits.GiB(config.Get(crcConfig.DiskSize).AsUInt()),
CPUs: config.Get(crcConfig.CPUs).AsUInt(),
NameServer: config.Get(crcConfig.NameServer).AsString(),
PullSecret: cluster.NewInteractivePullSecretLoader(config),
Expand Down Expand Up @@ -181,13 +182,13 @@ func (s *startResult) prettyPrintTo(writer io.Writer) error {
}

func validateStartFlags() error {
if err := validation.ValidateMemory(config.Get(crcConfig.Memory).AsUInt(), crcConfig.GetPreset(config)); err != nil {
if err := validation.ValidateMemory(strongunits.MiB(config.Get(crcConfig.Memory).AsUInt()), crcConfig.GetPreset(config)); err != nil {
return err
}
if err := validation.ValidateCPUs(config.Get(crcConfig.CPUs).AsUInt(), crcConfig.GetPreset(config)); err != nil {
return err
}
if err := validation.ValidateDiskSize(config.Get(crcConfig.DiskSize).AsUInt()); err != nil {
if err := validation.ValidateDiskSize(strongunits.GiB(config.Get(crcConfig.DiskSize).AsUInt())); err != nil {
return err
}
if err := validation.ValidateBundle(config.Get(crcConfig.Bundle).AsString(), crcConfig.GetPreset(config)); err != nil {
Expand Down
27 changes: 14 additions & 13 deletions cmd/crc/cmd/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cmd
import (
"errors"
"fmt"
"github.com/containers/common/pkg/strongunits"
"io"
"net/url"
"os"
Expand Down Expand Up @@ -44,14 +45,14 @@ type status struct {
CrcStatus string `json:"crcStatus,omitempty"`
OpenShiftStatus types.OpenshiftStatus `json:"openshiftStatus,omitempty"`
OpenShiftVersion string `json:"openshiftVersion,omitempty"`
DiskUsage int64 `json:"diskUsage,omitempty"`
DiskSize int64 `json:"diskSize,omitempty"`
CacheUsage int64 `json:"cacheUsage,omitempty"`
DiskUsage strongunits.B `json:"diskUsage,omitempty"`
DiskSize strongunits.B `json:"diskSize,omitempty"`
CacheUsage strongunits.B `json:"cacheUsage,omitempty"`
CacheDir string `json:"cacheDir,omitempty"`
RAMSize int64 `json:"ramSize,omitempty"`
RAMUsage int64 `json:"ramUsage,omitempty"`
PersistentVolumeUse int `json:"persistentVolumeUsage,omitempty"`
PersistentVolumeSize int `json:"persistentVolumeSize,omitempty"`
RAMSize strongunits.B `json:"ramSize,omitempty"`
RAMUsage strongunits.B `json:"ramUsage,omitempty"`
PersistentVolumeUse strongunits.B `json:"persistentVolumeUsage,omitempty"`
PersistentVolumeSize strongunits.B `json:"persistentVolumeSize,omitempty"`
Preset preset.Preset `json:"preset"`
}

Expand All @@ -67,8 +68,8 @@ func runWatchStatus(writer io.Writer, client *daemonclient.Client, cacheDir stri

status := getStatus(client, cacheDir)
// do not render RAM size/use
status.RAMSize = -1
status.RAMUsage = -1
status.RAMSize = 0
status.RAMUsage = 0
renderError := render(status, writer, outputFormat)
if renderError != nil {
return renderError
Expand Down Expand Up @@ -107,8 +108,8 @@ func runWatchStatus(writer io.Writer, client *daemonclient.Client, cacheDir stri
}
}

ramBar.SetTotal(loadResult.RAMSize)
ramBar.SetCurrent(loadResult.RAMUse)
ramBar.SetTotal(int64(loadResult.RAMSize))
ramBar.SetCurrent(int64(loadResult.RAMUse))
for i, cpuLoad := range loadResult.CPUUse {
cpuBars[i].SetCurrent(cpuLoad)
}
Expand Down Expand Up @@ -173,7 +174,7 @@ func getStatus(client *daemonclient.Client, cacheDir string) *status {
DiskSize: clusterStatus.DiskSize,
RAMSize: clusterStatus.RAMSize,
RAMUsage: clusterStatus.RAMUse,
CacheUsage: size,
CacheUsage: strongunits.B(size),
PersistentVolumeUse: clusterStatus.PersistentVolumeUse,
PersistentVolumeSize: clusterStatus.PersistentVolumeSize,
CacheDir: cacheDir,
Expand All @@ -197,7 +198,7 @@ func (s *status) prettyPrintTo(writer io.Writer) error {

lines = append(lines, line{s.Preset.ForDisplay(), openshiftStatus(s)})

if s.RAMSize != -1 && s.RAMUsage != -1 {
if s.RAMSize != 0 && s.RAMUsage != 0 {
lines = append(lines, line{"RAM Usage", fmt.Sprintf(
"%s of %s",
units.HumanSize(float64(s.RAMUsage)),
Expand Down
2 changes: 0 additions & 2 deletions cmd/crc/cmd/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ func TestPlainStatus(t *testing.T) {

expected := `CRC VM: Running
OpenShift: Running (v4.5.1)
RAM Usage: 0B of 0B
Disk Usage: 10GB of 20GB (Inside the CRC VM)
Cache Usage: 10kB
Cache Directory: %s
Expand Down Expand Up @@ -89,7 +88,6 @@ func TestStatusWithoutPodman(t *testing.T) {

expected := `CRC VM: Running
OpenShift: Running (v4.5.1)
RAM Usage: 0B of 0B
Disk Usage: 10GB of 20GB (Inside the CRC VM)
Cache Usage: 10kB
Cache Directory: %s
Expand Down
11 changes: 6 additions & 5 deletions pkg/crc/api/api_client_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package api

import (
"github.com/containers/common/pkg/strongunits"
"net/http"
"net/http/httptest"
"os"
Expand Down Expand Up @@ -68,10 +69,10 @@ func TestStatus(t *testing.T) {
CrcStatus: "Running",
OpenshiftStatus: "Running",
OpenshiftVersion: "4.5.1",
DiskUse: int64(10000000000),
DiskSize: int64(20000000000),
RAMUse: int64(1000),
RAMSize: int64(2000),
DiskUse: strongunits.B(10000000000),
DiskSize: strongunits.B(20000000000),
RAMUse: strongunits.B(1000),
RAMSize: strongunits.B(2000),
Preset: preset.OpenShift,
},
statusResult,
Expand Down Expand Up @@ -224,7 +225,7 @@ func TestConfigGetMultiple(t *testing.T) {
apiClient.GetConfigResult{
Configs: map[string]interface{}{
"cpus": float64(4),
"memory": float64(10752),
"memory": strongunits.MiB(10752),
},
},
configGetMultiplePropertyResult,
Expand Down
13 changes: 7 additions & 6 deletions pkg/crc/api/client/types.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package client

import (
"github.com/containers/common/pkg/strongunits"
"github.com/crc-org/crc/v2/pkg/crc/machine/state"
"github.com/crc-org/crc/v2/pkg/crc/machine/types"
"github.com/crc-org/crc/v2/pkg/crc/preset"
Expand All @@ -23,12 +24,12 @@ type ClusterStatusResult struct {
CrcStatus string
OpenshiftStatus string
OpenshiftVersion string `json:"OpenshiftVersion,omitempty"`
DiskUse int64
DiskSize int64
RAMUse int64
RAMSize int64
PersistentVolumeUse int `json:"PersistentVolumeUse,omitempty"`
PersistentVolumeSize int `json:"PersistentVolumeSize,omitempty"`
DiskUse strongunits.B
DiskSize strongunits.B
RAMUse strongunits.B
RAMSize strongunits.B
PersistentVolumeUse strongunits.B `json:"PersistentVolumeUse,omitempty"`
PersistentVolumeSize strongunits.B `json:"PersistentVolumeSize,omitempty"`
Preset preset.Preset
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/crc/api/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package api

import (
gocontext "context"
"github.com/containers/common/pkg/strongunits"
"net/http"

"github.com/crc-org/crc/v2/pkg/crc/api/client"
Expand Down Expand Up @@ -121,8 +122,8 @@ func (h *Handler) Start(c *context) error {
func getStartConfig(cfg crcConfig.Storage, args client.StartConfig) types.StartConfig {
return types.StartConfig{
BundlePath: cfg.Get(crcConfig.Bundle).AsString(),
Memory: cfg.Get(crcConfig.Memory).AsUInt(),
DiskSize: cfg.Get(crcConfig.DiskSize).AsUInt(),
Memory: strongunits.MiB(cfg.Get(crcConfig.Memory).AsUInt()),
DiskSize: strongunits.GiB(cfg.Get(crcConfig.DiskSize).AsUInt()),
CPUs: cfg.Get(crcConfig.CPUs).AsUInt(),
NameServer: cfg.Get(crcConfig.NameServer).AsString(),
PullSecret: cluster.NewNonInteractivePullSecretLoader(cfg, args.PullSecretFile),
Expand Down
3 changes: 2 additions & 1 deletion pkg/crc/config/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"fmt"
"github.com/containers/common/pkg/strongunits"
"runtime"

"github.com/crc-org/crc/v2/pkg/crc/constants"
Expand Down Expand Up @@ -155,7 +156,7 @@ func defaultCPUs(cfg Storage) uint {
return constants.GetDefaultCPUs(GetPreset(cfg))
}

func defaultMemory(cfg Storage) uint {
func defaultMemory(cfg Storage) strongunits.MiB {
return constants.GetDefaultMemory(GetPreset(cfg))
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/crc/config/settings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"fmt"
"github.com/containers/common/pkg/strongunits"
"path/filepath"
"testing"

Expand All @@ -20,7 +21,8 @@ func validateMemoryNoPhysicalCheck(value interface{}, preset crcpreset.Preset) (
if err != nil {
return false, fmt.Sprintf("requires integer value in MiB >= %d", constants.GetDefaultMemory(preset))
}
if v < constants.GetDefaultMemory(preset) {
memory := strongunits.MiB(v)
if memory < constants.GetDefaultMemory(preset) {
return false, fmt.Sprintf("requires memory in MiB >= %d", constants.GetDefaultMemory(preset))
}
return true, ""
Expand Down
9 changes: 6 additions & 3 deletions pkg/crc/config/validations.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"fmt"
"github.com/containers/common/pkg/strongunits"
"runtime"
"strings"

Expand Down Expand Up @@ -31,10 +32,11 @@ func validateString(value interface{}) (bool, string) {

// validateDiskSize checks if provided disk size is valid in the config
func validateDiskSize(value interface{}) (bool, string) {
diskSize, err := cast.ToUintE(value)
valueAsUInt, err := cast.ToUintE(value)
if err != nil {
return false, fmt.Sprintf("could not convert '%s' to integer", value)
}
diskSize := strongunits.GiB(valueAsUInt)
if err := validation.ValidateDiskSize(diskSize); err != nil {
return false, err.Error()
}
Expand Down Expand Up @@ -70,11 +72,12 @@ func validateCPUs(value interface{}, preset crcpreset.Preset) (bool, string) {
// validateMemory checks if provided memory is valid in the config
// It's defined as a variable so that it can be overridden in tests to disable the physical memory check
var validateMemory = func(value interface{}, preset crcpreset.Preset) (bool, string) {
v, err := cast.ToUintE(value)
valueAsInt, err := cast.ToUintE(value)
if err != nil {
return false, fmt.Sprintf("requires integer value in MiB >= %d", constants.GetDefaultMemory(preset))
}
if err := validation.ValidateMemory(v, preset); err != nil {
memory := strongunits.MiB(valueAsInt)
if err := validation.ValidateMemory(memory, preset); err != nil {
return false, err.Error()
}
return true, ""
Expand Down
5 changes: 3 additions & 2 deletions pkg/crc/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package constants

import (
"fmt"
"github.com/containers/common/pkg/strongunits"
"os"
"path/filepath"
"runtime"
Expand All @@ -13,7 +14,7 @@ import (

const (
DefaultName = "crc"
DefaultDiskSize = 31
DefaultDiskSize = strongunits.GiB(31)

DefaultPersistentVolumeSize = 15

Expand Down Expand Up @@ -211,7 +212,7 @@ func GetDefaultCPUs(preset crcpreset.Preset) uint {
}
}

func GetDefaultMemory(preset crcpreset.Preset) uint {
func GetDefaultMemory(preset crcpreset.Preset) strongunits.MiB {
switch preset {
case crcpreset.OpenShift, crcpreset.OKD:
return 10752
Expand Down
9 changes: 6 additions & 3 deletions pkg/crc/machine/config/config.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package config

import "github.com/crc-org/crc/v2/pkg/crc/network"
import (
"github.com/containers/common/pkg/strongunits"
"github.com/crc-org/crc/v2/pkg/crc/network"
)

type MachineConfig struct {
// CRC system bundle
Expand All @@ -9,10 +12,10 @@ type MachineConfig struct {
// Virtual machine configuration
Name string
// Memory holds value in MiB
Memory uint
Memory strongunits.MiB
CPUs uint
// DiskSize holds value in GiB
DiskSize uint
DiskSize strongunits.GiB
ImageSourcePath string
ImageFormat string
SSHKeyPath string
Expand Down
8 changes: 2 additions & 6 deletions pkg/crc/machine/config/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,10 @@ import (
"github.com/crc-org/machine/libmachine/drivers"
)

func ConvertGiBToBytes(gib uint) uint64 {
return uint64(gib) * 1024 * 1024 * 1024
}

func InitVMDriverFromMachineConfig(machineConfig MachineConfig, driver *drivers.VMDriver) {
driver.CPU = machineConfig.CPUs
driver.Memory = machineConfig.Memory
driver.DiskCapacity = ConvertGiBToBytes(machineConfig.DiskSize)
driver.Memory = uint(machineConfig.Memory)
driver.DiskCapacity = uint64(machineConfig.DiskSize.ToBytes())
driver.BundleName = machineConfig.BundleName
driver.ImageSourcePath = machineConfig.ImageSourcePath
driver.ImageFormat = machineConfig.ImageFormat
Expand Down
16 changes: 8 additions & 8 deletions pkg/crc/machine/driver.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package machine

import (
"github.com/crc-org/crc/v2/pkg/crc/machine/config"
"github.com/containers/common/pkg/strongunits"
"github.com/crc-org/crc/v2/pkg/libmachine/host"
libmachine "github.com/crc-org/machine/libmachine/drivers"
)
Expand All @@ -21,12 +21,12 @@ func updateDriverValue(host *host.Host, setDriverValue valueSetter) error {
return updateDriverConfig(host, driver)
}

func setMemory(host *host.Host, memorySize uint) error {
func setMemory(host *host.Host, memorySize strongunits.MiB) error {
memorySetter := func(driver *libmachine.VMDriver) bool {
if driver.Memory == memorySize {
if driver.Memory == uint(memorySize.ToBytes()) {
return false
}
driver.Memory = memorySize
driver.Memory = uint(memorySize)
return true
}

Expand All @@ -45,13 +45,13 @@ func setVcpus(host *host.Host, vcpus uint) error {
return updateDriverValue(host, vcpuSetter)
}

func setDiskSize(host *host.Host, diskSizeGiB uint) error {
func setDiskSize(host *host.Host, diskSize strongunits.GiB) error {
diskSizeSetter := func(driver *libmachine.VMDriver) bool {
capacity := config.ConvertGiBToBytes(diskSizeGiB)
if driver.DiskCapacity == capacity {
capacity := diskSize.ToBytes()
if driver.DiskCapacity == uint64(capacity) {
return false
}
driver.DiskCapacity = capacity
driver.DiskCapacity = uint64(capacity)
return true
}

Expand Down
Loading

0 comments on commit fe4daab

Please sign in to comment.