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.

Sign up for GitHub

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

Jump to bottom

feat(virtctl): Allow to infer from specified volume in create vm #9631

Merged
merged 2 commits into from
May 3, 2023
Merged

feat(virtctl): Allow to infer from specified volume in create vm #9631

merged 2 commits into from
May 3, 2023

Conversation

0xFelix
Copy link
Member

@0xFelix 0xFelix commented Apr 19, 2023

What this PR does / why we need it:

This changes the --infer-instancetype and --infer-preference flags to
take optional strings which allow to specify the volume of the VM from
which the instancetype or preference should be inferred from. If no
string is passed the previous behaviour is kept (infer from first boot
disk or first volume).

Special notes for your reviewer:

This is based on #9629, merge after.

Release note:

virtctl: Allow to infer instancetype or preference from specified volume when creating VMs

@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. size/L labels Apr 19, 2023
@0xFelix 0xFelix changed the title Virtctl create vm infer name feat(virtctl): Allow to infer from specified volume in create vm Apr 19, 2023
@0xFelix 0xFelix force-pushed the virtctl-create-vm-infer-name branch from cfd12f6 to 4d6db2b Compare April 19, 2023 09:19
@0xFelix
Copy link
Member Author

0xFelix commented Apr 19, 2023

/cc @lyarwood

@0xFelix 0xFelix mentioned this pull request Apr 19, 2023
Merged
@fabiand
Copy link
Member

fabiand commented Apr 24, 2023

Hi. Does this mean that the IT / Pref would be infered on the client side?

@lyarwood
Copy link
Member

lyarwood commented Apr 24, 2023 •
edited

Hi. Does this mean that the IT / Pref would be infered on the client side?

Nope, that part remains on the backend, this is just populating the {Instancetype,Preference}Matcher with a volume name.

@0xFelix 0xFelix force-pushed the virtctl-create-vm-infer-name branch 3 times, most recently from 2992b1e to 81b4f09 Compare April 25, 2023 07:00
@0xFelix
Copy link
Member Author

0xFelix commented Apr 25, 2023

@fabiand Lee is right, this makes use of the backend side lookup. IMO it is best to keep virtctl as simple as possible. WDYT?

alicefr
alicefr reviewed Apr 25, 2023
View reviewed changes
pkg/virtctl/create/vm/vm.go Outdated
@@ -163,11 +165,13 @@ func NewCommand() *cobra.Command {
cmd.Flags().Int64Var(&c.terminationGracePeriod, TerminationGracePeriodFlag, c.terminationGracePeriod, "Specify the termination grace period of the VM.")

cmd.Flags().StringVar(&c.instancetype, InstancetypeFlag, c.instancetype, "Specify the Instance Type of the VM.")
cmd.Flags().BoolVar(&c.inferInstancetype, InferInstancetypeFlag, c.inferInstancetype, "Specify that the Instance Type of the VM is inferred from the booted volume.")
cmd.Flags().StringVar(&c.inferInstancetype, InferInstancetypeFlag, c.inferInstancetype, "Specify that the Instance Type of the VM should be inferred. Optionall specify the volume to infer from.")
Copy link
Member

Choose a reason for hiding this comment

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

typo: Optionall -> Optionally

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@0xFelix 0xFelix force-pushed the virtctl-create-vm-infer-name branch from 81b4f09 to c59cf09 Compare April 25, 2023 09:23
@0xFelix
cleanup(virtctl): Add NoArgs validator to create vm cmd
b0e4647 
No positional arguments to this command are expected so the
appropriate validator is added.

Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
@0xFelix
feat(virtctl): Allow to infer from specified volume in create vm
ed7869b 
This changes the --infer-instancetype and --infer-preference flags to
take optional strings which allow to specify the volume of the VM from
which the instancetype or preference should be inferred from. If no
string is passed the previous behaviour is kept (infer from first boot
disk or first volume).

Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
@0xFelix 0xFelix force-pushed the virtctl-create-vm-infer-name branch from c59cf09 to ed7869b Compare April 26, 2023 07:26
@0xFelix
Copy link
Member Author

0xFelix commented Apr 27, 2023

/retest

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Apr 27, 2023 •
edited

@0xFelix: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-fossa ed7869b link false /test pull-kubevirt-fossa

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. I understand the commands that are listed here.

@opokornyy
Copy link
Contributor

I think it looks good, thanks Felix!

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2023
@alicefr
Copy link
Member

alicefr commented Apr 28, 2023

/approve

1 similar comment
@alicefr
Copy link
Member

alicefr commented May 2, 2023

/approve

@alicefr
Copy link
Member

alicefr commented May 3, 2023

/approve

Hopefully, it should now work

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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 May 3, 2023
@kubevirt-commenter-bot
Copy link

/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
Copy link

/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 kubevirt-bot merged commit 8909538 into kubevirt:main May 3, 2023
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alicefr alicefr alicefr left review comments

@acardace acardace Awaiting requested review from acardace

@kbidarkar kbidarkar Awaiting requested review from kbidarkar

@lyarwood lyarwood Awaiting requested review from lyarwood

Assignees

@opokornyy opokornyy

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/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@0xFelix @fabiand @lyarwood @kubevirt-bot @opokornyy @alicefr @kubevirt-commenter-bot