-
Notifications
You must be signed in to change notification settings - Fork 64
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
Use nocloud when specified #212
Conversation
Pull Request Test Coverage Report for Build 3654098425
💛 - Coveralls |
d9ac625
to
bbffc37
Compare
/ok-to-test |
pkg/kubevirt/utils.go
Outdated
CDRom: &kubevirtv1.CDRomTarget{ | ||
Bus: kubevirtv1.DiskBusSATA, | ||
ReadOnly: func() *bool { | ||
b := true | ||
return &b | ||
}(), | ||
}, |
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.
is there a reason why noCloud should use a cdrom instead of a disk with virtio?
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.
To some tests the CDROM type appears to work better with Talos
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.
How about we default to the disk
approach that we already have today, but only create that disk if there's not already a disk with the name cloudinitvolume
So this would allow you to create a disk however you want and our controllers won't touch it. But if the cloudinit disk doesn't exist, our controllers will give one as a default.
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.
I tried it with Disk again and it appears to work fine.
I like the flow of providing a disk/volume with a given name and it fills in the rest using your changes on top.
Made it so it adds to if it doesn't already exist 57d40f5
pkg/kubevirt/utils.go
Outdated
Bus: "virtio", | ||
}, | ||
}, | ||
cloudInitType, index := noCloudVolume(template) |
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.
instead of calling this function noCloudVolume
, could it instead be something like, detectCloudInitVolume
pkg/kubevirt/utils.go
Outdated
cloudInitType, index := noCloudVolume(template) | ||
cloudInitVolume := cloudinitVolume(template, ctx, cloudInitType) | ||
cloudInitDisk := cloudinitDisk(template, cloudInitType) | ||
if cloudInitType == "CloudInitNoCloud" { |
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.
instead of keying off the "CloudInitNoCloud" type here, could we instead detect if a previous cloudinit volume existed and use that index... So maybe have the detectCloudInitVolume
function return either an index >= 0 if a volume already exists, or maybe -1 if no previous volume exists. Then with this if/def we'd just have if index >= 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.
I think while that would make it less, I believe that would require extra explaining and documentation.
It might be clearer with outputs like exists
and index
.
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 any case, the goal is to have it default to CloudInitConfigDrive, unless there's an non-nil CloudInitNoCloud volume named cloudinitvolume
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.
yeah, an exists
bool with an index
makes sense as well.
I think i'd rather avoid detection logic that keys for the CloudInitNoCloud
type directly, and instead have one that simply returns the cloud init type found in the volume. Right now we only have CloudInitNoCloud, and ConfigDrive, but in the future there might be more options. It would be nice if this logic naturally worked if the cloud init types are added to in the future
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.
Using a different method based on the type detected, such as CloudInitNoCloud or CloudInitConfigDrive, will future proof it more than detecting only CloudInitNoCloud.
I'm sure there's some way to add more support another way
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
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BobyMCbobs, davidvossel 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 |
/lgtm cancel |
@BobyMCbobs the PR looks good now, but can you check why the go-linter unit test is failing? |
pkg/kubevirt/utils.go
Outdated
if err != nil { | ||
panic(err) | ||
} | ||
break |
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.
Please remove. Not needed on golang. It will aldo solve the linter warning.
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.
Updated in e58acaa
pkg/kubevirt/utils.go
Outdated
break | ||
case "CloudInitConfigDrive": | ||
template.Spec.Volumes = append(template.Spec.Volumes, cloudInitVolume) | ||
break |
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
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.
Updated in e58acaa
pkg/kubevirt/utils.go
Outdated
case "CloudInitNoCloud": | ||
err := mergo.Merge(&template.Spec.Volumes[index], cloudInitVolume) | ||
if err != nil { | ||
panic(err) |
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.
Panic will kill the whole process. Are uou sure this is the behaviour we want here?
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.
buildVirtualMachineInstanceTemplate doesn't return an error.
Which behaviour would you suggest here?
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.
I'm unsure how the behaviour would be if it were unhandled, like
_ = mergo.Merge(&template.Spec.Volumes[index], cloudInitVolume)
but perhaps that's an option?
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.
After reading the code of mergo.Merge
, I think it's too risky to ignore the error. I'm afraid there is no other option but change the signature of the buildVirtualMachineInstanceTemplate
function, to add an error return value, and to do the same for the calling function, newVirtualMachineFromKubevirtMachine
, then check this error in the Machine::Create
methods (that does return error). These functions are also called from unit tests, so there is no choice but fixing these test as well.
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.
Fair evaluation!
Updated in bb66a2d
@davidvossel, @nunnatsa looking for 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.
Thank for the fixes. I added few more comments...
Also, a general comment for the whole PR: the "CloudInitConfigDrive"
and "CloudInitNoCloud"
strings are only used to get logic decisions. Not sure string is the best option for that. Please consider using integer constants with iota
. For example:
type cloudInitType int
const (
cloudInitConfigDrive cloudInitType = iota
cloudInitNoCloud
)
pkg/kubevirt/machine_test.go
Outdated
@@ -309,7 +309,8 @@ var _ = Describe("util functions", func() { | |||
machineContext.KubevirtMachine.Spec.VirtualMachineTemplate.Spec.DataVolumeTemplates = dataVolumeTemplates | |||
machineContext.KubevirtMachine.Spec.VirtualMachineTemplate.Spec.Template.Spec.Volumes = volumes | |||
|
|||
newVM := newVirtualMachineFromKubevirtMachine(machineContext, "default") | |||
newVM, err := newVirtualMachineFromKubevirtMachine(machineContext, "default") | |||
Expect(err).To(BeNil()) |
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.
Nit: Please consider using the dedicated gomega error assertion.
Expect(err).To(BeNil()) | |
Expect(err).ToNot(HaveOccurred()) |
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.
Updated in 6a8fd2b
pkg/kubevirt/machine_test.go
Outdated
@@ -453,7 +454,8 @@ var _ = Describe("util functions", func() { | |||
machineContext.KubevirtMachine.Spec.VirtualMachineTemplate.Spec.DataVolumeTemplates = dataVolumeTemplates | |||
machineContext.KubevirtMachine.Spec.VirtualMachineTemplate.Spec.Template.Spec.Volumes = volumes | |||
|
|||
newVM := newVirtualMachineFromKubevirtMachine(machineContext, "default") | |||
newVM, err := newVirtualMachineFromKubevirtMachine(machineContext, "default") | |||
Expect(err).To(BeNil()) |
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.
Nit: Please consider using the dedicated gomega error assertion.
Expect(err).To(BeNil()) | |
Expect(err).ToNot(HaveOccurred()) |
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.
Updated in 6a8fd2b
pkg/kubevirt/utils.go
Outdated
func detectCloudInitVolumeType(vmi *kubevirtv1.VirtualMachineInstanceTemplateSpec) (preferredType string, index int) { | ||
preferredType = "CloudInitConfigDrive" | ||
for i, v := range vmi.Spec.Volumes { | ||
if v.CloudInitNoCloud != nil && v.Name == cloudInitVolumeName { | ||
preferredType, index = "CloudInitNoCloud", i | ||
break | ||
} | ||
} | ||
return preferredType, index | ||
} | ||
|
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.
Personally, I don't like named return values and I find them confusing. If there is no reason to use them, please consider to drop them.
Also, I think we could write a shorter, simpler version of this function. How do you feel about this suggestion?
func detectCloudInitVolumeType(vmi *kubevirtv1.VirtualMachineInstanceTemplateSpec) (preferredType string, index int) { | |
preferredType = "CloudInitConfigDrive" | |
for i, v := range vmi.Spec.Volumes { | |
if v.CloudInitNoCloud != nil && v.Name == cloudInitVolumeName { | |
preferredType, index = "CloudInitNoCloud", i | |
break | |
} | |
} | |
return preferredType, index | |
} | |
func detectCloudInitVolumeType(vmi *kubevirtv1.VirtualMachineInstanceTemplateSpec) (string, int) { | |
for i, v := range vmi.Spec.Volumes { | |
if v.CloudInitNoCloud != nil && v.Name == cloudInitVolumeName { | |
return "CloudInitNoCloud", i | |
} | |
} | |
return "CloudInitConfigDrive", 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.
The suggested function does look tidier, I've updated it to that in a621d12
Personally, I don't like named return values and I find them confusing. If there is no reason to use them, please consider to drop them.
The named return values make it clearing what is expected to be returned and I think are useful.
However, if you mean that the specific names are not helpful I would suggest preferredCloudInitVolumeType
and existingCloudInitVolumeIndex
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.
As I wrote, it's a personal preference. I'm not a fan of the named return values feature. But I'll won't block the PR for 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.
Updated in dc49cec
pkg/kubevirt/utils.go
Outdated
func detectCloudInitDisk(vmi *kubevirtv1.VirtualMachineInstanceTemplateSpec) (found bool, index int) { | ||
for i, v := range vmi.Spec.Domain.Devices.Disks { | ||
if v.Disk != nil && v.Name == cloudInitVolumeName { | ||
found, index = true, i | ||
break | ||
} | ||
} | ||
return constants.WorkerNodeRoleValue | ||
return found, index |
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.
Please consider removing the named return values. Also, the index return value is not in use and can be dropped. This function can be simplified a bit; for example:
func detectCloudInitDisk(vmi *kubevirtv1.VirtualMachineInstanceTemplateSpec) (found bool, index int) { | |
for i, v := range vmi.Spec.Domain.Devices.Disks { | |
if v.Disk != nil && v.Name == cloudInitVolumeName { | |
found, index = true, i | |
break | |
} | |
} | |
return constants.WorkerNodeRoleValue | |
return found, index | |
func detectCloudInitDisk(vmi *kubevirtv1.VirtualMachineInstanceTemplateSpec) bool { | |
for _, v := range vmi.Spec.Domain.Devices.Disks { | |
if v.Disk != nil && v.Name == cloudInitVolumeName { | |
return true | |
} | |
} | |
return false | |
} |
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.
Updated in acd5d10.
The named return values in the signature will be useful when reading documentation and for maintaining the functions in the future
pkg/kubevirt/utils.go
Outdated
func cloudinitVolume(vmi *kubevirtv1.VirtualMachineInstanceTemplateSpec, ctx *context.MachineContext, preferredType string) kubevirtv1.Volume { | ||
switch preferredType { | ||
case "CloudInitNoCloud": | ||
return kubevirtv1.Volume{ | ||
Name: cloudInitVolumeName, | ||
VolumeSource: kubevirtv1.VolumeSource{ | ||
CloudInitNoCloud: &kubevirtv1.CloudInitNoCloudSource{ | ||
UserDataSecretRef: &corev1.LocalObjectReference{ | ||
Name: *ctx.Machine.Spec.Bootstrap.DataSecretName + "-userdata", | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
default: | ||
return kubevirtv1.Volume{ | ||
Name: cloudInitVolumeName, | ||
VolumeSource: kubevirtv1.VolumeSource{ | ||
CloudInitConfigDrive: &kubevirtv1.CloudInitConfigDriveSource{ | ||
UserDataSecretRef: &corev1.LocalObjectReference{ | ||
Name: *ctx.Machine.Spec.Bootstrap.DataSecretName + "-userdata", | ||
}, | ||
}, | ||
}, | ||
} |
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.
Not sure about this function. Another way to implement this logic will be to split it to two functions, each generating one version of cloud init volume. Then use each one of them in the matching case in the switch sentence in the buildVirtualMachineInstanceTemplate
function above, where we already know the volume type.
The reason I'm not comfortable with the current implementation is that we ask the same question again and again (what is the volume type?), while we already know the answer.
@nunnatsa, thanks for your review.
I'm not sure why integer constants are preferred when strings provide immediate readability of what we're checking for. |
Integer constants are as readable as strings, I think. But comparing ints is much more efficient than comparing strings. Since the only usage of this values is for branches (if/switch), and there is no use at the actual value (e.g. print it out), then we can use the golang |
pkg/kubevirt/utils.go
Outdated
return "CloudInitNoCloud", i | ||
} | ||
} | ||
return "CloudInitConfigDrive", 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.
After thinking about it again, 0 is a valid slice index, and returning it is misleading, even if we don't actually using it in the case of CloudInitConfigDrive
. Let's change it to -1
, to be more clear about not finding the volume in the slice. What do you think?
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 thought! Updated in c2123db
I must admit I haven't used this feature before, but it seems to work fine just tested it. |
Currently experiencing a runtime panic, no experienced before recent changes
Update
|
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.
Looks great to me. I made one more comment inline, and my only other comment is to please squash the commits down to a single commit, or at most two commits, one for logic and one for the tests. We don't always require that, but 16 commits for this pr is a little excessive.
err := mergo.Merge(&template.Spec.Volumes[index], cloudInitVolume) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return template, nil |
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.
I don't think there is any value in the mergo logic here. There's only one possible solution for how the CloudIniitNoCloud volume source can be configured. All we need to do is preserve the volume name, and override whatever is in the CloudInitNoCloud volume source. Anything else can result in an unusable configuration.
This also removes the need to perform any error checking in this function.
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.
@davidvossel, this is the simpler method than doing a manual three-way merge.
The logic right now is, if there is a volume called "cloudinitvolume" with a non-nil cloudInitNoCloud volume source it will merge the existing one, which has whatever properties configured, with the one setting the same for the bootstrap secret name.
I'm happy to update if you are really sure. The team is requiring this feature as soon as it's possible.
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 should be all we need.
if index >= 0 {
template.Spec.Volumes[index] = cloudInitVolume
} else {
template.Spec.Domain.Devices.Disks = append(template.Spec.Domain.Devices.Disks, cloudInitDisk)
}
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.
I'm unsure whether that would work, the logic is stating to have either a volume added or a disk.
The current way things work means that
- you can bring your custom cloudInitNoCloud and we'll provide the secret, so that current behaviour is preserved but you can optionally use cloudInitNoCloud
- e.g: providing custom noCloud networkData
- a cloudInit disk will be only added if one doesn't already exist, so that you can specify the type of disk that it is (if I wanted CDROM instead)
and i think that is useful
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.
i'm not following how the mergo
usage is necessary to achieve what you previously replied with. I just recommending that the volume be overwritten if the index is known, or appended otherwise.
err := mergo.Merge(&template.Spec.Volumes[index], cloudInitVolume) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return template, nil |
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 should be all we need.
if index >= 0 {
template.Spec.Volumes[index] = cloudInitVolume
} else {
template.Spec.Domain.Devices.Disks = append(template.Spec.Domain.Devices.Disks, cloudInitDisk)
}
for i, v := range vmi.Spec.Volumes { | ||
if v.CloudInitNoCloud != nil && v.Name == cloudInitVolumeName { | ||
return cloudInitNoCloud, i | ||
} | ||
} | ||
return cloudInitConfigDrive, -1 |
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.
someone could theoretically set a CloudInitConfigDrive
volume too. I don't like that this logic is assuming that only a CloudInitNoCloud volume get's the special behavior.
making the default cloudInitConfigDrive is good though
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 was the way to provide difference, since there's only two types implement currently.
the logic is extend able to show that other cloudInit types are found. it can be adapted when those are implement
I'm comfortable merging this once the mergo logic is replaced. |
Thank you @davidvossel. |
What this PR does / why we need it:
Allows for CloudInitNoCloud volume type to be used and filled in when a volume is specified with the given type and the name "cloudinitvolume" and doesn't insert a CloudInitConfigDrive volume and disk.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #200Special notes for your reviewer:
Release notes: