-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add virtctl to tekton task #244
Conversation
b561e36
to
fecf87f
Compare
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.
Good start! I added some comments and one further question: Can the task return the name of the VM? This leaves it open to create a VM with a random name.
@@ -51,8 +50,8 @@ func main() { | |||
if err := vmCreator.OwnVolumes(vm); err != nil { | |||
exit.ExitFromError(OwnVolumesErrorExitCode, err) | |||
} | |||
runStrategy := cliOptions.GetRunStrategy() |
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.
Why drop this check?
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.
We can specify the RunStrategy
either through virtctl parameters or directly in a manifest. The runStrategy
checked in this condition is specified separately in the task spec. If we don't use the RunStrategy
parameter of a task the string will be empty and it will depend only on StartVM
flag. Also StartVM
should not perform any action if runStrategy
of a created VM is already set to Always. - https://kubevirt.io/user-guide/virtual_machines/run_strategies/#virtctl
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.
So this always tries to start the VM and mightt return an error from the API?
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.
It only tries to start the VM when StartVM
param is specified. It should not return error, when the VM is already running nothing happens.
modules/create-vm/go.mod
Outdated
go.uber.org/zap v1.19.1 | ||
k8s.io/api v0.25.2 | ||
k8s.io/apimachinery v0.25.2 | ||
k8s.io/client-go v12.0.0+incompatible | ||
kubevirt.io/api v0.59.0 | ||
kubevirt.io/client-go v0.59.0 | ||
kubevirt.io/api v0.60.0-alpha.0 |
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.
Can we stay on stable?
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.
Ondrej selected this tag, because there was something missing in 0.59. @opokornyy can you please confirm? This tag will be changed when v0.60 is officially released
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.
Yes, the stable tag does not contain all of the virtctl functionality that is needed.
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.
For the latest fixes you potentially want to pin the version to a specific commit. E.g. to include kubevirt/kubevirt#9651
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.
You might want to update this to v1.0.0-alpha.0
to have kubevirt/kubevirt#9651 included.
@@ -35,6 +36,7 @@ type CLIOptions struct { | |||
RunStrategy string `arg:"--run-strategy,env:RUN_STRATEGY" help:"Set run strategy to vm"` | |||
Output output.OutputType `arg:"-o" placeholder:"FORMAT" help:"Output format. One of: yaml|json"` | |||
Debug bool `arg:"--debug" help:"Sets DEBUG log level"` | |||
Virtctl string `arg:"--virtctl,env:VIRTCTL" placeholder:"VIRTCTL" help:"Specifies a virtctl create vm params to be used to create VirtualMachine."` |
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.
s/params/command/
?
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.
In this task param should be specified only the parameters, that will be used with the virtctl command like this for example --name vm-test --run-strategy Always
. So maybe it is beter to use params instead of a command?
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.
If you could improve the help text that's fine.
Entry("no mode", "one of vm-manifest, template-name should be specified", &parse.CLIOptions{}), | ||
Entry("both modes", "only one of vm-manifest, template-name should be specified", &parse.CLIOptions{ | ||
Entry("no mode", "only one of vm-manifest, template-name or virtctl should be specified", &parse.CLIOptions{}), | ||
Entry("both modes", "only one of vm-manifest, template-name or virtctl should be specified", &parse.CLIOptions{ |
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.
s/both/multiple/
?
@@ -21,9 +23,9 @@ func NewVirtualMachineProvider(client kubevirtcliv1.KubevirtClient) VirtualMachi | |||
} | |||
|
|||
func (v *virtualMachineProvider) Create(namespace string, vm *kubevirtv1.VirtualMachine) (*kubevirtv1.VirtualMachine, error) { | |||
return v.client.VirtualMachine(namespace).Create(vm) | |||
return v.client.VirtualMachine(namespace).Create(context.TODO(), vm) |
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.
Are we ever going to change the context? Might use context.Background()
?
namespace := v.targetNamespace | ||
if namespace == "" { | ||
if namespace, err = env.GetActiveNamespace(); err != nil { | ||
return nil, zerrors.NewMissingRequiredError("can't get active namespace: %v", err.Error()) |
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.
Same.
var vm kubevirtv1.VirtualMachine | ||
output := &bytes.Buffer{} | ||
|
||
cmd := createCommand(v.cliOptions.Virtctl, output) |
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.
Create command like it is done here? https://github.com/kubevirt/kubevirt/blob/3c25c8606e8aac1fcb31a73a3f444ebfffb27d20/tests/clientcmd/virtctl.go#L53
ExpectedLogs: ExpectedSuccessfulVMCreation, | ||
}, | ||
TaskData: testconfigs.CreateVMTaskData{ | ||
Virtctl: fmt.Sprintf("--name %s --run-strategy=Halted", vmName), |
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 same way to specify flags? Either with =
or space.
ExpectedLogs: ExpectedSuccessfulVMCreation, | ||
}, | ||
TaskData: testconfigs.CreateVMTaskData{ | ||
Virtctl: fmt.Sprintf("--name %s --run-strategy=Halted --instancetype %s", vmName, instancetypeName), |
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.
Same as above.
ExpectLogs(config.GetAllExpectedLogs()...) | ||
}) | ||
|
||
DescribeTable("should fail creating VM", func(config *testconfigs.CreateVMTestConfig) { |
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.
Isn't this more of a general test case that affects every creation mode? I would have placed it on a higher level.
Please improve commit messages and PR description. |
fecf87f
to
20b5e84
Compare
The task is just creating a VM and it does not return its name. It depends on a user if he specifies a name of a VM, if he does not for virtctl command, then the created VM will have a random name. |
@@ -51,8 +50,8 @@ func main() { | |||
if err := vmCreator.OwnVolumes(vm); err != nil { | |||
exit.ExitFromError(OwnVolumesErrorExitCode, err) | |||
} | |||
runStrategy := cliOptions.GetRunStrategy() |
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.
So this always tries to start the VM and mightt return an error from the API?
deleteInstancetype(f, instancetypeName) | ||
}) | ||
|
||
DescribeTable("should fail creating VM", func(config *testconfigs.CreateVMTestConfig) { |
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.
Merge this into taskrun fails and no VM is created
?
@opokornyy Tasks can have return values, so why not return the name/namespace of the created VM? Other tasks (e.g. creating DataVolumes) do the same. |
I have to correct myself here, the task already have a return values with name/namespace of the VM that was created. |
15634f5
to
dcd4263
Compare
/retest |
ExpectedLogs: ExpectedSuccessfulVMCreation, | ||
}, | ||
TaskData: testconfigs.CreateVMTaskData{ | ||
Virtctl: fmt.Sprintf("--name %s --run-strategy Halted", vmName), |
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 random name returned by task instead?
ExpectedLogs: ExpectedSuccessfulVMCreation, | ||
}, | ||
TaskData: testconfigs.CreateVMTaskData{ | ||
Virtctl: fmt.Sprintf("--name %s --instancetype %s", vmName, instancetypeName), |
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.
Same as above.
ExpectedLogs: ExpectedSuccessfulVMCreation, | ||
}, | ||
TaskData: testconfigs.CreateVMTaskData{ | ||
Virtctl: fmt.Sprintf("--name %s --run-strategy Halted --instancetype %s", vmName, instancetypeName), |
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.
Same as above.
/retest |
1 similar comment
/retest |
/retest |
1 similar comment
/retest |
if err != nil { | ||
|
||
if cliOptions.GetStartVMFlag() && | ||
(vm.Spec.RunStrategy == nil || *vm.Spec.RunStrategy != kubevirtv1.RunStrategyAlways) { |
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.
What happens if spec.running
is already true
, i.e. the VM is already running?
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.
It caused an error and should be fixed now.
modules/create-vm/go.mod
Outdated
go.uber.org/zap v1.19.1 | ||
k8s.io/api v0.25.2 | ||
k8s.io/apimachinery v0.25.2 | ||
k8s.io/client-go v12.0.0+incompatible | ||
kubevirt.io/api v0.59.0 | ||
kubevirt.io/client-go v0.59.0 | ||
kubevirt.io/api v0.60.0-alpha.0 |
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.
You might want to update this to v1.0.0-alpha.0
to have kubevirt/kubevirt#9651 included.
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
This commit adds functionality to create-vm-from-manifest-task to create vm with virtctl create vm command. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
/lgtm |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xFelix, opokornyy 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:
This PR adds functionality to create-vm-from-manifest task to allow user to create vm with virtctl command.
Release note: