Add checks of dedicatedCPUPlacement to validation webhook #8886

New issue

Jump to bottom

Merged

kubevirt-bot merged 1 commit into kubevirt:main from opokornyy:dedicatedCPUPlacement

Dec 13, 2022

+122 −52

Contributor

opokornyy commented Dec 1, 2022 •

edited

Loading

What this PR does / why we need it:

Setting dedicatedCPUPLacement is not supported at the moment and causes VMI to fail during start. This PR makes sure that instance type with dedicatedCPUPLacement is rejected by validation webhook.

Which issue(s) this PR fixes :
Fixes #8867

Release note:

Bug #8867 has been fixed, for now the dedicatedCPUPlacementattribute is no longer supported within the VirtualMachineInstancetype and VirtualMachineClusterInstancetype CRDs until support for resource requests is introduced.

Signed-off-by: Ondrej Pokorny opokorny@redhat.com

kubevirt-bot added dco-signoff: yes do-not-merge/release-note-label-needed size/L labels

kubevirt-bot requested review from alicefr and vladikr

December 1, 2022 09:20

kubevirt-bot added release-note and removed do-not-merge/release-note-label-needed labels

Member

lyarwood commented Dec 1, 2022

/area instancetype
/cc @lyarwood

kubevirt-bot requested a review from lyarwood

December 1, 2022 09:59

kubevirt-bot added the area/instancetype label

lyarwood suggested changes

View reviewed changes

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter.go Outdated

