Skip to content

Change KubeVirt VM CPU assignment to not overwrite cpu alloc ratio #1906

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
50 changes: 36 additions & 14 deletions pkg/cloudprovider/provider/kubevirt/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ type Config struct {
DNSConfig *corev1.PodDNSConfig
DNSPolicy corev1.DNSPolicy
CPUs string
VCPUs *kubevirtcorev1.CPU
Resources *corev1.ResourceList
Memory string
Namespace string
OSImageSource *cdicorev1beta1.DataVolumeSource
Expand Down Expand Up @@ -273,14 +275,21 @@ func (p *provider) getConfig(provSpec clusterv1alpha1.ProviderSpec) (*Config, *p
return nil, nil, fmt.Errorf("failed to decode kubeconfig: %w", err)
}

config.CPUs, err = p.configVarResolver.GetConfigVarStringValue(rawConfig.VirtualMachine.Template.CPUs)
cpus, err := p.configVarResolver.GetConfigVarStringValue(rawConfig.VirtualMachine.Template.CPUs)
if err != nil {
return nil, nil, fmt.Errorf(`failed to get value of "cpus" field: %w`, err)
}
config.Memory, err = p.configVarResolver.GetConfigVarStringValue(rawConfig.VirtualMachine.Template.Memory)

memory, err := p.configVarResolver.GetConfigVarStringValue(rawConfig.VirtualMachine.Template.Memory)
if err != nil {
return nil, nil, fmt.Errorf(`failed to get value of "memory" field: %w`, err)
}

config.Resources, config.VCPUs, err = parseResources(cpus, memory, rawConfig.VirtualMachine.Template.VCPUs)
if err != nil {
return nil, nil, fmt.Errorf(`failed to get value of "vcpusEnabled" field: %w`, err)
}

config.Namespace = getNamespace()
if len(rawConfig.VirtualMachine.Template.PrimaryDisk.ExtraHeaders) > 0 {
config.ExtraHeaders = rawConfig.VirtualMachine.Template.PrimaryDisk.ExtraHeaders
Expand Down Expand Up @@ -616,8 +625,12 @@ func (p *provider) Validate(ctx context.Context, _ *zap.SugaredLogger, spec clus
// If instancetype is specified, skip CPU and Memory validation.
// Values will come from instancetype.
if c.Instancetype == nil {
if _, err := parseResources(c.CPUs, c.Memory); err != nil {
return err
if c.Resources == nil {
return fmt.Errorf("no memory requests set for the virtual machine")
}

if (c.VCPUs == nil && c.Resources.Cpu() == nil) || (c.VCPUs != nil && c.Resources.Cpu() != nil) {
return fmt.Errorf("invalid/no cpus configured. Either vpus or cpus have to be configured.")
Copy link
Member

Choose a reason for hiding this comment

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

The error message here is quite misleading since in cases where both vCPU and CPU are set, you are still returning the same message. Please split this into two different if blocks or be a bit more coherent with your error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I separated both conditions and changed the return message to be a bit more clearer.

}
}

Expand Down Expand Up @@ -753,12 +766,8 @@ func (p *provider) newVirtualMachine(c *Config, pc *providerconfig.Config, machi

// if no instancetype, resources are from config.
if c.Instancetype == nil {
requestsAndLimits, err := parseResources(c.CPUs, c.Memory)
if err != nil {
return nil, err
}
resourceRequirements.Requests = *requestsAndLimits
resourceRequirements.Limits = *requestsAndLimits
resourceRequirements.Requests = *c.Resources
resourceRequirements.Limits = *c.Resources
}

// Add cluster labels
Expand Down Expand Up @@ -840,6 +849,13 @@ func (p *provider) newVirtualMachine(c *Config, pc *providerconfig.Config, machi
DataVolumeTemplates: getDataVolumeTemplates(c, dataVolumeName, dvAnnotations),
},
}

if c.VCPUs != nil {
virtualMachine.Spec.Template.Spec.Domain.CPU = &kubevirtcorev1.CPU{
Cores: c.VCPUs.Cores,
}
}

return virtualMachine, nil
}

Expand Down Expand Up @@ -867,19 +883,25 @@ func (p *provider) Cleanup(ctx context.Context, _ *zap.SugaredLogger, machine *c
return false, sigClient.Delete(ctx, vm)
}

func parseResources(cpus, memory string) (*corev1.ResourceList, error) {
func parseResources(cpus, memory string, vpcus kubevirttypes.VCPUs) (*corev1.ResourceList, *kubevirtcorev1.CPU, error) {
memoryResource, err := resource.ParseQuantity(memory)
if err != nil {
return nil, fmt.Errorf("failed to parse memory requests: %w", err)
return nil, nil, fmt.Errorf("failed to parse memory requests: %w", err)
}

if vpcus.Cores != 0 {
return &corev1.ResourceList{corev1.ResourceMemory: memoryResource}, &kubevirtcorev1.CPU{Cores: uint32(vpcus.Cores)}, nil
}

cpuResource, err := resource.ParseQuantity(cpus)
if err != nil {
return nil, fmt.Errorf("failed to parse cpu request: %w", err)
return nil, nil, fmt.Errorf("failed to parse cpu requests: %w", err)
}

return &corev1.ResourceList{
corev1.ResourceMemory: memoryResource,
corev1.ResourceCPU: cpuResource,
}, nil
}, nil, nil
}

func (p *provider) SetMetricsForMachines(_ clusterv1alpha1.MachineList) error {
Expand Down
11 changes: 11 additions & 0 deletions pkg/cloudprovider/provider/kubevirt/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type kubevirtProviderSpecConf struct {
ProviderNetwork *kubevirt.ProviderNetwork
ExtraHeadersSet bool
EvictStrategy string
VCPUs uint32
}

func (k kubevirtProviderSpecConf) rawProviderSpec(t *testing.T) []byte {
Expand Down Expand Up @@ -132,7 +133,13 @@ func (k kubevirtProviderSpecConf) rawProviderSpec(t *testing.T) []byte {
},
{{- end }}
"template": {
{{- if .VCPUs }}
"vcpus": {
"cores": {{ .VCPUs }}
},
{{- else }}
"cpus": "2",
{{- end }}
"memory": "2Gi",
{{- if .SecondaryDisks }}
"secondaryDisks": [{
Expand Down Expand Up @@ -283,6 +290,10 @@ func TestNewVirtualMachine(t *testing.T) {
name: "eviction-strategy-live-migrate",
specConf: kubevirtProviderSpecConf{EvictStrategy: "LiveMigrate"},
},
{
name: "dedicated-vcpus",
specConf: kubevirtProviderSpecConf{VCPUs: 2},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
80 changes: 80 additions & 0 deletions pkg/cloudprovider/provider/kubevirt/testdata/dedicated-vcpus.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
apiVersion: kubevirt.io/v1
kind: VirtualMachine
metadata:
annotations:
labels:
cluster.x-k8s.io/cluster-name: cluster-name
cluster.x-k8s.io/role: worker
kubevirt.io/vm: dedicated-vcpus
md: md-name
name: dedicated-vcpus
namespace: test-namespace
spec:
dataVolumeTemplates:
- metadata:
name: dedicated-vcpus
spec:
storage:
accessModes:
- ReadWriteMany
resources:
requests:
storage: 10Gi
storageClassName: longhorn
source:
http:
url: http://x.y.z.t/ubuntu.img
runStrategy: Once
template:
metadata:
creationTimestamp: null
annotations:
"kubevirt.io/allow-pod-bridge-network-live-migration": "true"
"ovn.kubernetes.io/allow_live_migration": "true"
labels:
cluster.x-k8s.io/cluster-name: cluster-name
cluster.x-k8s.io/role: worker
kubevirt.io/vm: dedicated-vcpus
md: md-name
spec:
affinity: {}
domain:
cpu:
cores: 2
devices:
disks:
- disk:
bus: virtio
name: datavolumedisk
- disk:
bus: virtio
name: cloudinitdisk
interfaces:
- name: default
bridge: {}
networkInterfaceMultiqueue: true
resources:
limits:
memory: 2Gi
requests:
memory: 2Gi
networks:
- name: default
pod: {}
terminationGracePeriodSeconds: 30
topologyspreadconstraints:
- maxskew: 1
topologykey: kubernetes.io/hostname
whenunsatisfiable: ScheduleAnyway
labelselector:
matchlabels:
md: md-name
volumes:
- dataVolume:
name: dedicated-vcpus
name: datavolumedisk
- cloudInitNoCloud:
secretRef:
name: udsn
name: cloudinitdisk
evictionStrategy: External
8 changes: 8 additions & 0 deletions sdk/cloudprovider/kubevirt/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,20 @@ type Flavor struct {

// Template.
type Template struct {
VCPUs VCPUs `json:"vcpus,omitempty"`
CPUs providerconfig.ConfigVarString `json:"cpus,omitempty"`
Memory providerconfig.ConfigVarString `json:"memory,omitempty"`
PrimaryDisk PrimaryDisk `json:"primaryDisk,omitempty"`
SecondaryDisks []SecondaryDisks `json:"secondaryDisks,omitempty"`
}

// VCPUs.
type VCPUs struct {
Cores int `json:"cores,omitempty"`
Sockets int `json:"sockets,omitempty"`
Threads int `json:"threads,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Both Sockets and Threads are unused. We should either remove these variables or utilize them at

return &corev1.ResourceList{corev1.ResourceMemory: memoryResource}, &kubevirtcorev1.CPU{Cores: uint32(vpcus.Cores)}, nil
and
virtualMachine.Spec.Template.Spec.Domain.CPU = &kubevirtcorev1.CPU{
Cores: c.VCPUs.Cores,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the unused values and only left cores for now.

}

// PrimaryDisk.
type PrimaryDisk struct {
Disk
Expand Down