-
Notifications
You must be signed in to change notification settings - Fork 2
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
[traffic-gen-in-vm] Refactor VMI creation #147
[traffic-gen-in-vm] Refactor VMI creation #147
Conversation
Currently, the `vmi.go` file in the `vmi` package, contains code that does mostly VMI spec building. Follow-up commits will move all non-spec building logic away from this file. Its test file was renamed as well. Signed-off-by: Orel Misan <[email protected]>
Currently, the code that creates the two custom VMIs is located in `checkup.go`. Add a new `vmi.go` file in the `checkup` package, that will contain all the logic related to creating customized VMIs. Signed-off-by: Orel Misan <[email protected]>
Currently, both VMI under test and traffic-gen are built by the shared `NewDPDKVMI` function. In an effort to move all checkup-specific logic away from `spec.go`, Move the `NewDPDKVMI` function and its accompanying DPDKVMIConfig struct, to the `vmi.go` file in the `checkup` package. Also move the `CloudInit` and `RandomizeName` functions. Signed-off-by: Orel Misan <[email protected]>
In order to move the `Affinity` function in the following commit, Export the `newRequiredNodeAffinity` and `newPreferredPodAntiAffinity` functions. Signed-off-by: Orel Misan <[email protected]>
Currently, the affinity calculation is located under the `vmi` package. The above code is checkup-specific, and could be moved to the `checkup` package, along with the other check-specific code. Signed-off-by: Orel Misan <[email protected]>
The DPDKCheckupUIDLabelKey constant is checkup-specific, move it to the `checkup` package. Signed-off-by: Orel Misan <[email protected]>
In order to separate the build of the two VMIs in a follow-up commit, move the shared constants to a higher scope. Signed-off-by: Orel Misan <[email protected]>
Tested against an OpenShift Virtualization 4.14 cluster. |
7f28215
to
1b631cb
Compare
Change: Updated commit message. |
1b631cb
to
b2c76d2
Compare
Change: Refactored last commit so it will be better suited for the follow-up PR. |
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.
maybe spec.go-->vmispec.go or something the like?
.golangci.yml
Outdated
@@ -3,8 +3,6 @@ | |||
linters-settings: | |||
gofmt: | |||
simplify: false | |||
dupl: |
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 you insert this in a separate PR?
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.
Sure.
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 implementing your suggestion, this change is no longer needed.
pkg/internal/checkup/vmi.go
Outdated
Username: config.VMIUsername, | ||
Password: config.VMIPassword, | ||
} | ||
vmiUnderTest := newBaseVMI(checkupConfig) |
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 seems redundant to iterate over the options in two places (vmi.New
and applyOptionsToVMI
).
How about creating a function baseVMIOptions and then something like this?
baseOptions := baseVMIOptions(checkupConfig)
vmiunderTestCustomOptions := []vmi.Option{
vmi.WithAffinity(Affinity(checkupConfig.DPDKNodeLabelSelector, checkupConfig.PodUID)),
vmi.WithSRIOVInterface(eastNetworkName, checkupConfig.DPDKEastMacAddress.String(), config.VMIEastNICPCIAddress),
vmi.WithSRIOVInterface(westNetworkName, checkupConfig.DPDKWestMacAddress.String(), config.VMIWestNICPCIAddress),
vmi.WithContainerDisk(rootDiskName, checkupConfig.VMContainerDiskImage),
vmi.WithCloudInitNoCloudVolume(cloudInitDiskName, CloudInit(config.VMIUsername, config.VMIPassword)
}
return vmi.New(RandomizeName(VMIUnderTestNamePrefix),baseOptions, vmiunderTestCustomOptions)
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 idea.
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.
Done.
It really improved the code.
Thank you.
It is already under the A name that I don't like is |
Currently, the two VMIs are built using the same basic spec, but with different parameters: 1. VMI name. 2. Affinity settings. 3. ContainerDisk image name. 4. NIC{1..2} MAC and PCI addresses. The traffic gen VMI, needs to have additional configuration in order to receive the TRex configuration using a ConfigMap object. Separate the build of the two VMIs. Use a base spec that is identical between the VMIs. Signed-off-by: Orel Misan <[email protected]>
Currently, the two VMIs are given a different random suffix. Also, the traffic gen ConfigMap object has no suffix, which prevents running more than one instance of the checkup in a single namespace. Give the two VMIs and the traffic gen ConfigMap the same random suffix, so it will be easier to identify then in a glance. Signed-off-by: Orel Misan <[email protected]>
b2c76d2
to
a086020
Compare
Change: Answered review comments. |
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
Currently, the two VMIs are built using the same basic spec, but with different parameters, such as:
In order to enable the traffic gen VMI to receive TRex configuration files using a ConfigMap, the traffic gen VMI spec
should receive additional parameters (and be independent from the VMI under test).
Move all high-level VMI building logic to the
vmi.go
file under thecheckup
package.Keep the low-level VMI spec building logic under the
checkup/vmi
package.Add an identical random suffix to all created objects: