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

AlonaKaplan · 2022-12-11T08:12:06Z

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

oshoval · 2022-12-11T08:20:09Z

pkg/controllers/virtualmachineinstance_controller.go

@@ -54,7 +54,9 @@ func (r *VirtualMachineInstanceReconciler) Reconcile(ctx context.Context, reques
 		// Error reading the object - requeue the request.
 		return ctrl.Result{}, err
 	}
-	err = r.ZoneManager.UpdateZone(request.NamespacedName, FilterMultusNonDefaultInterfaces(vmi.Status.Interfaces, vmi.Spec.Networks))
+	filteredInterfaces := FilterMultusNonDefaultInterfaces(vmi.Status.Interfaces, vmi.Spec.Networks)
+	filteredInterfaces = FilterInterfacesWithNoName(vmi.Status.Interfaces) // The interface/network name is used to build the FQDN, therefore, interfaces reported without a name are filtered out


Aint you overriding the list that was created in the previous line ?

yes! Good catch:) The linter cached it as well.

oshoval

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:. ...

oshoval · 2022-12-12T07:37:26Z

pkg/controllers/interface_filter.go

@@ -30,3 +30,13 @@ func getDefaultNetwork(networks []v1.Network) *v1.Network {
 func isDefaultNetwork(net v1.Network) bool {
 	return net.Pod != nil || (net.Multus != nil && net.Multus.Default)
 }
+
+func FilterInterfacesWithNoName(ifaces []v1.VirtualMachineInstanceNetworkInterface) []v1.VirtualMachineInstanceNetworkInterface {


I think filter functions need to be named according what they do keep
FilterNamedInterfaces

And then the logic might be something like

if iface.Name == "" { continue }

if desirable - "return early"

Changed the name.

oshoval · 2022-12-12T07:38:18Z

pkg/controllers/virtualmachineinstance_controller.go

@@ -54,7 +54,9 @@ func (r *VirtualMachineInstanceReconciler) Reconcile(ctx context.Context, reques
 		// Error reading the object - requeue the request.
 		return ctrl.Result{}, err
 	}
-	err = r.ZoneManager.UpdateZone(request.NamespacedName, FilterMultusNonDefaultInterfaces(vmi.Status.Interfaces, vmi.Spec.Networks))
+	filteredInterfaces := FilterMultusNonDefaultInterfaces(vmi.Status.Interfaces, vmi.Spec.Networks)
+	filteredInterfaces = FilterInterfacesWithNoName(filteredInterfaces) // The interface/network name is used to build the FQDN, therefore, interfaces reported without a name are filtered out


Lets remove please the comment, it is pretty trivial here
also comment should be line above the code usually imo

I find the comment useful and not trivial.
Moved it to be above the code.

Imo it is very trivial and the comment is redundant
code should be (and it is) self explanatory

Moreover since name is part of the FQDN of course its needed

oshoval · 2022-12-12T07:43:22Z

pkg/controllers/interface_filter.go

@@ -30,3 +30,13 @@ func getDefaultNetwork(networks []v1.Network) *v1.Network {
 func isDefaultNetwork(net v1.Network) bool {
 	return net.Pod != nil || (net.Multus != nil && net.Multus.Default)
 }
+
+func FilterInterfacesWithNoName(ifaces []v1.VirtualMachineInstanceNetworkInterface) []v1.VirtualMachineInstanceNetworkInterface {


btw since it is used in the same package, maybe it can be lower cased first character ?
If so same for function on line 7

I find it much more readable and safe to have public API to the interface_filter file.

but it is used internally only, we don't expose stuff that is not need to be exposed

The public method are the methods that the interace_file exposes. I want it to be clear.
It is even considered a good practice to have the unit tests in a separate package, so it will be clear what is the API of the tested file.

But we don't have it on a separate package atm.
also API is for stuff that is used outside of the module, not inside.
else i would suggest for example that once we move unit tests to their own package
then we can expose it

moved the filter tests to a separate package.

AlonaKaplan · 2022-12-12T08:22:40Z

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 · 2022-12-12T08:24:24Z

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

The interface/network name is used to build the FQDN. Therefore, interfaces reported without a name are filtered out. Signed-off-by: Alona Paz <alkaplan@redhat.com>

To allow writing the tests as though they were a real user of the package. Signed-off-by: Alona Paz <alkaplan@redhat.com>

oshoval

Thanks

AlonaKaplan · 2022-12-12T08:58:03Z

/approve

kubevirt-bot · 2022-12-12T08:58:07Z

[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~~ [AlonaKaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 11, 2022

kubevirt-bot requested review from dteplits and oshoval December 11, 2022 08:12

kubevirt-bot added the size/M label Dec 11, 2022

oshoval reviewed Dec 11, 2022

View reviewed changes

AlonaKaplan force-pushed the filter_no_name branch 2 times, most recently from 83bd2be to 9fe933d Compare December 11, 2022 12:50

oshoval reviewed Dec 12, 2022

View reviewed changes

AlonaKaplan changed the title ~~Filter out interface with no name~~ status_monitor: Filter out interface with no name Dec 12, 2022

AlonaKaplan changed the title ~~status_monitor: Filter out interface with no name~~ status-monitor: Filter out interface with no name Dec 12, 2022

Filter out interface with no name

658c274

The interface/network name is used to build the FQDN. Therefore, interfaces reported without a name are filtered out. Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the filter_no_name branch from 9fe933d to 658c274 Compare December 12, 2022 08:32

moving the filter tests to a separate package

5338cd2

To allow writing the tests as though they were a real user of the package. Signed-off-by: Alona Paz <alkaplan@redhat.com>

oshoval approved these changes Dec 12, 2022

View reviewed changes

kubevirt-bot assigned oshoval Dec 12, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2022

kubevirt-bot merged commit dfbddff into kubevirt:main Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AlonaKaplan commented Dec 11, 2022 •

edited

Loading

oshoval Dec 11, 2022 •

edited

Loading

AlonaKaplan Dec 11, 2022

oshoval left a comment •

edited

Loading

oshoval Dec 12, 2022

AlonaKaplan Dec 12, 2022

oshoval Dec 12, 2022

AlonaKaplan Dec 12, 2022

oshoval Dec 12, 2022 •

edited

Loading

oshoval Dec 12, 2022 •

edited

Loading

AlonaKaplan Dec 12, 2022

oshoval Dec 12, 2022

AlonaKaplan Dec 12, 2022

oshoval Dec 12, 2022

AlonaKaplan Dec 12, 2022

AlonaKaplan commented Dec 12, 2022

oshoval commented Dec 12, 2022

oshoval left a comment

AlonaKaplan commented Dec 12, 2022

kubevirt-bot commented Dec 12, 2022

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

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

Conversation

AlonaKaplan commented Dec 11, 2022 • edited Loading

oshoval Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

oshoval Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlonaKaplan commented Dec 12, 2022

oshoval commented Dec 12, 2022

oshoval left a comment

Choose a reason for hiding this comment

AlonaKaplan commented Dec 12, 2022

kubevirt-bot commented Dec 12, 2022

AlonaKaplan commented Dec 11, 2022 •

edited

Loading

oshoval Dec 11, 2022 •

edited

Loading

oshoval left a comment •

edited

Loading

oshoval Dec 12, 2022 •

edited

Loading

oshoval Dec 12, 2022 •

edited

Loading