add nix-collect-garbage endpoint #21
|
@ -8,9 +8,12 @@ from selfprivacy_api.graphql.mutations.mutation_interface import (
|
|||
GenericJobMutationReturn,
|
||||
GenericMutationReturn,
|
||||
MutationReturnInterface,
|
||||
GenericJobMutationReturn,
|
||||
)
|
||||
|
||||
import selfprivacy_api.actions.system as system_actions
|
||||
from selfprivacy_api.graphql.common_types.jobs import job_to_api_job
|
||||
from selfprivacy_api.jobs.nix_collect_garbage import start_nix_collect_garbage
|
||||
import selfprivacy_api.actions.ssh as ssh_actions
|
||||
|
||||
|
||||
|
@ -195,3 +198,14 @@ class SystemMutations:
|
|||
message=f"Failed to pull repository changes:\n{result.data}",
|
||||
code=500,
|
||||
)
|
||||
|
||||
@strawberry.mutation(permission_classes=[IsAuthenticated])
|
||||
def nix_collect_garbage(self) -> GenericJobMutationReturn:
|
||||
job = start_nix_collect_garbage()
|
||||
|
||||
|
||||
return GenericJobMutationReturn(
|
||||
success=True,
|
||||
code=200,
|
||||
message="Garbage collector started...",
|
||||
job=job_to_api_job(job),
|
||||
)
|
||||
|
|
|
@ -67,8 +67,8 @@ def move_folder(
|
|||
|
||||
try:
|
||||
data_path.mkdir(mode=0o750, parents=True, exist_ok=True)
|
||||
except Exception as e:
|
||||
print(f"Error creating data path: {e}")
|
||||
except Exception as error:
|
||||
print(f"Error creating data path: {error}")
|
||||
return
|
||||
|
||||
try:
|
||||
|
|
|
@ -0,0 +1,154 @@
|
|||
import re
|
||||
import subprocess
|
||||
from typing import Tuple, Iterable
|
||||
|
||||
from selfprivacy_api.utils.huey import huey
|
||||
|
||||
from selfprivacy_api.jobs import JobStatus, Jobs, Job
|
||||
|
||||
def marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
typo typo
|
||||
|
||||
class ShellException(Exception):
|
||||
inex marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
So the user sees "We are sorry, result was not found :(" after starting garbage collection. What would this even mean? So the user sees "We are sorry, result was not found :(" after starting garbage collection. What would this even mean?
def
commented
Outdated
Review
done done
inex
commented
Outdated
Review
Remove emotions from the message. Remove emotions from the message.
def
commented
Outdated
Review
done done
|
||||
"""Shell-related errors"""
|
||||
|
||||
|
||||
COMPLETED_WITH_ERROR = "Error occurred, please report this to the support chat."
|
||||
RESULT_WAS_NOT_FOUND_ERROR = (
|
||||
"We are sorry, garbage collection result was not found. "
|
||||
"Something went wrong, please report this to the support chat."
|
||||
inex marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
Variable Variable `PIPE` not allowed in type expression
def
commented
Outdated
Review
done done
|
||||
)
|
||||
CLEAR_COMPLETED = "Garbage collection completed."
|
||||
|
||||
|
||||
def delete_old_gens_and_return_dead_report() -> str:
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
Add typing to functions Add typing to functions
def
commented
Outdated
Review
done done
|
||||
subprocess.run(
|
||||
["nix-env", "-p", "/nix/var/nix/profiles/system", "--delete-generations old"],
|
||||
check=False,
|
||||
)
|
||||
|
||||
result = subprocess.check_output(["nix-store", "--gc", "--print-dead"]).decode(
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
You mean some very specific type of line here. Please, paste an example as a docstring and maybe rename function for clarity You mean some very specific type of line here. Please, paste an example as a docstring and maybe rename function for clarity
def
commented
Outdated
Review
done done
inex
commented
Outdated
Review
Expression of type "IO[bytes] | None" cannot be assigned to return type "Popen[Unknown]" Expression of type "IO[bytes] | None" cannot be assigned to return type "Popen[Unknown]"
Type "IO[bytes] | None" cannot be assigned to type "Popen[Unknown]"
"IO[bytes]" is incompatible with "Popen[Unknown]"
def
commented
Outdated
Review
done done
|
||||
"utf-8"
|
||||
)
|
||||
|
||||
return " " if result is None else result
|
||||
|
||||
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
There is There is `JobStatus.Error`, please use it
inex
commented
Outdated
Review
Also, the whole tuple doesn't look very good. Why do you return the Also, the whole tuple doesn't look very good. Why do you return the `tuple[Literal[JobStatus.FINISHED], Literal[100], Literal['Completed with an error'], Literal['We are sorry, result was not found :(']] | tuple[Literal[JobStatus.FINISHED], Literal[100], Literal['Cleaning completed.'], str]` type?
def
commented
Outdated
Review
(1) done (1) done
|
||||
def run_nix_collect_garbage() -> Iterable[bytes]:
|
||||
process = subprocess.Popen(
|
||||
["nix-store", "--gc"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT
|
||||
)
|
||||
return process.stdout if process.stdout else iter([])
|
||||
|
||||
|
||||
def parse_line(job: Job, line: str) -> Job:
|
||||
"""
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
In current version of jobs, it is not necessary to set progress to 100 explicitly when finishing the job In current version of jobs, it is not necessary to set progress to 100 explicitly when finishing the job
def
commented
Outdated
Review
Okay, but I won't change it so I don't have to rewrite the argument return. Okay, but I won't change it so I don't have to rewrite the argument return.
inex
commented
Outdated
Review
you should rewrite the return type of this function you should rewrite the return type of this function
def
commented
Outdated
Review
done done
|
||||
We parse the string for the presence of a final line,
|
||||
with the final amount of space cleared.
|
||||
Simply put, we're just looking for a similar string:
|
||||
houkime
commented
Outdated
Review
You can pass job as an argument to a function to not have these complex resturns You can pass job as an argument to a function to not have these complex resturns
def
commented
Outdated
Review
Yeah, but I've already done it my way Yeah, but I've already done it my way
inex
commented
Outdated
Review
you didn't you didn't
|
||||
"1537 store paths deleted, 339.84 MiB freed".
|
||||
"""
|
||||
pattern = re.compile(r"[+-]?\d+\.\d+ \w+(?= freed)")
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
Consider renaming to Consider renaming to `process_stream`.
We do not stream processes here, we process streams.
def
commented
Outdated
Review
done done
houkime
commented
Outdated
Review
Errored jobs are currently not handled by this endpoint. Errored jobs are currently not handled by this endpoint.
Add a handler and a test where the call errors out.
|
||||
match = re.search(pattern, line)
|
||||
|
||||
if match is None:
|
||||
raise ShellException("nix returned gibberish output")
|
||||
|
||||
else:
|
||||
Jobs.update(
|
||||
job=job,
|
||||
status=JobStatus.FINISHED,
|
||||
status_text=CLEAR_COMPLETED,
|
||||
result=f"{match.group(0)} have been cleared",
|
||||
)
|
||||
inex marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
no typing no typing
def
commented
Outdated
Review
done done
|
||||
return job
|
||||
|
||||
|
||||
def process_stream(job: Job, stream: Iterable[bytes], total_dead_packages: int) -> None:
|
||||
completed_packages = 0
|
||||
prev_progress = 0
|
||||
|
||||
for line in stream:
|
||||
line = line.decode("utf-8")
|
||||
def marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
This may lead to percents not summing up to 100. You better count the number of paths deleted and divide them by a total. This may lead to percents not summing up to 100. You better count the number of paths deleted and divide them by a total.
|
||||
|
||||
if "deleting '/nix/store/" in line:
|
||||
completed_packages += 1
|
||||
percent = int((completed_packages / total_dead_packages) * 100)
|
||||
|
||||
if percent - prev_progress >= 5:
|
||||
Jobs.update(
|
||||
job=job,
|
||||
status=JobStatus.RUNNING,
|
||||
progress=percent,
|
||||
status_text="Cleaning...",
|
||||
)
|
||||
prev_progress = percent
|
||||
|
||||
elif "store paths deleted," in line:
|
||||
parse_line(job, line)
|
||||
|
||||
|
||||
def get_dead_packages(output) -> Tuple[int, float]:
|
||||
dead = len(re.findall("/nix/store/", output))
|
||||
percent = 0
|
||||
if dead != 0:
|
||||
inex marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
no typing no typing
def
commented
Outdated
Review
done done
|
||||
percent = 100 / dead
|
||||
return dead, percent
|
||||
|
||||
|
||||
@huey.task()
|
||||
def calculate_and_clear_dead_paths(job: Job):
|
||||
Jobs.update(
|
||||
job=job,
|
||||
status=JobStatus.RUNNING,
|
||||
progress=0,
|
||||
status_text="Calculate the number of dead packages...",
|
||||
inex marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
You've declared that You've declared that `job` is of `Jobs` type, but this function expects the `Job` type.
def
commented
Outdated
Review
done done
inex
commented
Outdated
Review
You've broken it even further. Also improve your tests to check the status of Job object. You've broken it even further. Also improve your tests to check the status of Job object.
def
commented
Outdated
Review
the error highlighting tells me that I've definitely fixed it now. the error highlighting tells me that I've definitely fixed it now.
def
commented
Outdated
Review
done done
|
||||
)
|
||||
|
||||
dead_packages, package_equal_to_percent = get_dead_packages(
|
||||
delete_old_gens_and_return_dead_report()
|
||||
)
|
||||
|
||||
if dead_packages == 0:
|
||||
Jobs.update(
|
||||
job=job,
|
||||
status=JobStatus.FINISHED,
|
||||
status_text="Nothing to clear",
|
||||
result="System is clear",
|
||||
)
|
||||
return True
|
||||
|
||||
Jobs.update(
|
||||
job=job,
|
||||
status=JobStatus.RUNNING,
|
||||
progress=0,
|
||||
status_text=f"Found {dead_packages} packages to remove!",
|
||||
)
|
||||
|
||||
stream = run_nix_collect_garbage()
|
||||
try:
|
||||
process_stream(job, stream, dead_packages)
|
||||
except ShellException as error:
|
||||
Jobs.update(
|
||||
job=job,
|
||||
status=JobStatus.ERROR,
|
||||
status_text=COMPLETED_WITH_ERROR,
|
||||
error=RESULT_WAS_NOT_FOUND_ERROR,
|
||||
)
|
||||
|
||||
|
||||
def start_nix_collect_garbage() -> Job:
|
||||
job = Jobs.add(
|
||||
type_id="maintenance.collect_nix_garbage",
|
||||
name="Collect garbage",
|
||||
description="Cleaning up unused packages",
|
||||
)
|
||||
|
||||
calculate_and_clear_dead_paths(job=job)
|
||||
|
||||
Jobs.update(
|
||||
job=job,
|
||||
status=JobStatus.FINISHED,
|
||||
status_text="Collect garbage finished",
|
||||
progress=100,
|
||||
)
|
||||
|
||||
return job
|
|
@ -7,6 +7,8 @@ from selfprivacy_api.services.tasks import move_service
|
|||
from selfprivacy_api.jobs.upgrade_system import rebuild_system_task
|
||||
|
||||
from selfprivacy_api.jobs.test import test_job
|
||||
from selfprivacy_api.jobs.nix_collect_garbage import calculate_and_clear_dead_paths
|
||||
|
||||
|
||||
if environ.get("TEST_MODE"):
|
||||
from tests.test_huey import sum
|
||||
|
|
|
@ -0,0 +1,229 @@
|
|||
# pylint: disable=redefined-outer-name
|
||||
# pylint: disable=unused-argument
|
||||
# pylint: disable=missing-function-docstring
|
||||
|
||||
import pytest
|
||||
from selfprivacy_api.utils.huey import huey
|
||||
|
||||
from selfprivacy_api.jobs import JobStatus, Jobs
|
||||
from tests.test_graphql.common import (
|
||||
get_data,
|
||||
assert_ok,
|
||||
assert_empty,
|
||||
)
|
||||
|
||||
from selfprivacy_api.jobs.nix_collect_garbage import (
|
||||
get_dead_packages,
|
||||
parse_line,
|
||||
ShellException,
|
||||
)
|
||||
|
||||
OUTPUT_PRINT_DEAD = """
|
||||
inex marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
- Do you actually need this dependency?
- This breaks CI. New deps must be discussed.
def
commented
Outdated
Review
done done
def
commented
Outdated
Review
deadcode deadcode
|
||||
finding garbage collector roots...
|
||||
determining live/dead paths...
|
||||
/nix/store/02k8pmw00p7p7mf2dg3n057771w7liia-python3.10-cchardet-2.1.7
|
||||
/nix/store/03vc6dznx8njbvyd3gfhfa4n5j4lvhbl-python3.10-async-timeout-4.0.2
|
||||
/nix/store/03ybv2dvfk7c3cpb527y5kzf6i35ch41-python3.10-pycparser-2.21
|
||||
/nix/store/04dn9slfqwhqisn1j3jv531lms9w5wlj-python3.10-hypothesis-6.50.1.drv
|
||||
/nix/store/04hhx2z1iyi3b48hxykiw1g03lp46jk7-python-remove-bin-bytecode-hook
|
||||
"""
|
||||
|
||||
|
||||
OUTPUT_COLLECT_GARBAGE = """
|
||||
removing old generations of profile /nix/var/nix/profiles/per-user/def/channels
|
||||
finding garbage collector roots...
|
||||
deleting garbage...
|
||||
deleting '/nix/store/02k8pmw00p7p7mf2dg3n057771w7liia-python3.10-cchardet-2.1.7'
|
||||
deleting '/nix/store/03vc6dznx8njbvyd3gfhfa4n5j4lvhbl-python3.10-async-timeout-4.0.2'
|
||||
deleting '/nix/store/03ybv2dvfk7c3cpb527y5kzf6i35ch41-python3.10-pycparser-2.21'
|
||||
deleting '/nix/store/04dn9slfqwhqisn1j3jv531lms9w5wlj-python3.10-hypothesis-6.50.1.drv'
|
||||
deleting '/nix/store/04hhx2z1iyi3b48hxykiw1g03lp46jk7-python-remove-bin-bytecode-hook'
|
||||
deleting unused links...
|
||||
note: currently hard linking saves -0.00 MiB
|
||||
190 store paths deleted, 425.51 MiB freed
|
||||
"""
|
||||
|
||||
OUTPUT_COLLECT_GARBAGE_ZERO_TRASH = """
|
||||
removing old generations of profile /nix/var/nix/profiles/per-user/def/profile
|
||||
removing old generations of profile /nix/var/nix/profiles/per-user/def/channels
|
||||
finding garbage collector roots...
|
||||
deleting garbage...
|
||||
deleting unused links...
|
||||
note: currently hard linking saves 0.00 MiB
|
||||
0 store paths deleted, 0.00 MiB freed
|
||||
"""
|
||||
|
||||
|
||||
# ---
|
||||
|
||||
|
||||
inex
commented
Outdated
Review
I would suggest mocking system calls, and not your own functions. I would suggest mocking system calls, and not your own functions.
|
||||
def test_parse_line():
|
||||
txt = "note: currently hard linking saves -0.00 MiB 190 store paths deleted, 425.51 MiB freed"
|
||||
|
||||
job = Jobs.add(
|
||||
name="name",
|
||||
type_id="parse_line",
|
||||
description="description",
|
||||
)
|
||||
|
||||
output = parse_line(job, txt)
|
||||
assert output.result == "425.51 MiB have been cleared"
|
||||
assert output.status == JobStatus.FINISHED
|
||||
assert output.error is None
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
what are you trying to do here exactly?(dead code?) what are you trying to do here exactly?(dead code?)
def
commented
Outdated
Review
done done
|
||||
|
||||
|
||||
def test_parse_line_with_blank_line():
|
||||
txt = ""
|
||||
job = Jobs.add(
|
||||
name="name",
|
||||
type_id="parse_line",
|
||||
description="description",
|
||||
)
|
||||
|
||||
with pytest.raises(ShellException):
|
||||
output = parse_line(job, txt)
|
||||
|
||||
|
||||
def test_get_dead_packages():
|
||||
assert get_dead_packages(OUTPUT_PRINT_DEAD) == (5, 20.0)
|
||||
|
||||
|
||||
def test_get_dead_packages_zero():
|
||||
assert get_dead_packages("") == (0, 0)
|
||||
|
||||
|
||||
RUN_NIX_COLLECT_GARBAGE_MUTATION = """
|
||||
mutation CollectGarbage {
|
||||
system {
|
||||
nixCollectGarbage {
|
||||
success
|
||||
message
|
||||
code
|
||||
job {
|
||||
uid,
|
||||
typeId,
|
||||
name,
|
||||
description,
|
||||
status,
|
||||
statusText,
|
||||
progress,
|
||||
createdAt,
|
||||
updatedAt,
|
||||
finishedAt,
|
||||
error,
|
||||
result,
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
"""
|
||||
|
||||
|
||||
def test_graphql_nix_collect_garbage(authorized_client, fp):
|
||||
assert huey.immediate is True
|
||||
|
||||
fp.register(
|
||||
["nix-env", "-p", "/nix/var/nix/profiles/system", "--delete-generations old"],
|
||||
stdout="",
|
||||
inex
commented
Outdated
Review
Lacking test of trying to call mutation without auth Lacking test of trying to call mutation without auth
def
commented
Outdated
Review
done done
|
||||
)
|
||||
fp.register(["nix-store", "--gc", "--print-dead"], stdout=OUTPUT_PRINT_DEAD)
|
||||
fp.register(["nix-store", "--gc"], stdout=OUTPUT_COLLECT_GARBAGE)
|
||||
inex
commented
Outdated
Review
This test just hangs and doesn't finish This test just hangs and doesn't finish
def
commented
Outdated
Review
done done
|
||||
|
||||
response = authorized_client.post(
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
This is not a query, this is a Mutation. Both semantically and you wrote it yourself. This is not a query, this is a Mutation. Both semantically and you wrote it yourself.
def
commented
Outdated
Review
If you mean I don't ask for mutation fields, I've changed that. so, done? If you mean I don't ask for mutation fields, I've changed that.
so, done?
inex
commented
Outdated
Review
no, it is about naming. no, it is about naming.
def
commented
Outdated
Review
done done
|
||||
"/graphql",
|
||||
json={
|
||||
"query": RUN_NIX_COLLECT_GARBAGE_MUTATION,
|
||||
},
|
||||
)
|
||||
|
||||
output = get_data(response)["system"]["nixCollectGarbage"]
|
||||
assert_ok(output)
|
||||
inex marked this conversation as resolved
Outdated
houkime
commented
Outdated
Review
`is True` already checks that it is a boolean and it is True
def
commented
Outdated
Review
done done
|
||||
assert output["job"] is not None
|
||||
assert output["job"]["status"] == "FINISHED"
|
||||
assert output["job"]["error"] is None
|
||||
|
||||
assert (
|
||||
fp.call_count(
|
||||
[
|
||||
"nix-env",
|
||||
"-p",
|
||||
"/nix/var/nix/profiles/system",
|
||||
"--delete-generations old",
|
||||
]
|
||||
)
|
||||
== 1
|
||||
)
|
||||
assert fp.call_count(["nix-store", "--gc", "--print-dead"]) == 1
|
||||
assert fp.call_count(["nix-store", "--gc"]) == 1
|
||||
|
||||
|
||||
def test_graphql_nix_collect_garbage_return_zero_trash(authorized_client, fp):
|
||||
assert huey.immediate is True
|
||||
|
||||
fp.register(
|
||||
["nix-env", "-p", "/nix/var/nix/profiles/system", "--delete-generations old"],
|
||||
stdout="",
|
||||
)
|
||||
fp.register(["nix-store", "--gc", "--print-dead"], stdout=OUTPUT_PRINT_DEAD)
|
||||
fp.register(["nix-store", "--gc"], stdout=OUTPUT_COLLECT_GARBAGE_ZERO_TRASH)
|
||||
|
||||
response = authorized_client.post(
|
||||
"/graphql",
|
||||
json={
|
||||
"query": RUN_NIX_COLLECT_GARBAGE_MUTATION,
|
||||
},
|
||||
)
|
||||
|
||||
output = get_data(response)["system"]["nixCollectGarbage"]
|
||||
assert_ok(output)
|
||||
assert output["job"] is not None
|
||||
assert output["job"]["status"] == "FINISHED"
|
||||
assert output["job"]["error"] is None
|
||||
|
||||
assert (
|
||||
fp.call_count(
|
||||
[
|
||||
"nix-env",
|
||||
"-p",
|
||||
"/nix/var/nix/profiles/system",
|
||||
"--delete-generations old",
|
||||
]
|
||||
)
|
||||
== 1
|
||||
)
|
||||
assert fp.call_count(["nix-store", "--gc", "--print-dead"]) == 1
|
||||
def marked this conversation as resolved
Outdated
alexoundos
commented
Outdated
Review
Is it cyrillic letter Is it cyrillic letter `С`?
inex
commented
Outdated
Review
yes, it is, @def should have fixed this. yes, it is, @def should have fixed this.
|
||||
assert fp.call_count(["nix-store", "--gc"]) == 1
|
||||
|
||||
|
||||
def test_graphql_nix_collect_garbage_not_authorized_client(client, fp):
|
||||
assert huey.immediate is True
|
||||
|
||||
fp.register(
|
||||
["nix-env", "-p", "/nix/var/nix/profiles/system", "--delete-generations old"],
|
||||
stdout="",
|
||||
)
|
||||
fp.register(["nix-store", "--gc", "--print-dead"], stdout=OUTPUT_PRINT_DEAD)
|
||||
fp.register(["nix-store", "--gc"], stdout=OUTPUT_COLLECT_GARBAGE)
|
||||
|
||||
response = client.post(
|
||||
"/graphql",
|
||||
json={
|
||||
"query": RUN_NIX_COLLECT_GARBAGE_MUTATION,
|
||||
},
|
||||
)
|
||||
|
||||
assert_empty(response)
|
||||
|
||||
assert (
|
||||
fp.call_count(
|
||||
[
|
||||
"nix-env",
|
||||
"-p",
|
||||
"/nix/var/nix/profiles/system",
|
||||
"--delete-generations old",
|
||||
]
|
||||
)
|
||||
def marked this conversation as resolved
Outdated
inex
commented
Outdated
Review
status update? status update?
|
||||
== 0
|
||||
)
|
||||
assert fp.call_count(["nix-store", "--gc", "--print-dead"]) == 0
|
||||
assert fp.call_count(["nix-store", "--gc"]) == 0
|
add a handler for an error too