Add abstract functions for PoE enabling #2636

stveit · 2023-05-31T12:56:10Z

For #1938.

Adds empty functions to ManagementHandler that should be implemented by vendor specific handlers in different ways. Different
vendors support different PoE states, so get_poe_state_options should return all available options. These options should then be displayed in PortAdmin and be selectable.

github-actions · 2023-05-31T13:04:14Z

Test results

    12 files     12 suites 12m 34s ⏱️
3 171 tests 3 171 ✔️ 0 💤 0 ❌
8 988 runs 8 988 ✔️ 0 💤 0 ❌

Results for commit b9ace82.

♻️ This comment has been updated with latest results.

lunkwill42

This is a good start - but (and don't take my word for it), I would assume that not every port on a device is necessarily capable of power delivery, so the API might need a way to signal PoE-eligibility for individual ports.

When it comes to the power state tuple-thingy, I would suggest using a NamedTuple, which would greatly clarify what this magic tuple-thingy is.

let each implementation handle conversion if the actual state is represented as integers or whatever, but for this interface just let it be strings, e.g. DISABLED, AUTO or something

codecov · 2023-06-02T10:48:56Z

Codecov Report

Merging #2636 (b9ace82) into master (b7f24ec) will increase coverage by 0.54%.
Report is 95 commits behind head on master.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #2636      +/-   ##
==========================================
+ Coverage   54.20%   54.74%   +0.54%     
==========================================
  Files         558      560       +2     
  Lines       40634    40743     +109     
==========================================
+ Hits        22026    22306     +280     
+ Misses      18608    18437     -171

Files Changed	Coverage Δ
python/nav/portadmin/handlers.py	`45.04% <75.00%> (+3.04%)`	⬆️

... and 26 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

the state options should be the same on the netbox for ports that support it, so no need to check that per interface. what we actually care about is if the interface supports poe, so this does that more explicitly

stveit · 2023-06-02T12:00:38Z

Made it a bit less magical. Instead of actual state and name, just return the names as a tuple of strings and let each management implementation handle conversion behind the scenes. Added function for checking if specific interface supports PoE

sonarcloud · 2023-06-06T09:59:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

hmpf

Looks fine to me. When we have two subclasses we'll know if it is good enough. We will then also have examples (and tests!)

@lunkwill42 needs to approve it, though.

python/nav/portadmin/handlers.py

stveit · 2023-08-28T11:12:54Z

python/nav/portadmin/handlers.py

+
+    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
+


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

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

nav/python/nav/portadmin/handlers.py

Lines 59 to 72 in 5d97c40

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.

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

stveit · 2023-08-28T11:14:23Z

python/nav/portadmin/handlers.py

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


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

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.

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

lunkwill42

This looks a lot better without too much magic - but please do consider how bulk operations will work (preferably similar to other methods on the ManagementHandler)

lunkwill42 · 2023-08-30T09:47:09Z

python/nav/portadmin/handlers.py

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


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.

stveit · 2023-09-04T07:38:43Z

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.

I believe dict should be used for return types and Mapping is recommended for arguments

new `get_poe_states` supports single interface queries

stveit · 2023-09-04T07:50:53Z

python/nav/portadmin/handlers.py

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


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.

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.

sonarcloud · 2023-09-04T07:59:25Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

lunkwill42

This looks just fine as it is, but I also agree that the interface_supports_poe might be redundant. Either that, or it could have a base implementation that calls get_poe_state_options() for the interface and returns True/False based on whether the interface has any state options.

lunkwill42 · 2023-09-04T11:27:55Z

python/nav/portadmin/handlers.py

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


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.

sonarcloud · 2023-09-04T18:41:29Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

stveit added 3 commits May 31, 2023 14:52

Add func for getting poe enabling options

ab51092

Add func for setting poe option

4ea580c

Add func for getting current poe state

cad788d

stveit self-assigned this May 31, 2023

lunkwill42 requested changes Jun 1, 2023

View reviewed changes

stveit added 3 commits June 2, 2023 12:37

Make state be a string

229bfda

let each implementation handle conversion if the actual state is represented as integers or whatever, but for this interface just let it be strings, e.g. DISABLED, AUTO or something

Get state options per interface

f6a6554

Be consistent with capitalization

39473fb

stveit requested a review from lunkwill42 June 2, 2023 10:47

Add function that checks if interface supports poe

d3c9131

the state options should be the same on the netbox for ports that support it, so no need to check that per interface. what we actually care about is if the interface supports poe, so this does that more explicitly

Represent PoE state using dataclass

5d97c40

stveit marked this pull request as ready for review June 6, 2023 09:57

stveit mentioned this pull request Jul 12, 2023

Add support for configuring PoE for cisco equipment #2635

Merged

hmpf self-requested a review July 26, 2023 10:19

hmpf reviewed Jul 26, 2023

View reviewed changes

stveit commented Aug 28, 2023

View reviewed changes

python/nav/portadmin/handlers.py Show resolved Hide resolved

stveit commented Aug 28, 2023

View reviewed changes

stveit mentioned this pull request Aug 28, 2023

Add PoE config for Juniper devices #2666

Merged

lunkwill42 requested changes Aug 30, 2023

View reviewed changes

Use Sequence typing instead of Tuple specifically

269430d

stveit added 2 commits September 4, 2023 09:44

Add func for getting state of multiple interfaces

10e77b4

Remove func for only getting one interface

9a59f75

new `get_poe_states` supports single interface queries

stveit commented Sep 4, 2023

View reviewed changes

Shorten line length

8a027e7

fixup! Use Sequence typing instead of Tuple specifically

6f51c86

lunkwill42 approved these changes Sep 4, 2023

View reviewed changes

Build PoE support functionality into get_poe_state

b9ace82

stveit merged commit f0fe990 into Uninett:master Sep 5, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add abstract functions for PoE enabling #2636

Add abstract functions for PoE enabling #2636

stveit commented May 31, 2023 •

edited

github-actions bot commented May 31, 2023 •

edited

lunkwill42 left a comment

codecov bot commented Jun 2, 2023 •

edited

stveit commented Jun 2, 2023 •

edited

sonarcloud bot commented Jun 6, 2023

hmpf left a comment

stveit Aug 28, 2023

lunkwill42 Aug 30, 2023

stveit Aug 30, 2023

stveit Aug 28, 2023

lunkwill42 Aug 30, 2023

stveit Aug 30, 2023

lunkwill42 left a comment

lunkwill42 Aug 30, 2023

stveit commented Sep 4, 2023 •

edited

stveit Sep 4, 2023 •

edited

lunkwill42 Sep 4, 2023

sonarcloud bot commented Sep 4, 2023

lunkwill42 left a comment

lunkwill42 Sep 4, 2023

sonarcloud bot commented Sep 4, 2023

	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

Add abstract functions for PoE enabling #2636

Add abstract functions for PoE enabling #2636

Conversation

stveit commented May 31, 2023 • edited

github-actions bot commented May 31, 2023 • edited

Test results

lunkwill42 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2023 • edited

Codecov Report

stveit commented Jun 2, 2023 • edited

sonarcloud bot commented Jun 6, 2023

hmpf left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunkwill42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stveit commented Sep 4, 2023 • edited

stveit Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 4, 2023

lunkwill42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 4, 2023

stveit commented May 31, 2023 •

edited

github-actions bot commented May 31, 2023 •

edited

codecov bot commented Jun 2, 2023 •

edited

stveit commented Jun 2, 2023 •

edited

stveit commented Sep 4, 2023 •

edited

stveit Sep 4, 2023 •

edited