-
Notifications
You must be signed in to change notification settings - Fork 2
Add host resource validation before creating VMs #46
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing , try the following before open a pull request
make test
make build
|
@mrgb7 I got the error now, it failed when it try to |
| return fmt.Errorf("failed to validate host resources: %w", err) | ||
| } | ||
|
|
||
| portStatus, err := validator.ValidatePorts() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what is the problem on Ports, is not multipass ensure dedicated IP address for each node ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mention in issue feature file, you want to validate if port is available? , if not -> fail fast.
That's what i tried to achieve, I don't know where multipass check for port in code for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, please keep in mind that when I outline a new issue or feature, it's not necessarily the finalized design. Some assumptions may be inaccurate or incomplete, so it's important that you validate and critically evaluate the requirements before proceeding
internal/validator/platform.go
Outdated
| } | ||
|
|
||
| func getAvailableMemoryImpl() (float64, error) { | ||
| var m runtime.MemStats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runtime memStats it reflects go runtime managed memory allocated only.
read this
https://cs.opensource.google/go/go/+/refs/tags/go1.24.4:src/runtime/mstats.go;l=347
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it. i will fix it.
fixing how i can got the available memory
internal/validator/platform.go
Outdated
| return runtime.NumCPU(), nil | ||
| } | ||
|
|
||
| func getAvailableMemoryImpl() (float64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Impl suffix means ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that , This is a common pattern in Go for making code more testable and flexible, especially when dealing with system-level operations that might need to be mocked in tests. so i do it in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmn3m Could you refer to the docs I am interested to learn more about this convention in go ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrgb7 ,
To be honest, there isn’t an official Go documentation link that defines the Implsuffix as a convention.
In this case, I used the Impl suffix to clearly separate the default implementation from the function variable, which can then be overridden in tests. This allows us to inject mocks without using interfaces.
Is there any alternative, I can use instead of that suffix?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this isn't considered best practice in Go. Golang is designed to be straightforward. so It doesn't require interfaces solely for testing purposes. It's not an object-oriented language, but rather one that emphasizes simplicity and package-based structuring. You can effectively write unit tests without unnecessary abstraction layers. I'd recommend checking out this article for a clear explanation on writing tests in Go using the built-in testing package: How to Write Unit Tests in Go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrgb7 Noticed, I fixed.
removing Impl suffix | handling testing
| } | ||
|
|
||
| if !skipValidation { | ||
| requirements, err := validator.CalculateResourceRequirements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see an issue on calculating disk it returns 0GB free
./bin/playground cluster create --name obs-test --size 1 \
--master-cpus 8 --master-memory 20G
Resource validation failed:
❌ Disk: 0.0 GB available (20.0 GB required)
Recommendations:
- Free up at least 20.0 GB of disk space to meet minimum requirements
- Consider cleaning up disk space for optimal performance
To bypass this check, use: playground create cluster --skip-host-validation
Failed to create cluster: insufficient resources for cluster creation
Add Automatic Host Resource Validation to Cluster Creation
Summary
This PR implements automatic resource validation during the
playground create clusterworkflow. It checks the host machine for minimum CPU, memory, disk space, and port availability before proceeding with cluster provisioning. If validation fails, cluster creation is halted with clear, actionable feedback.Key Changes
--skip-host-validationand--forceflags to override default behaviorLinked Issue