Add support for user-defined network interface order (kvm only) #4452
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
requested review from
christoph-zededa,
eriknordmark,
OhmSpectator,
rene,
rouming,
rucoder,
shjala and
uncleDecart
milan-zededa
force-pushed
the
app-interface-order
branch
2 times, most recently
from
c1c26a9 to
5a74513
Compare
|
I'm going to ignore this yetus complain: Importing gomega directly makes tests more readable... |
milan-zededa
changed the title
|
Is it still a draft, or is it better to start reviewing? |
I left it in the draft state until we complete the end-to-end testing. |
OhmSpectator
reviewed
Dec 2, 2024
milan-zededa
force-pushed
the
app-interface-order
branch
from
5a74513 to
acd0dd7
Compare
milan-zededa
force-pushed
the
app-interface-order
branch
2 times, most recently
from
7ea1ee6 to
38c620c
Compare
OhmSpectator
reviewed
Dec 3, 2024
milan-zededa
force-pushed
the
app-interface-order
branch
from
38c620c to
8287291
Compare
OhmSpectator
reviewed
Dec 4, 2024
pkg/pillar/hypervisor/kvm.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (f *pciAssignmentsTemplateFiller) do(file io.Writer, pciAssignments []pciDevice, pciID int) error { | ||
| func (f *pciAssignmentsTemplateFiller) do(file io.Writer, pciAssignments []pciDevice) error { |
There was a problem hiding this comment.
@christoph-zededa, could you please take a look at the changes here as well? I'm looking at it now, but it would be nice if you have a look, too =)
milan-zededa
force-pushed
the
app-interface-order
branch
from
8287291 to
54683f2
Compare
OhmSpectator
reviewed
Dec 4, 2024
OhmSpectator
reviewed
Dec 4, 2024
OhmSpectator
reviewed
Dec 4, 2024
OhmSpectator
reviewed
Dec 4, 2024
OhmSpectator
approved these changes
Dec 4, 2024
There was a problem hiding this comment.
In general, it looks good to me. I hope the testing is successful.
Also, I would like @christoph-zededa and @rucoder to go through the code.
uncleDecart
reviewed
Dec 5, 2024
Adapter (direct assignment) and NetworkAdapter (VIF) now allow to configure the desired application network interface order. EVE ability to enforce the order is limited, however. Signed-off-by: Milan Lenco <[email protected]>
Move parsing of the templates into the init function (we only need to do it once) and declare contexts for templates as proper (not inline) types for better readability. Signed-off-by: Milan Lenco <[email protected]>
Parse newly added interface order fields and the EnforceNetworkInterfaceOrder boolean option. Signed-off-by: Milan Lenco <[email protected]>
milan-zededa
force-pushed
the
app-interface-order
branch
from
54683f2 to
1339c1e
Compare
OhmSpectator
approved these changes
Dec 5, 2024
milan-zededa
force-pushed
the
app-interface-order
branch
from
1339c1e to
c72827c
Compare
milan-zededa
force-pushed
the
app-interface-order
branch
from
c72827c to
dca6a5a
Compare
Currently, when application has both virtual network interfaces configured and some network devices directly assigned, EVE will first attach virtual interfaces in the order that follows ACL IDs (a historical workaround for missing interface order field), followed by directly assigned network adapters, in the order of the AppInstanceConfig.adapters list. To allow the user to specify the order between all application network interfaces (across both virtual and passthrough devices), we introduced a new boolean flag EnforceNetworkInterfaceOrder inside the application instance config and allow the controller to pass the order requirements for all the application network adapters. For the KVM hypervisor, and depending on the application’s operating system, we can influence the interface order through the PCI topology. A network device that should appear earlier in the interface list is assigned a lower PCI address. This ensures the desired network interface order, at least for Linux-based operating systems with systemd (see [1]). However, there are limitations when assigning multifunction PCI devices to applications. Specifically, the functions of a multifunction device cannot be interleaved with other virtual or passthrough devices. If the user configures a network interface order that violates this constraint, EVE will report an error for the application, and it will not be started. [1] https://www.freedesktop.org/software/systemd/man/latest/systemd.net-naming-scheme.html Signed-off-by: Milan Lenco <[email protected]>
Describe both the legacy and the user-enforced interface ordering with all its limitations. Signed-off-by: Milan Lenco <[email protected]>
milan-zededa
force-pushed
the
app-interface-order
branch
from
dca6a5a to
3de69bd
Compare
|
Marking this as ready for review so that it is not blocked from merging. However, I didn't get a chance to test it end-to-end since the controller is not ready and I also wasn't yet provided access to the target edge device. |
|
@eriknordmark , do we need to port it to 13.4 to be a part of the upcoming release? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, when application has both virtual network interfaces
configured and some network devices directly assigned, EVE will first
attach virtual interfaces in the order that follows ACL IDs (a historical
workaround for missing interface order field), followed by directly assigned
network adapters, in the order of the AppInstanceConfig.adapters list.
To allow the user to specify the order between all application
network interfaces (across both virtual and passthrough devices), we
introduced a new boolean flag
EnforceNetworkInterfaceOrderinsidethe application instance config and allow the controller to pass the order
requirements for all the application network adapters.
For the KVM hypervisor, and depending on the application’s operating system,
we can influence the interface order through the PCI topology. A network
device that should appear earlier in the interface list is assigned a lower
PCI address. This ensures the desired network interface order, at least for
Linux-based operating systems with systemd. However, there are
limitations when assigning multifunction PCI devices to applications.
Specifically, the functions of a multifunction device cannot be interleaved
with other virtual or passthrough devices. If the user configures a network
interface order that violates this constraint, EVE will report an error for
the application, and it will not be started.
In a separate commit, I have also cleaned up Go templates used for generating
Qemu config a bit. Specifically, I moved parsing of the templates into the
initfunction (we only need to do it once) and declared contexts for templates as proper
(not inline) types for better readability.