Add CalcOnlyRunning option to ProxMox scheduler #432
Conversation
deepdivenow
requested review from
65278,
avorima,
mcbenjemaa,
pborn-ionos and
wikkyk
as code owners
External PRTest runs on external PRs require manual approval. |
🚀 e2e tests runWe add labels to the PRs to control the e2e test runs by running specific tests and skipping some test contexts,
ℹ️ Ask a |
| ) | ||
| opts := []any{proxmox.WithHTTPClient(httpClient), proxmox.WithAPIToken(ProxmoxTokenID, ProxmoxSecret)} | ||
| if strings.ToLower(ProxmoxReserveOnlyRunning) == "true" { | ||
| opts = append(opts, goproxmox.WithCalcOnlyRunning()) |
There was a problem hiding this comment.
Wouldn't be better to set it as field in the cluster/machine spec?
There was a problem hiding this comment.
Wouldn't be better to set it as field in the cluster/machine spec?
Maybe on Cluster scope, on mashine scope need special config structure for each physical node in physical proxmox Cluster.
proxmoxmachines.infrastructure.cluster.x-k8s.io is virtualMashine and very strange set this option here (not For Physical mashine|node)
|
What's the context for this change? |
| // ProxmoxSecret env variable that defines the Proxmox secret for the given token id. | ||
| ProxmoxSecret string | ||
| ProxmoxSecret string | ||
| ProxmoxReserveOnlyRunning string |
There was a problem hiding this comment.
The name of this variable isn't particularly descriptive and it's missing a comment.
The type should be bool.
| proxmox.WithAPIToken(ProxmoxTokenID, ProxmoxSecret), | ||
| ) | ||
| opts := []any{proxmox.WithHTTPClient(httpClient), proxmox.WithAPIToken(ProxmoxTokenID, ProxmoxSecret)} | ||
| if strings.ToLower(ProxmoxReserveOnlyRunning) == "true" { |
| ProxmoxURL = env.GetString("PROXMOX_URL", "") | ||
| ProxmoxTokenID = env.GetString("PROXMOX_TOKEN", "") | ||
| ProxmoxSecret = env.GetString("PROXMOX_SECRET", "") | ||
| ProxmoxReserveOnlyRunning = env.GetString("PROXMOX_RESERVE_ONLY_RUNNING", "") |
There was a problem hiding this comment.
A new environment variable needs user documentation.
| // NewAPIClient initializes a Proxmox API client. If the client is misconfigured, an error is returned. | ||
| func NewAPIClient(ctx context.Context, logger logr.Logger, baseURL string, options ...proxmox.Option) (*APIClient, error) { | ||
| // options can be goproxmox.Option or proxmox.Option | ||
| func NewAPIClient(ctx context.Context, logger logr.Logger, baseURL string, options ...any) (*APIClient, error) { |
| var pOpts []proxmox.Option | ||
| var apiOpts []Option | ||
| for _, option := range options { | ||
| switch o := option.(type) { | ||
| case proxmox.Option: | ||
| pOpts = append(pOpts, o) | ||
| case Option: | ||
| apiOpts = append(apiOpts, o) | ||
| default: | ||
| return nil, fmt.Errorf("invalid option %T", option) | ||
| } | ||
| } |
There was a problem hiding this comment.
This should've been a type.
There was a problem hiding this comment.
Can you get example
How to not use "any", if we have type from upstream library "proxmox.Option", and we need local type in this library?
| return &APIClient{ | ||
| ac := &APIClient{ | ||
| Client: upstreamClient, | ||
| logger: logger, | ||
| }, nil | ||
| } | ||
| for _, o := range apiOpts { | ||
| o(ac) | ||
| } | ||
| return ac, nil |
For example my test environment have a lot VMs, but most of the time these machines are not working. |
3 participants
Issue #, if available:
Description of changes:
Add CalcOnlyRunning option to ProxMox scheduler.
Add Scheduler feature ignore VMs in Not Running State for test environments.
Refactor goproxmox options for support new local options
Testing performed:
Testing on local env with proxmox 8.3.3