-
Notifications
You must be signed in to change notification settings - Fork 17
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
vm spec will use the preference requirements in vm creation test #287
vm spec will use the preference requirements in vm creation test #287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will help verify the VM can boot using the preference requirements provided, especially important for cases when spread cpu topology is used
This test only creates a VM, it does NOT wait for it to be booted. I.e. this is only testing if VMs using these preferences can be created. There is no boot volume attached to the created VMs.
I see what you want to achieve now. Can you rename the existing test case to something like "verify common-instancetypes ships at least one instancetype providing enough resources for a given preference"? So in the end we have both test cases? |
I don't understand the name change you suggested, we don't verify any shipping of instancetypes in the test, if we want to verify the deployment of instancetypes, I think it should be done in a different test. Did you mean you want to create an extra test to verify we deployed the |
I want you to create an extra test with the proposed name or a similar one that keeps the behavior of the test case you are changing. In the end we should have two test cases, one that verifies the requirements provided by a preference are correct and a second one that verifies we ship at least one instancetype in common-instancetypes that provides sufficient resources to create a VM with a given preference. |
Signed-off-by: Roni Kishner<[email protected]>
9d92868
to
514805e
Compare
/retest |
/test ? |
@0xFelix: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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-sigs/prow repository. |
/test all |
/retest-required |
} | ||
|
||
vm = randomVM(&instanceTypeMatcher, &v1.PreferenceMatcher{Name: preference.Name}, v1.RunStrategyHalted) | ||
vm, err = virtClient.VirtualMachine(testNamespace).Create(context.Background(), vm, metav1.CreateOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use DryRunAll in CreateOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be better as a follow up given the current tests don't use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RoniKishner Can you create an issue/PR for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lyarwood 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 |
What this PR does / why we need it:
[test_id:10737] can be created when enough resources are provided
to[test_id:TODO] verifies all preferences have at least one compatible instance type
This will help verify the VM can be created using the preference requirements provided, especially important for cases when
spread
cpu topology is used.The old test naming wasn't expressing it's intention.
Release note: