From f333e791e1a75e259c7f03f02aeb73de33060ac2 Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Fri, 8 Mar 2024 09:04:05 +0000
Subject: [PATCH 1/9] refactor(service): break out ServiceStatus and
 ServiceDNSRecord

---
 selfprivacy_api/models/services.py  | 24 ++++++++++++++++++++
 selfprivacy_api/services/service.py | 34 +++++------------------------
 selfprivacy_api/utils/systemd.py    |  6 ++---
 3 files changed, 33 insertions(+), 31 deletions(-)
 create mode 100644 selfprivacy_api/models/services.py

diff --git a/selfprivacy_api/models/services.py b/selfprivacy_api/models/services.py
new file mode 100644
index 0000000..638ecf8
--- /dev/null
+++ b/selfprivacy_api/models/services.py
@@ -0,0 +1,24 @@
+from enum import Enum
+from typing import Optional
+from pydantic import BaseModel
+
+
+class ServiceStatus(Enum):
+    """Enum for service status"""
+
+    ACTIVE = "ACTIVE"
+    RELOADING = "RELOADING"
+    INACTIVE = "INACTIVE"
+    FAILED = "FAILED"
+    ACTIVATING = "ACTIVATING"
+    DEACTIVATING = "DEACTIVATING"
+    OFF = "OFF"
+
+
+class ServiceDnsRecord(BaseModel):
+    type: str
+    name: str
+    content: str
+    ttl: int
+    display_name: str
+    priority: Optional[int] = None
diff --git a/selfprivacy_api/services/service.py b/selfprivacy_api/services/service.py
index 9add2dc..c44f51e 100644
--- a/selfprivacy_api/services/service.py
+++ b/selfprivacy_api/services/service.py
@@ -1,13 +1,15 @@
 """Abstract class for a service running on a server"""
 from abc import ABC, abstractmethod
-from enum import Enum
 from typing import List, Optional
 
-from pydantic import BaseModel
-from selfprivacy_api.jobs import Job, Jobs, JobStatus, report_progress
-
+from selfprivacy_api import utils
+from selfprivacy_api.utils import ReadUserData, WriteUserData
+from selfprivacy_api.utils.waitloop import wait_until_true
 from selfprivacy_api.utils.block_devices import BlockDevice, BlockDevices
 
