Skip to content

Propegate Default HW Spec Values to the Backend#13

Merged
noorvir merged 7 commits intomasterfrom
callum/unw-182-2
May 31, 2023
Merged

Propegate Default HW Spec Values to the Backend#13
noorvir merged 7 commits intomasterfrom
callum/unw-182-2

Conversation

@caldempsey
Copy link
Copy Markdown
Contributor

@caldempsey caldempsey commented May 30, 2023

This pull request adds the SetDefaultValues function to establish sensible default values and constraints for unset fields within the HardwareSpec struct. This supports the backend and CLI by ensuring sensible defaults for Node/Execs get set. Since this information gets cached in our local DB the approach also assumes Providers return an error if they are unable to supply a container or VM with a fully qualified HardwareSpec. So rather than using the response of each Provider as a source of truth, we trust the reflective property of the HW specification asked for.

Key Changes:

  • Introduced constants for consistent default values.
  • Added logic to handle unset minimum and maximum values.
  • Enforced constraints to prevent maximum values from being below minimum values for any HW Spec.
  • Update to prefer HDD in place of Storage

Supports: unweave/cli#14

@vercel
Copy link
Copy Markdown

vercel bot commented May 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2023 9:20am

@caldempsey caldempsey changed the base branch from callum/unw-182 to master May 30, 2023 14:39
@caldempsey caldempsey changed the base branch from master to callum/unw-182 May 30, 2023 14:39
@caldempsey caldempsey requested a review from noorvir May 30, 2023 14:40
Base automatically changed from callum/unw-182 to master May 30, 2023 15:43
HDD HardwareRequestRange `json:"hdd"`
}

func SetSpecDefaultValues(spec HardwareSpec) HardwareSpec {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe do one of either mutating the state or returning a new HardwareSpec type but not both. That seems like it would cause issues. Maybe make a copy and return it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can do that!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This got marked as outdated but still un resolved

Copy link
Copy Markdown
Contributor Author

@caldempsey caldempsey May 30, 2023

Choose a reason for hiding this comment

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

Feedback hasn’t been addressed yet, will quickly fix this up tonight

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Resolved

# Conflicts:
#	api/server/exec.go
@noorvir noorvir merged commit 4cc2e49 into master May 31, 2023
@noorvir noorvir deleted the callum/unw-182-2 branch May 31, 2023 20:53
defaultMinCPU = 1
defaultMinHDD = 4
defaultMinGPUs = 1
defaultGPURequest = "rtx_4000"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was a mistake actually. The GPU type is provider specific so it can't go in the generic hardware spec. We probably need to validate the hardware spec in each provider.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants