-
Notifications
You must be signed in to change notification settings - Fork 74
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
Show virtual interface's TAP device #1077
Conversation
Updated pixels |
3503023
to
b31df61
Compare
Line coverage is related to the code I just moved around, not anything I added in this PR. Tests have only known flakes |
Ready for code review |
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 you, LGTM! (Re-running flaked test)
Can you please update the screenshot to be less wide, and update the description for a release note?
@@ -105,6 +105,55 @@ VmNetworkActions.propTypes = { | |||
networks: PropTypes.array.isRequired, | |||
}; | |||
|
|||
const NetworkSource = ({ network, networkId, vm, hostDevices }) => { | |||
const id = vmId(vm.name); | |||
const checkDeviceAviability = (network) => { |
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.
"Aviability" means checking if the device can fly, right?
The typo already exists in the moved code, it's just a funny typo.
Made the IP address column consistent with the source column. HorizontalHorizontal FuildVertical |
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.
Let's go with "horizontal fluid" for now. We don't use top-labelling much elsewhere in Cockpit.
What happens when you're in mobile? Does it "do the right thing"? If it doesn't, and vertical works, then we'll just go with that instead.
@garrett Updated the PR to fluid: |
Dropdown doesn't work correctly, that needs fix |
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.
Thanks! Looks good.
bc6440a
to
afec49a
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.
Otherwise LGTM, just needs deconflicting pixels. Thanks!
Network interfaces that are attached to VMs have a TUN/TAP device. If you want to check manually, e.g. why the device is not working correctly, you need to know the interface name. Name of the tap device is be specified with attribute of the <target> element. If no target dev is specified upon vnic attachement, libvirt will create a new standard tap device with a name of the pattern "vnetN" Shot this device in the list of VM's interfaces. In the future, we could also allow user to choose/name a tap device during vNIC attachement. Fixes cockpit-project#1047
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.
Dakujem!
return () => { | ||
if (source !== null && checkDeviceAvailability(source)) { | ||
cockpit.jump(`/network#/${source}`, cockpit.transport.host); |
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.
These 3 added lines are not executed by any test. Details
const addressPortSourceElem = source => (<> | ||
<DescriptionListGroup> | ||
<DescriptionListTerm> | ||
{_("Address")} | ||
</DescriptionListTerm> | ||
<DescriptionListDescription id={`${id}-network-${networkId}-address`}> | ||
{source.address} | ||
</DescriptionListDescription> | ||
</DescriptionListGroup> | ||
<DescriptionListGroup> | ||
<DescriptionListTerm> | ||
{_("Port")} | ||
</DescriptionListTerm> | ||
<DescriptionListDescription id={`${id}-network-${networkId}-port`}> | ||
{source.port} | ||
</DescriptionListDescription> | ||
</DescriptionListGroup> |
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.
These 17 added lines are not executed by any test. Details
{ips.inet6 && <DescriptionListGroup> | ||
<DescriptionListTerm> | ||
{_("inet6")} | ||
</DescriptionListTerm> | ||
<DescriptionListDescription id={`${id}-network-${networkId}-ipv6-address`}> | ||
{ips.inet6} | ||
</DescriptionListDescription> | ||
</DescriptionListGroup>} |
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.
These 8 added lines are not executed by any test. Details
All green, apart from one unrelated flake |
Network interfaces that are attached to VMs have a TAP device. Tap devices exist to facilitate virtual private networks, network tunnels...
Every virtual network interface is assigned to a TAP device, since tap device is a representation of a virtual network device on lower layers:
If you want to troubleshoot the virtual network device manually, capture its packets, etc. on those lower layers, you need to know the interface name, so we should show it in the UI:
BEFORE
AFTER
Fixes #1047
Show virtual interface's TAP device
Network interfaces attached to VMs have a TAP device assigned, which represents the device at lower layers. Troubleshooting the virtual network device may require knowledge of the associated TAP device.