+              	if instancetypeSpec.CPU.DedicatedCPUPlacement {
+              		return []metav1.StatusCause{{
+              			Type:    metav1.CauseTypeFieldValueInvalid,
+              			Message: "setting spec.resources.DedicatedCPUPlacement is now allowed",

Member

lyarwood Dec 1, 2022

dedicatedCPUPlacement is not currently supported

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter.go Outdated

+              		return []metav1.StatusCause{{
+              			Type:    metav1.CauseTypeFieldValueInvalid,
+              			Message: "setting spec.resources.DedicatedCPUPlacement is now allowed",
+              			Field:   k8sfield.NewPath("spec", "instancetype").String(),

Member

lyarwood Dec 1, 2022

k8sfield.NewPath("spec", "cpu", "dedicatedCPUPlacement").String()

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter.go

Comment on lines +113 to +115

+              	if ar.Request.Resource != instanceTypeResource {
+              		return fmt.Errorf("expected '%s' got '%s'", &instanceTypeResource, ar.Request.Resource)
+              	}

Member

lyarwood Dec 1, 2022

Do we have test coverage of this?

Contributor Author

opokornyy Dec 1, 2022

There is just one test, that is testing unsupported version of ar.Request.Resource. I will add more tests to check also unsupported group and resource.

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter.go Outdated

               	return &admissionv1.AdmissionResponse{
               		Allowed: true,
               	}
               }
-              type ClusterInstancetypeAdmitter struct{}
+              func GetInstanceTypeFromAdmissionReview(ar *admissionv1.AdmissionReview) (*instancetypev1alpha2.VirtualMachineInstancetype, error) {

Member

lyarwood Dec 1, 2022

getInstanceTypeFromAdmissionReview

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter.go Outdated

               	return &admissionv1.AdmissionResponse{
               		Allowed: true,
               	}
               }
+              func GetClusterInstanceTypeFromAdmissionReview(ar *admissionv1.AdmissionReview) (*instancetypev1alpha2.VirtualMachineClusterInstancetype, error) {

Member

lyarwood Dec 1, 2022

getClusterInstanceTypeFromAdmissionReview

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter.go Outdated

+              	return &newInstanceType, nil
+              }
+              func ValidateRequestResource(ar *admissionv1.AdmissionReview, resourceType string) error {

Member

lyarwood Dec 1, 2022

validateRequestResource

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter.go Outdated

+              	return nil
+              }
+              func ValidateDedicatedCPUPlacement(instancetypeSpec *instancetypev1alpha2.VirtualMachineInstancetypeSpec) []metav1.StatusCause {

Member

lyarwood Dec 1, 2022

validateDedicatedCPUPlacement

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter_test.go Outdated

+              		ar := createInstancetypeAdmissionReview(instancetypeObj)
+              		response := admitter.Admit(ar)
+              		Expect(response.Allowed).To(BeFalse(), "Expected instancetype to not be allowed")

Member

lyarwood Dec 1, 2022

nit - Expect

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter_test.go Outdated

Comment on lines 53 to 56

+              			ObjectMeta: metav1.ObjectMeta{
+              				Name:      "test-name",
+              				Namespace: "test-namespace",
+              			},

Member

lyarwood Dec 1, 2022

BeforeEach already provides a basic InstancetypeObj with this set, all you need to do here is update the spec.

instancetypeObj.Spec = instancetypev1alpha2.VirtualMachineInstancetypeSpec{
	CPU: instancetypev1alpha2.CPUInstancetype{
		DedicatedCPUPlacement: true,
	},
}

pkg/virt-api/webhooks/validating-webhook/admitters/instancetype-admitter_test.go Outdated

               		Expect(response.Allowed).To(BeFalse(), "Expected instancetype to not be allowed")
               		Expect(response.Result.Code).To(Equal(int32(http.StatusBadRequest)), "Expected error 400: BadRequest")
               	})
+              	It("should reject instanceType with dedicatedCPUPlacement", func() {

Member

lyarwood Dec 1, 2022

supernit - instancetype

kubevirt-bot assigned lyarwood


          Add checks of dedicatedCPUPlacement to validation webhook

48a2ade

Setting dedicatedCPUPLacement is not supported at the
moment and causes VMI to fail during start. This PR
makes sure that instance type with dedicatedCPUPLacement
is rejected by validation webhook

Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>

opokornyy force-pushed the dedicatedCPUPlacement branch from 4907260 to 48a2ade Compare

December 1, 2022 13:16

lyarwood approved these changes

View reviewed changes

Member

lyarwood left a comment •

edited

Loading

/lgtm

What do you think about using the following as the release note?

Bug #8867 has been fixed, for now the `dedicatedCPUPlacement` attribute is no longer supported within the `VirtualMachineInstancetype` and `VirtualMachineClusterInstancetype` CRDs until support for resource requests is introduced.`

kubevirt-bot added the lgtm label

Member

lyarwood commented Dec 1, 2022

/retest

2 similar comments

Contributor Author

opokornyy commented Dec 2, 2022

/retest

Contributor Author

opokornyy commented Dec 3, 2022

/retest

Member

ksimon1 commented Dec 6, 2022

/lgtm
@xpivarc @enp0s3 can you please check this PR?

kubevirt-bot assigned ksimon1

Member

ksimon1 commented Dec 12, 2022

@davidvossel, @acardace, @vladikr can you please check this PR?

xpivarc reviewed

View reviewed changes

Member

xpivarc left a comment

/approve
I would personally split the changes into two commits, functional change and refactoring. It is then easier to review. This is just a suggestion for the future.

Contributor

kubevirt-bot commented Dec 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xpivarc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-bot added the approved label

kubevirt-commenter-bot commented Dec 13, 2022

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment

kubevirt-commenter-bot commented Dec 13, 2022

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot merged commit a7a69d5 into kubevirt:main

Contributor Author

opokornyy commented Dec 13, 2022

/cherrypick release-0.58

kubevirt-bot mentioned this pull request

[release-0.58] Add checks of dedicatedCPUPlacement to validation webhook #8938

Closed

Contributor

kubevirt-bot commented Dec 13, 2022

@opokornyy: new pull request created: #8938

In response to this:

/cherrypick release-0.58

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lyarwood added a commit to lyarwood/common-instancetypes that referenced this pull request


          Drop use of dedicatedCPUPlacement

2c29446

A recent change to KubeVirt [1][2] has essentially disabled the use of
dedicatedCPUPlacement as we have no way of expressing the required
resource requirements through instance types at present.

Until this is corrected all requests to create instance types with this
attribute will be rejected. This change drops the use of this attribute
until this is resolved.

[1] kubevirt/kubevirt#8886
[2] kubevirt/kubevirt#8867

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

lyarwood mentioned this pull request

Drop use of dedicatedCPUPlacement kubevirt/common-instancetypes#20

Merged

lyarwood added a commit to kubevirt/common-instancetypes that referenced this pull request


          Drop use of dedicatedCPUPlacement

d929205

A recent change to KubeVirt [1][2] has essentially disabled the use of
dedicatedCPUPlacement as we have no way of expressing the required
resource requirements through instance types at present.

Until this is corrected all requests to create instance types with this
attribute will be rejected. This change drops the use of this attribute
until this is resolved.

[1] kubevirt/kubevirt#8886
[2] kubevirt/kubevirt#8867

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

lyarwood mentioned this pull request

common-instancetypes: Update to v0.0.2-rc kubevirt/ssp-operator#464

Merged

lyarwood added a commit to lyarwood/common-instancetypes that referenced this pull request


          CX: Remove guestMappingPassthrough

3151fc7

Instance types using dedicatedCPUPlacement have recently started to be
rejected by virt-api [1] as we currently have no way of expressing the
required resource requests within an instance type.

dedicatedCPUPlacement is also a requirement when using
guestMappingPassthrough and as such this also needs to be removed from
our shipped instance types ahead of a similar check being introduced
[2] in virt-api.

[1] kubevirt/kubevirt#8886
[2] kubevirt/kubevirt#9105

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

lyarwood mentioned this pull request

CX: Remove guestMappingPassthrough kubevirt/common-instancetypes#25

Merged

kubevirt-bot pushed a commit to kubevirt/common-instancetypes that referenced this pull request


          CX: Remove guestMappingPassthrough (#25)

a3e02b8

Instance types using dedicatedCPUPlacement have recently started to be
rejected by virt-api [1] as we currently have no way of expressing the
required resource requests within an instance type.

dedicatedCPUPlacement is also a requirement when using
guestMappingPassthrough and as such this also needs to be removed from
our shipped instance types ahead of a similar check being introduced
[2] in virt-api.

[1] kubevirt/kubevirt#8886
[2] kubevirt/kubevirt#9105

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

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

Labels

approved area/instancetype dco-signoff: yes lgtm release-note size/L