+from selfprivacy_api.jobs import Job, Jobs, JobStatus, report_progress
+
+from selfprivacy_api.models.services import ServiceStatus, ServiceDnsRecord
 from selfprivacy_api.services.generic_size_counter import get_storage_usage
 from selfprivacy_api.services.owned_path import OwnedPath, Bind
 from selfprivacy_api.services.moving import (
@@ -20,34 +22,10 @@ from selfprivacy_api.services.moving import (
     move_data_to_volume,
 )
 
-from selfprivacy_api import utils
-from selfprivacy_api.utils.waitloop import wait_until_true
-from selfprivacy_api.utils import ReadUserData, WriteUserData
 
 DEFAULT_START_STOP_TIMEOUT = 5 * 60
 
 
-class ServiceStatus(Enum):
-    """Enum for service status"""
-
-    ACTIVE = "ACTIVE"
-    RELOADING = "RELOADING"
-    INACTIVE = "INACTIVE"
-    FAILED = "FAILED"
-    ACTIVATING = "ACTIVATING"
-    DEACTIVATING = "DEACTIVATING"
-    OFF = "OFF"
-
-
-class ServiceDnsRecord(BaseModel):
-    type: str
-    name: str
-    content: str
-    ttl: int
-    display_name: str
-    priority: Optional[int] = None
-
-
 class Service(ABC):
     """
     Service here is some software that is hosted on the server and
diff --git a/selfprivacy_api/utils/systemd.py b/selfprivacy_api/utils/systemd.py
index f8b6244..3b3ec6c 100644
--- a/selfprivacy_api/utils/systemd.py
+++ b/selfprivacy_api/utils/systemd.py
@@ -2,16 +2,16 @@
 import subprocess
 from typing import List
 
-from selfprivacy_api.services.service import ServiceStatus
+from selfprivacy_api.models.services import ServiceStatus
 
 
-def get_service_status(service: str) -> ServiceStatus:
+def get_service_status(unit: str) -> ServiceStatus:
     """
     Return service status from systemd.
     Use systemctl show to get the status of a service.
     Get ActiveState from the output.
     """
-    service_status = subprocess.check_output(["systemctl", "show", service])
+    service_status = subprocess.check_output(["systemctl", "show", unit])
     if b"LoadState=not-found" in service_status:
         return ServiceStatus.OFF
     if b"ActiveState=active" in service_status:

From 534d965cab6fa11ec311622070d01d2d45d9ebfe Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Fri, 8 Mar 2024 09:04:54 +0000
Subject: [PATCH 2/9] refactor(service): break out sync rebuilding

---
 selfprivacy_api/jobs/upgrade_system.py | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/selfprivacy_api/jobs/upgrade_system.py b/selfprivacy_api/jobs/upgrade_system.py
index 940efdb..ab16120 100644
--- a/selfprivacy_api/jobs/upgrade_system.py
+++ b/selfprivacy_api/jobs/upgrade_system.py
@@ -63,9 +63,13 @@ def check_running_status(job: Job, unit_name: str):
         return False
 
 
-@huey.task()
-def rebuild_system_task(job: Job, upgrade: bool = False):
-    """Rebuild the system"""
+def rebuild_system(job: Job, upgrade: bool = False):
+    """
+    Broken out to allow calling it synchronously.
+    We cannot just block until task is done because it will require a second worker
+    Which we do not have
+    """
+
     unit_name = "sp-nixos-upgrade.service" if upgrade else "sp-nixos-rebuild.service"
     try:
         command = ["systemctl", "start", unit_name]
@@ -124,3 +128,9 @@ def rebuild_system_task(job: Job, upgrade: bool = False):
             status=JobStatus.ERROR,
             status_text=str(e),
         )
+
+
+@huey.task()
+def rebuild_system_task(job: Job, upgrade: bool = False):
+    """Rebuild the system"""
+    rebuild_system(job, upgrade)

From 70a028779450ec9d80768c295d5009a459387a43 Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Fri, 8 Mar 2024 09:28:58 +0000
Subject: [PATCH 3/9] refactor(service): move finishing the job out of moving
 function

---
 selfprivacy_api/services/service.py               | 15 +++++++--------
 selfprivacy_api/services/test_service/__init__.py |  1 -
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/selfprivacy_api/services/service.py b/selfprivacy_api/services/service.py
index c44f51e..a5d7d64 100644
--- a/selfprivacy_api/services/service.py
+++ b/selfprivacy_api/services/service.py
@@ -365,14 +365,6 @@ class Service(ABC):
         report_progress(95, job, f"Finishing moving {service_name}...")
         self.set_location(new_volume)
 
-        Jobs.update(
-            job=job,
-            status=JobStatus.FINISHED,
-            result=f"{service_name} moved successfully.",
-            status_text=f"Starting {service_name}...",
-            progress=100,
-        )
-
     def move_to_volume(self, volume: BlockDevice, job: Job) -> Job:
         service_name = self.get_display_name()
 
@@ -384,6 +376,13 @@ class Service(ABC):
         with StoppedService(self):
             report_progress(9, job, "Stopped service, starting the move...")
             self.do_move_to_volume(volume, job)
+            Jobs.update(
+                job=job,
+                status=JobStatus.FINISHED,
+                result=f"{service_name} moved successfully.",
+                status_text=f"Starting {service_name}...",
+                progress=100,
+            )
 
         return job
 
diff --git a/selfprivacy_api/services/test_service/__init__.py b/selfprivacy_api/services/test_service/__init__.py
index f869bb3..48f84c6 100644
--- a/selfprivacy_api/services/test_service/__init__.py
+++ b/selfprivacy_api/services/test_service/__init__.py
@@ -192,6 +192,5 @@ class DummyService(Service):
         if self.simulate_moving is False:
             return super(DummyService, self).do_move_to_volume(volume, job)
         else:
-            Jobs.update(job, status=JobStatus.FINISHED)
             self.set_drive(volume.name)
             return job

From b257d7f39eb7e8b200736dbadfcd75e156796a9a Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Fri, 8 Mar 2024 10:25:04 +0000
Subject: [PATCH 4/9] fix(service): FAILING TESTS, rebuild when moving

---
 selfprivacy_api/services/service.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/selfprivacy_api/services/service.py b/selfprivacy_api/services/service.py
index a5d7d64..6ef0837 100644
--- a/selfprivacy_api/services/service.py
+++ b/selfprivacy_api/services/service.py
@@ -8,6 +8,7 @@ from selfprivacy_api.utils.waitloop import wait_until_true
 from selfprivacy_api.utils.block_devices import BlockDevice, BlockDevices
 
 from selfprivacy_api.jobs import Job, Jobs, JobStatus, report_progress
+from selfprivacy_api.jobs.upgrade_system import rebuild_system
 
 from selfprivacy_api.models.services import ServiceStatus, ServiceDnsRecord
 from selfprivacy_api.services.generic_size_counter import get_storage_usage
@@ -376,6 +377,8 @@ class Service(ABC):
         with StoppedService(self):
             report_progress(9, job, "Stopped service, starting the move...")
             self.do_move_to_volume(volume, job)
+            report_progress(98, job, "Move complete, rebuilding...")
+            rebuild_system(job, upgrade=False)
             Jobs.update(
                 job=job,
                 status=JobStatus.FINISHED,

From fed5735b248871a430b13106c7bf51220824d91d Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Wed, 13 Mar 2024 12:46:33 +0000
Subject: [PATCH 5/9] refactor(service): break out DNS records into a separate
 resolver field

---
 .../graphql/common_types/service.py           | 61 +++++++++++--------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/selfprivacy_api/graphql/common_types/service.py b/selfprivacy_api/graphql/common_types/service.py
index 314e6b6..9ec1753 100644
--- a/selfprivacy_api/graphql/common_types/service.py
+++ b/selfprivacy_api/graphql/common_types/service.py
@@ -1,14 +1,17 @@
 from enum import Enum
-import typing
-import strawberry
+from typing import Optional, List
 import datetime
+import strawberry
+
 from selfprivacy_api.graphql.common_types.backup import BackupReason
 from selfprivacy_api.graphql.common_types.dns import DnsRecord
 
 from selfprivacy_api.services import get_service_by_id, get_services_by_location
 from selfprivacy_api.services import Service as ServiceInterface
+from selfprivacy_api.services import ServiceDnsRecord
+
 from selfprivacy_api.utils.block_devices import BlockDevices
-import selfprivacy_api.utils.network as network_utils
+from selfprivacy_api.utils.network import get_ip4, get_ip6
 
 
 def get_usages(root: "StorageVolume") -> list["StorageUsageInterface"]:
@@ -33,8 +36,8 @@ class StorageVolume:
     used_space: str
     root: bool
     name: str
-    model: typing.Optional[str]
-    serial: typing.Optional[str]
+    model: Optional[str]
+    serial: Optional[str]
     type: str
 
     @strawberry.field
@@ -46,7 +49,7 @@ class StorageVolume:
 @strawberry.interface
 class StorageUsageInterface:
     used_space: str
-    volume: typing.Optional[StorageVolume]
+    volume: Optional[StorageVolume]
     title: str
 
 
@@ -54,7 +57,7 @@ class StorageUsageInterface:
 class ServiceStorageUsage(StorageUsageInterface):
     """Storage usage for a service"""
 
-    service: typing.Optional["Service"]
+    service: Optional["Service"]
 
 
 @strawberry.enum
@@ -86,6 +89,19 @@ def get_storage_usage(root: "Service") -> ServiceStorageUsage:
     )
 
 
+def service_dns_to_graphql(record: ServiceDnsRecord):
+    # Do we really need 2 types for this?
+    # ServiceDNSRecord and DnsRecord are almost identical
+    return DnsRecord(
+        record_type=record.type,
+        name=record.name,
+        content=record.content,
+        ttl=record.ttl,
+        priority=record.priority,
+        display_name=record.display_name,
+    )
+
+
 @strawberry.type
 class Service:
     id: str
@@ -98,16 +114,26 @@ class Service:
     can_be_backed_up: bool
     backup_description: str
     status: ServiceStatusEnum
-    url: typing.Optional[str]
-    dns_records: typing.Optional[typing.List[DnsRecord]]
+    url: Optional[str]
+
+    @strawberry.field
+    def dns_records(self) -> Optional[List[DnsRecord]]:
+        service = get_service_by_id(self.id)
+        if service is None:
+            raise LookupError(f"no service {self.id}. Should be unreachable")
+
+        raw_records = service.get_dns_records(get_ip4(), get_ip6())
+        dns_records = [service_dns_to_graphql(record) for record in raw_records]
+        return dns_records
 
     @strawberry.field
     def storage_usage(self) -> ServiceStorageUsage:
         """Get storage usage for a service"""
         return get_storage_usage(self)
 
+    # TODO: fill this
     @strawberry.field
-    def backup_snapshots(self) -> typing.Optional[typing.List["SnapshotInfo"]]:
+    def backup_snapshots(self) -> Optional[List["SnapshotInfo"]]:
         return None
 
 
@@ -133,23 +159,10 @@ def service_to_graphql_service(service: ServiceInterface) -> Service:
         backup_description=service.get_backup_description(),
         status=ServiceStatusEnum(service.get_status().value),
         url=service.get_url(),
-        dns_records=[
-            DnsRecord(
-                record_type=record.type,
-                name=record.name,
-                content=record.content,
-                ttl=record.ttl,
-                priority=record.priority,
-                display_name=record.display_name,
-            )
-            for record in service.get_dns_records(
-                network_utils.get_ip4(), network_utils.get_ip6()
-            )
-        ],
     )
 
 
-def get_volume_by_id(volume_id: str) -> typing.Optional[StorageVolume]:
+def get_volume_by_id(volume_id: str) -> Optional[StorageVolume]:
     """Get volume by id"""
     volume = BlockDevices().get_block_device(volume_id)
     if volume is None:

From 8c8c9a51ccc0c9fee9fa167f203ebf2d9c5d4625 Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Wed, 13 Mar 2024 12:48:11 +0000
Subject: [PATCH 6/9] refactor(service): visually break down the move function
 a bit

---
 selfprivacy_api/services/service.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/selfprivacy_api/services/service.py b/selfprivacy_api/services/service.py
index 6ef0837..64a1e80 100644
--- a/selfprivacy_api/services/service.py
+++ b/selfprivacy_api/services/service.py
@@ -377,8 +377,10 @@ class Service(ABC):
         with StoppedService(self):
             report_progress(9, job, "Stopped service, starting the move...")
             self.do_move_to_volume(volume, job)
+
             report_progress(98, job, "Move complete, rebuilding...")
             rebuild_system(job, upgrade=False)
+
             Jobs.update(
                 job=job,
                 status=JobStatus.FINISHED,

From 12b2153b7cdba49b53337b45b7187df26532a104 Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Wed, 13 Mar 2024 12:50:41 +0000
Subject: [PATCH 7/9] test(service): do not call bash needlessly (it screwed up
 with fp)

---
 .../services/test_service/__init__.py            | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/selfprivacy_api/services/test_service/__init__.py b/selfprivacy_api/services/test_service/__init__.py
index 48f84c6..caf4666 100644
--- a/selfprivacy_api/services/test_service/__init__.py
+++ b/selfprivacy_api/services/test_service/__init__.py
@@ -8,10 +8,9 @@ from os import path
 
 # from enum import Enum
 
-from selfprivacy_api.jobs import Job, Jobs, JobStatus
-from selfprivacy_api.services.service import Service, ServiceDnsRecord, ServiceStatus
+from selfprivacy_api.jobs import Job
+from selfprivacy_api.services.service import Service, ServiceStatus
 from selfprivacy_api.utils.block_devices import BlockDevice
-import selfprivacy_api.utils.network as network_utils
 
 from selfprivacy_api.services.test_service.icon import BITWARDEN_ICON
 
@@ -89,7 +88,7 @@ class DummyService(Service):
     @classmethod
     def set_status(cls, status: ServiceStatus):
         with open(cls.status_file(), "w") as file:
-            status_string = file.write(status.value)
+            file.write(status.value)
 
     @classmethod
     def get_status(cls) -> ServiceStatus:
@@ -102,16 +101,17 @@ class DummyService(Service):
         cls, new_status: ServiceStatus, delay_sec: float
     ):
         """simulating a delay on systemd side"""
-        status_file = cls.status_file()
+        if delay_sec == 0:
+            cls.set_status(new_status)
+            return
 
+        status_file = cls.status_file()
         command = [
             "bash",
             "-c",
             f" sleep {delay_sec} && echo {new_status.value} > {status_file}",
         ]
-        handle = subprocess.Popen(command)
-        if delay_sec == 0:
-            handle.communicate()
+        subprocess.Popen(command)
 
     @classmethod
     def set_backuppable(cls, new_value: bool) -> None:

From 6e29da4a4fc2032113150d4acf39dd5d619a80f1 Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Wed, 13 Mar 2024 12:54:01 +0000
Subject: [PATCH 8/9] test(service): test moving with rebuilding via fp

---
 tests/test_graphql/test_services.py           | 28 +++++++++++++++----
 tests/test_graphql/test_system_nixos_tasks.py | 24 +++++++++-------
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/tests/test_graphql/test_services.py b/tests/test_graphql/test_services.py
index 06ef3a1..6e8dcf6 100644
--- a/tests/test_graphql/test_services.py
+++ b/tests/test_graphql/test_services.py
@@ -13,8 +13,7 @@ from selfprivacy_api.services.test_service import DummyService
 
 from tests.common import generate_service_query
 from tests.test_graphql.common import assert_empty, assert_ok, get_data
-from tests.test_block_device_utils import lsblk_singular_mock
-
+from tests.test_graphql.test_system_nixos_tasks import prepare_nixos_rebuild_calls
 
 LSBLK_BLOCKDEVICES_DICTS = [
     {
@@ -618,10 +617,7 @@ def test_graphql_move_service_without_folders_on_old_volume(
 
 
 def test_graphql_move_service(
-    authorized_client,
-    generic_userdata,
-    mock_check_volume,
-    dummy_service_with_binds,
+    authorized_client, generic_userdata, mock_check_volume, dummy_service_with_binds, fp
 ):
     dummy_service = dummy_service_with_binds
 
@@ -633,10 +629,30 @@ def test_graphql_move_service(
     dummy_service.set_drive(origin)
     dummy_service.set_simulated_moves(False)
 
+    unit_name = "sp-nixos-rebuild.service"
+    rebuild_command = ["systemctl", "start", unit_name]
+    prepare_nixos_rebuild_calls(fp, unit_name)
+
+    # We will be mounting and remounting folders
+    mount_command = ["mount", fp.any()]
+    unmount_command = ["umount", fp.any()]
+    fp.pass_command(mount_command, 2)
+    fp.pass_command(unmount_command, 2)
+
+    # We will be changing ownership
+    chown_command = ["chown", fp.any()]
+    fp.pass_command(chown_command, 2)
+
     mutation_response = api_move(authorized_client, dummy_service, target)
 
     data = get_data(mutation_response)["services"]["moveService"]
     assert_ok(data)
+    assert data["service"] is not None
+
+    assert fp.call_count(rebuild_command) == 1
+    assert fp.call_count(mount_command) == 2
+    assert fp.call_count(unmount_command) == 2
+    assert fp.call_count(chown_command) == 2
 
 
 def test_mailservice_cannot_enable_disable(authorized_client):
diff --git a/tests/test_graphql/test_system_nixos_tasks.py b/tests/test_graphql/test_system_nixos_tasks.py
index b50223e..2b60fe5 100644
--- a/tests/test_graphql/test_system_nixos_tasks.py
+++ b/tests/test_graphql/test_system_nixos_tasks.py
@@ -97,16 +97,7 @@ def test_graphql_system_rebuild_unauthorized(client, fp, action):
     assert fp.call_count([fp.any()]) == 0
 
 
-@pytest.mark.parametrize("action", ["rebuild", "upgrade"])
-def test_graphql_system_rebuild(authorized_client, fp, action, mock_sleep_intervals):
-    """Test system rebuild"""
-    unit_name = f"sp-nixos-{action}.service"
-    query = (
-        API_REBUILD_SYSTEM_MUTATION
-        if action == "rebuild"
-        else API_UPGRADE_SYSTEM_MUTATION
-    )
-
+def prepare_nixos_rebuild_calls(fp, unit_name):
     # Start the unit
     fp.register(["systemctl", "start", unit_name])
 
@@ -129,6 +120,19 @@ def test_graphql_system_rebuild(authorized_client, fp, action, mock_sleep_interv
 
     fp.register(["systemctl", "show", unit_name], stdout="ActiveState=inactive")
 
+
+@pytest.mark.parametrize("action", ["rebuild", "upgrade"])
+def test_graphql_system_rebuild(authorized_client, fp, action, mock_sleep_intervals):
+    """Test system rebuild"""
+    unit_name = f"sp-nixos-{action}.service"
+    query = (
+        API_REBUILD_SYSTEM_MUTATION
+        if action == "rebuild"
+        else API_UPGRADE_SYSTEM_MUTATION
+    )
+
+    prepare_nixos_rebuild_calls(fp, unit_name)
+
     response = authorized_client.post(
         "/graphql",
         json={

From b2edfe784a30e1b125794a450a9cab6fa823b7b4 Mon Sep 17 00:00:00 2001
From: Houkime <>
Date: Mon, 18 Mar 2024 11:44:53 +0000
Subject: [PATCH 9/9] refactor(service): add return typing to DNSrecord
 conversion and comments

---
 selfprivacy_api/graphql/common_types/dns.py     | 1 +
 selfprivacy_api/graphql/common_types/service.py | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/selfprivacy_api/graphql/common_types/dns.py b/selfprivacy_api/graphql/common_types/dns.py
index 1c79036..f47daa8 100644
--- a/selfprivacy_api/graphql/common_types/dns.py
+++ b/selfprivacy_api/graphql/common_types/dns.py
@@ -2,6 +2,7 @@ import typing
 import strawberry
 
 
+# TODO: use https://strawberry.rocks/docs/integrations/pydantic when it is stable
 @strawberry.type
 class DnsRecord:
     """DNS record"""
diff --git a/selfprivacy_api/graphql/common_types/service.py b/selfprivacy_api/graphql/common_types/service.py
index 9ec1753..275c14c 100644
--- a/selfprivacy_api/graphql/common_types/service.py
+++ b/selfprivacy_api/graphql/common_types/service.py
@@ -89,9 +89,10 @@ def get_storage_usage(root: "Service") -> ServiceStorageUsage:
     )
 
 
-def service_dns_to_graphql(record: ServiceDnsRecord):
-    # Do we really need 2 types for this?
-    # ServiceDNSRecord and DnsRecord are almost identical
+# TODO: This won't be needed when deriving DnsRecord via strawberry pydantic integration
+# https://strawberry.rocks/docs/integrations/pydantic
+# Remove when the link above says it got stable.
+def service_dns_to_graphql(record: ServiceDnsRecord) -> DnsRecord:
     return DnsRecord(
         record_type=record.type,
         name=record.name,