feat: declare all three roles, and refuse QPKG writes deliberately
A QNAP is a NAS, a hypervisor and a Linux host. It can now say so, because role
bases in napalm-device-types v1.0 declare their methods without implementing
them:
class QnapQtsDriver(StorageDriver, HypervisorDriver, LinuxDriver):
device_class comes from the first base, so DEVICE_CLASS is gone. The get_services
forwarder is gone with it — nothing shadows LinuxDriver's version any more — and
so is the TestMroForwarding class that guarded the collisions.
Removing the shadowing exposed something the shadowing had been hiding by
accident. This driver never implemented package management; StorageDriver's stub
for install_package covered LinuxDriver's working one, so calling it raised and
looked correct. It was a coincidence, not a decision: without the stub the call
falls through to LinuxDriver, which would run apt or dnf against a NAS that has
neither. QTS uses QPKG.
get_packages, install_package and uninstall_package are therefore overridden
here to refuse with a reason, until the device harvest supplies real QPKG
parsing. See netork#114.
This commit is contained in:
+32
-25
@@ -24,7 +24,12 @@ from __future__ import annotations
|
||||
import re
|
||||
from typing import Any
|
||||
|
||||
from napalm_device_types import FingerprintRule, PortSpec, StorageDriver
|
||||
from napalm_device_types import (
|
||||
FingerprintRule,
|
||||
HypervisorDriver,
|
||||
PortSpec,
|
||||
StorageDriver,
|
||||
)
|
||||
from napalm_linux.linux import LinuxDriver
|
||||
|
||||
#: QNAP Systems' IANA enterprise number. Used by discovery to recognise a NAS
|
||||
@@ -42,27 +47,22 @@ _DOCKER_GLOBS = (
|
||||
_VERSION_RE = re.compile(r"(\d+)\.")
|
||||
|
||||
|
||||
class QnapQtsDriver(StorageDriver, LinuxDriver):
|
||||
class QnapQtsDriver(StorageDriver, HypervisorDriver, LinuxDriver):
|
||||
"""NAPALM driver for QNAP NAS systems running QTS.
|
||||
|
||||
**Inheritance order matters.** ``StorageDriver`` precedes ``LinuxDriver`` in
|
||||
the MRO, so its ``NotImplementedError`` stubs shadow LinuxDriver's working
|
||||
implementations wherever the names collide — ``get_services``,
|
||||
``get_packages`` and ``install_package``. Each collision is resolved
|
||||
explicitly below rather than left to the MRO; see ``TestMroForwarding``.
|
||||
Declares all three roles it fills. The role bases carry no implementations,
|
||||
so listing them alongside ``LinuxDriver`` costs nothing and shadows nothing;
|
||||
what the driver can actually do is whatever it implements below.
|
||||
|
||||
NAS services are exposed as ``get_storage_services()``, not
|
||||
``get_services()``, because netOrk's poller reads the former for the storage
|
||||
snapshot and the latter for the OS service list. Same convention as
|
||||
napalm-openmediavault.
|
||||
NAS services are ``get_storage_services()``; the OS service list inherited
|
||||
from ``LinuxDriver`` stays ``get_services()``. Two names because they are two
|
||||
different things with different return shapes, not because they collided.
|
||||
"""
|
||||
|
||||
# A QNAP is three things at once, and the order says which one leads:
|
||||
# storage first, so netOrk shows it as a NAS. Nothing has to restate that
|
||||
# -- device_class is read from this line.
|
||||
TYPE_LABEL = "Storage"
|
||||
# Declared outright: the driver inherits LinuxDriver for the OS surface, so
|
||||
# netOrk's issubclass chain would reach OSDriver first and classify a QNAP
|
||||
# as "linux", hiding its Storage tab. VMs and containers stay visible
|
||||
# through capability introspection, not through this key.
|
||||
DEVICE_CLASS = "storage"
|
||||
VENDOR = "QNAP"
|
||||
DRIVER_NAME = "qnap_qts"
|
||||
driver_name = "qnap_qts"
|
||||
@@ -143,15 +143,22 @@ class QnapQtsDriver(StorageDriver, LinuxDriver):
|
||||
"""Override LinuxDriver's hook: docker is not on PATH under QTS."""
|
||||
return getattr(self, "_docker_path", None) or "docker"
|
||||
|
||||
# ── MRO collision resolution ──────────────────────────────────────────────
|
||||
# ── QPKG, not apt ─────────────────────────────────────────────────────────
|
||||
#
|
||||
# StorageDriver comes first in the MRO and its stubs would otherwise win.
|
||||
# QTS is Linux, so LinuxDriver's package methods are in the MRO and would
|
||||
# run happily -- against a package manager QTS does not have. They used to
|
||||
# be shadowed by StorageDriver's stubs, which made a QNAP refuse them by
|
||||
# accident. Now that role bases implement nothing, the refusal has to be
|
||||
# deliberate. QPKG parsing lands with the device harvest; until then,
|
||||
# refusing is the only honest answer.
|
||||
|
||||
def get_services(self) -> Any:
|
||||
"""OS services, not NAS services — this is what netOrk's poller reads.
|
||||
def get_packages(self) -> Any:
|
||||
raise NotImplementedError(
|
||||
"QTS uses QPKG, not a Linux package manager; QPKG parsing is not implemented yet"
|
||||
)
|
||||
|
||||
Forwarded explicitly past ``StorageDriver.get_services``, which shadows
|
||||
it and returns a different shape (dict of NAS services vs. list of OS
|
||||
services).
|
||||
"""
|
||||
return LinuxDriver.get_services(self)
|
||||
def install_package(self, name: str, version: str = "") -> None:
|
||||
raise NotImplementedError("QPKG install is not implemented yet")
|
||||
|
||||
def uninstall_package(self, name: str) -> None:
|
||||
raise NotImplementedError("QPKG removal is not implemented yet")
|
||||
|
||||
+45
-26
@@ -11,6 +11,7 @@ from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from napalm_device_types import primary_role_of, role_keys_of
|
||||
from napalm_qnap_qts import QnapQtsDriver
|
||||
|
||||
#: Parsers need real command output to be written against. These tests are the
|
||||
@@ -62,10 +63,14 @@ class TestDriverIdentity:
|
||||
def test_type_label_is_storage(self):
|
||||
assert QnapQtsDriver.TYPE_LABEL == "Storage"
|
||||
|
||||
def test_declares_device_class_explicitly(self):
|
||||
"""Inheriting LinuxDriver would otherwise get it classified as "linux",
|
||||
and netOrk would hide the Storage tab."""
|
||||
assert QnapQtsDriver.DEVICE_CLASS == "storage"
|
||||
def test_declares_every_role_it_fills(self):
|
||||
"""A QNAP is a NAS, a hypervisor and a Linux host at once."""
|
||||
assert role_keys_of(QnapQtsDriver) == ["storage", "hypervisor", "linux"]
|
||||
|
||||
def test_storage_leads_because_it_is_listed_first(self):
|
||||
"""device_class comes from the order of the role bases, not from an
|
||||
attribute restating it."""
|
||||
assert primary_role_of(QnapQtsDriver) == "storage"
|
||||
|
||||
def test_declares_at_least_one_fingerprint_source(self):
|
||||
"""Discovery silently skips a driver that declares no fingerprint data."""
|
||||
@@ -83,42 +88,51 @@ class TestDriverIdentity:
|
||||
assert patterns["qnap"].mandatory is True
|
||||
|
||||
|
||||
class TestMroForwarding:
|
||||
"""StorageDriver precedes LinuxDriver in the MRO, so its NotImplementedError
|
||||
stubs shadow LinuxDriver's working implementations. Every collision has to be
|
||||
resolved deliberately — this is the class of bug that makes a driver look
|
||||
fine until it runs against hardware.
|
||||
class TestRolesDoNotShadowLinux:
|
||||
"""The role bases declare their methods; they implement none.
|
||||
|
||||
Before that change, ``StorageDriver`` preceded ``LinuxDriver`` in the MRO and
|
||||
its ``NotImplementedError`` stubs replaced LinuxDriver's working
|
||||
implementations, so this driver carried a hand-written forwarder for every
|
||||
collision. There is nothing left to collide with.
|
||||
"""
|
||||
|
||||
def test_get_services_returns_the_linux_os_service_list(self, driver):
|
||||
"""netOrk's poller expects a list here (OS services). StorageDriver's
|
||||
stub would return a dict of NAS services, if it returned anything."""
|
||||
@pytest.mark.parametrize("method", ["get_services", "get_users", "get_docker_info"])
|
||||
def test_os_surface_resolves_to_linux(self, method):
|
||||
"""QTS really is Linux for these, so inheriting them is correct."""
|
||||
from napalm_linux.linux import LinuxDriver
|
||||
|
||||
with patch.object(LinuxDriver, "get_services", return_value=[{"name": "sshd"}]) as m:
|
||||
result = driver.get_services()
|
||||
owner = next(k for k in QnapQtsDriver.__mro__ if method in k.__dict__)
|
||||
assert owner is LinuxDriver
|
||||
|
||||
assert m.called
|
||||
assert result == [{"name": "sshd"}]
|
||||
@pytest.mark.parametrize(
|
||||
"method", ["get_packages", "install_package", "uninstall_package"]
|
||||
)
|
||||
def test_package_surface_is_refused_deliberately(self, method):
|
||||
"""QTS has no apt/dnf, so LinuxDriver's versions must not be inherited
|
||||
silently -- this driver overrides them to refuse."""
|
||||
owner = next(k for k in QnapQtsDriver.__mro__ if method in k.__dict__)
|
||||
assert owner is QnapQtsDriver
|
||||
|
||||
def test_no_forwarding_methods_remain(self):
|
||||
"""A forwarder here would mean the shadowing came back."""
|
||||
own = {
|
||||
name for name, val in vars(QnapQtsDriver).items()
|
||||
if callable(val) and not name.startswith("__")
|
||||
}
|
||||
assert "get_services" not in own
|
||||
|
||||
@pytest.mark.xfail(strict=True, reason=_PENDING_HARVEST)
|
||||
def test_nas_services_live_under_a_separate_name(self):
|
||||
"""get_storage_services is what netOrk's _collect.py actually reads for
|
||||
the storage snapshot — get_services is the OS list."""
|
||||
"""get_storage_services is what netOrk's _collect.py reads for the
|
||||
storage snapshot — get_services is the OS list."""
|
||||
assert hasattr(QnapQtsDriver, "get_storage_services")
|
||||
|
||||
@pytest.mark.xfail(strict=True, reason=_PENDING_HARVEST)
|
||||
def test_get_packages_is_not_the_storage_stub(self):
|
||||
from napalm_device_types import StorageDriver
|
||||
|
||||
assert QnapQtsDriver.get_packages is not StorageDriver.get_packages
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("method", "args"),
|
||||
[
|
||||
("install_package", ("qpkg-name",)),
|
||||
("remove_package", ("qpkg-name",)),
|
||||
("snapshot_create", ("DataVol1", "snap1")),
|
||||
("uninstall_package", ("qpkg-name",)),
|
||||
],
|
||||
)
|
||||
def test_out_of_scope_writers_still_raise(self, driver, method, args):
|
||||
@@ -127,6 +141,11 @@ class TestMroForwarding:
|
||||
with pytest.raises(NotImplementedError):
|
||||
getattr(driver, method)(*args)
|
||||
|
||||
def test_volume_snapshot_writer_is_not_implemented_yet(self):
|
||||
"""Declared on StorageDriver for type checkers only, so it does not
|
||||
exist until the harvest supplies a real implementation."""
|
||||
assert not hasattr(QnapQtsDriver, "create_volume_snapshot")
|
||||
|
||||
|
||||
class TestQtsVersionDetection:
|
||||
def test_parses_major_version(self, driver):
|
||||
|
||||
Reference in New Issue
Block a user