Skip to content
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

Add abstract functions for PoE enabling #2636

Merged
merged 14 commits into from Sep 5, 2023
32 changes: 31 additions & 1 deletion python/nav/portadmin/handlers.py
Expand Up @@ -15,8 +15,9 @@
#
"""Interface definition for PortAdmin management handlers"""
import time
from typing import List, Tuple, Dict, Any, Sequence
from typing import List, Tuple, Dict, Any, Sequence, Union
import logging
from dataclasses import dataclass

from nav.models import manage
from nav.portadmin.vlan import FantasyVlan
Expand All @@ -25,6 +26,17 @@
_logger = logging.getLogger(__name__)


@dataclass
class PoeState:
"""Class for defining PoE states.
`state` is the value used on the device itself.
`name` is a human readable name for the state
"""

state: Union[str, int]
name: str


stveit marked this conversation as resolved.
Show resolved Hide resolved
class ManagementHandler:
"""Defines a common interface for all types of PortAdmin management handlers.

Expand Down Expand Up @@ -278,6 +290,24 @@ def is_configurable(self) -> bool:
return False
return True

def get_poe_state_options(self) -> Tuple[PoeState, ...]:
"""Returns the available options for enabling/disabling PoE on this netbox"""
raise NotImplementedError
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to be called by the frontend to populate a dropdown menu or something. the user can then select one of the PoeState options, and this would then be fed into set_poe_state

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the return value very specifically need to be a tuple? Usually I prefer annotating things as Sequence unless there is a expectation of being able to use the result value specifically as a tuple or list object.

There is the same thing with dict vs. Mapping, but I must admit I haven't been using the latter as much as I probably should have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuple was used to signify that this is not something that is meant to be manipulated in any way, since it just informs the static choices. Havent used Sequence myself but I can try it out here :)


def set_poe_state(self, interface: manage.Interface, state: PoeState):
"""Set state for enabling/disabling PoE on this interface.
Available options should be retrieved using `get_poe_state_options`
"""
raise NotImplementedError

def get_poe_state(self, interface: manage.Interface) -> PoeState:
"""Returns current PoE state of this interface"""
raise NotImplementedError

def interface_supports_poe(self, interface: manage.Interface) -> bool:
"""Returns True if this interface supports PoE"""
raise NotImplementedError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When loading portadmin, youre gonna want to call get_poe_state for all poe enabled ports. Doing it one by one is not very efficient. Perhaps adding another function for getting ALL poe states would make sense. If this function returns poe state for all interfaces that support poe, then this response can be used to find out which interfaces do not support poe, making the interface_supports_poe function largely irrelevant.

Keeping the smaller functions for getting information about a specific interface would not hurt, but having functions that do the heavy lifting more efficient would definitely be useful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, some methods already have signatures that allow for fetching interface information from a range of interfaces, like this:

def get_interfaces(
self, interfaces: Sequence[manage.Interface] = None
) -> List[Dict[str, Any]]:
"""Retrieves running configuration switch ports on the device.
:param interfaces: Optional list of interfaces to filter for, as fetching
data for all interfaces may be a waste of time if only a
single interface is needed. The implementing
handler/protocol may not support this filter, so do not rely
on it.
:returns: A list of dicts with members `name`, `description`, `oper`, `admin`
and `vlan` (the latter being the access/untagged/native vlan ID.
"""
raise NotImplementedError

I think that could be a useful pattern to re-use. In these methods, the sequence of interfaces is an optional argument. If omitted, the method will operate on all known interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems quite useful, I'll prob add something like this

Copy link
Contributor Author

@stveit stveit Sep 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I question the need for this function. When ive tried to implement this for cisco and juniper, it boils down to trying to get the state from the device and returning true/false based on if it found anything. It doesnt actually save any time over just calling get_poe_states for the interface youre interested in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. You could simply change the definition of get_poe_state to return something like Dict[int, Optional[PoeState]], where the value for a non-supported interface would just be None.


class ManagementError(Exception):
"""Base exception class for device management errors"""
Expand Down