search

kubevirt / kubesecondarydns

DNS for KubeVirt VirtualMachines secondary interfaces
Apache License 2.0
7 stars 8 forks source link

status-monitor: Filter out interface with no name #46

Closed AlonaKaplan closed 1 year ago

AlonaKaplan commented 1 year ago

What this PR does / why we need it: The interface/network name is used to build the FQDN. Therefore, interfaces reported without a name are filtered out. The reported interface may not have a name from multiple reasons. e.g the interface was added manually in the guest, the interface MAC address was change manually, the interface is SRIOV.

Special notes for your reviewer: This PR is removing the empty name filtering from the zonemanger and moves it to a dedicated filter.

Release note:

NONE
AlonaKaplan commented 1 year ago

Thanks Please add example on PR desc about SRIOV that for example a SRIOV interface without mac wouldn't have a network name

Might even worth release notes ? not crucial as we don't collect it yet

Please consider adding that it was filtered, but you refactor it to be in the right place so it will be easier to understand why the ZM is changed

Please consider prefixing PR desc with status-monitor:. ...

Added the prefix and explanation why the code was removed form the zone manager. I don't think the sriov example it relevant here. The reported interface may not have a name from multiple reasons. e.g the interface was added manually in the guest, the interface MAC address was change manually in the guest and also the SRIOV case. I think that for kubesecondarydns it is irrelevant why there is no name, the only relevant part is that if there is no name, there is no FQDN.

oshoval commented 1 year ago

Thanks Please add example on PR desc about SRIOV that for example a SRIOV interface without mac wouldn't have a network name Might even worth release notes ? not crucial as we don't collect it yet Please consider adding that it was filtered, but you refactor it to be in the right place so it will be easier to understand why the ZM is changed Please consider prefixing PR desc with status-monitor:. ...

Added the prefix and explanation why the code was removed form the zone manager. I don't think the sriov example it relevant here. The reported interface may not have a name from multiple reasons. e.g the interface was added manually in the guest, the interface MAC address was change manually in the guest and also the SRIOV case. I think that for kubesecondarydns it is irrelevant why there is no name, the only relevant part is that if there is no name, there is no FQDN.

I understand, what about expressing all those examples in the PR description ? It might be nice to have it listed

AlonaKaplan commented 1 year ago

/approve

kubevirt-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

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: - ~~[OWNERS](https://github.com/kubevirt/kubesecondarydns/blob/main/OWNERS)~~ [AlonaKaplan] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment