Skip to content
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

Introduce preference resource requirements #58

Merged

Conversation

lyarwood
Copy link
Member

@lyarwood lyarwood commented Jun 15, 2023

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #55, fixes #51

Special notes for your reviewer:

Based on #57

Release note:

Resource requirements have been added to each of the supplied preference resources.

@lyarwood lyarwood added this to the v0.3.0 milestone Jun 15, 2023
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 15, 2023
@lyarwood lyarwood force-pushed the introduce-preference-resource-requirements branch from 92f3108 to 3142bf1 Compare June 15, 2023 19:25
@lyarwood
Copy link
Member Author

/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2023
@lyarwood lyarwood force-pushed the introduce-preference-resource-requirements branch from 3142bf1 to 6abe6cb Compare June 16, 2023 08:25
@lyarwood
Copy link
Member Author

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2023
@lyarwood
Copy link
Member Author

/cc @0xFelix

@ksimon1
Copy link
Member

ksimon1 commented Jun 16, 2023

How will you make sure the requirement are up to date?

@lyarwood
Copy link
Member Author

How will you make sure the requirement are up to date?

Great question, I had forgotten to document #59 and a move to osinfo in the future as with common-templates. I think hard coded values are fine for now however given the support status downstream and the fact these aren't deployed as yet by default upstream.

@ksimon1
Copy link
Member

ksimon1 commented Jun 16, 2023

/lgtm
For now, the hard coded values are ok

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2023
@lyarwood
Copy link
Member Author

/hold

As this is based on #57

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2023
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

Thanks!

/approve
/lgtm

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2023
@lyarwood
Copy link
Member Author

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 20, 2023
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
Given that all preferences now have resource requirements we need to
actually provide adequate resources to the VirtualMachine.

A negative test is also added for each preference ensuring that requests
to create a VirtualMachine using an instance type that doesn't provide
enough resources are rejected.

Additionally the use of virtctl is introduced, removing a long standing
TODO from both the preference and instance type tests.

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood lyarwood force-pushed the introduce-preference-resource-requirements branch from 6abe6cb to 37dbb7e Compare June 20, 2023 10:47
@kubevirt-bot kubevirt-bot added size/XL and removed lgtm Indicates that a PR is ready to be merged. size/XXL labels Jun 20, 2023
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023
@kubevirt-bot kubevirt-bot merged commit 07b47d6 into kubevirt:main Jun 20, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add minimum resource requirements to each preference Resolve TODOs in functest.sh
4 participants