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

Refactor Job Implementation #11

Closed
1 task done
fonzdm opened this issue Apr 9, 2024 · 19 comments · Fixed by #24
Closed
1 task done

Refactor Job Implementation #11

fonzdm opened this issue Apr 9, 2024 · 19 comments · Fixed by #24
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@fonzdm
Copy link
Owner

fonzdm commented Apr 9, 2024

Description

At the moment, several pre and post deploy helm jobs are implemented as a series of containers running a "curl equipped" docker image that execute a single, hard to read, GET/POST request.
This introduces several fingerprint overhead and makes it difficult to edit or modify the requests themselves.
Moreover, evil escapes are needed to make a proper curl command into the command section of a container, for example:

command:
- "/bin/sh"
- "-ec"
args:
- "curl -k -v -X POST \"http://servarr-prowlarr.{{ $.Release.Namespace }}.svc.cluster.local:9696/api/v1/indexer\" -d '{{ .body | toJson | print | replace "\"" "\\\"" }}' -H 'Content-Type: application/json' -H 'X-Api-Key: $(APIKEY)' -H 'X-Prowlarr-Client: true' -H 'X-Requested-With: XMLHttpRequest'"

Solution you'd like

It would be nice to replace this flow entirely with a proper python/easy-to-read language, as already done for the Jellyseerr post init job:

command:
- "/bin/sh"
- "-ec"
args:
- "python3 -u /mnt/init-jellyseerr.py 2>&1;"

The script can be loaded into a configmap and mounted in the hook job, for example:

volumeMounts:
- mountPath: "/mnt"
name: python-script
volumes:
- name: python-script
configMap:
name: init-jellyseerr-python-scripts

Code of Conduct

  • I agree to follow this project's Code of Conduct
@fonzdm fonzdm added enhancement New feature or request needs triage This issue may need triage. Remove it if it has been sufficiently triaged labels Apr 9, 2024
@fonzdm
Copy link
Owner Author

fonzdm commented Apr 9, 2024

@imgios do you feel like taking this? 😃

@imgios
Copy link
Collaborator

imgios commented Apr 9, 2024

Sure, I can give it a try 😄

@imgios
Copy link
Collaborator

imgios commented Apr 9, 2024

@fonzdm I was checking the project structure that's currently storing the init-jellyseerr.py script in /config/jellyseerr, but this could be misleading when refactoring the init-downloaders, because it includes all the dependencies init.

What do you think if we place them all in a common directory since we are going to create several Python scripts? I was thinking to something like this:

.
└── config/
    └── scripts/
        └── *.py

@fonzdm
Copy link
Owner Author

fonzdm commented Apr 9, 2024

What do you think if we place them all in a common directory since we are going to create several Python scripts?

Sounds good! And this will make life easier in case we rely on a single ConfigMap with all the scripts.

@imgios imgios self-assigned this Apr 10, 2024
@imgios
Copy link
Collaborator

imgios commented Apr 10, 2024

I was checking the init-downloaders job, and I can see that you have a container for each configuration you have to do. I like this approach, this means we may proceed for the init-downloaders script with a single script with x functions, where x is the configurations you have to do, and we can execute them by doing something like that: init_downloaders.py configure-x.

What's your POV on that?

@fonzdm
Copy link
Owner Author

fonzdm commented Apr 10, 2024

a single script with x functions, where x is the configurations you have to do, and we can execute them by doing something like that: init_downloaders.py configure-x.

It's nice and could allow us to keep the current structure AND have a readable configuration.
Just to point out, we currently have multiple requests in the same jobs towards different services.
Instead of moving everything in one script, what about using different scripts, one per services? (for example, as already done for jellyseerr only)

@imgios
Copy link
Collaborator

imgios commented Apr 11, 2024

Instead of moving everything in one script, what about using different scripts, one per services? (for example, as already done for jellyseerr only)

This is another valid option, would be good to evaluate pros/cons for both solution and see which one may be the best to adopt.

@fonzdm
Copy link
Owner Author

fonzdm commented Apr 11, 2024

Ok, let's write down some points.

Edit: of course is more of an aesthetic thing, as the best method to rule them all is to create a dedicated docker image and write some small library... but it would be another thing to create/maintain/publish.

Single Script Multiple Scripts
One implementation (reuse, fix and so on) Different copies of the same functions (for example, the basic GET/POST methods
Dependency of different APIs in the same code Each script refers to a service (and so a set of APIs)
One/Few jobs with multiple containers One job per service, cleaner
Difficult to split and maintain work (one man for the whole script) Work is splittable and maintainable (one man per service)
Easier to test (one invocation = one command) Harder to test (one flow per service with multiple operations)
Easier to add a add a single operation (and ensure idempotency) Easier to integrate services (for example, readarr is not integrated at the moment, but I wish to do it soon)

Some psudo code for invocation

Single Script

      containers:
        - name: <service>-operation-1
          image: python:latest
          command:
            - "/bin/sh"
            - "-ec"
          args:
            - "python3 -u /mnt/sevarr-configuration.py <service>-operation-1" 
        - name: <service>-operation-2
          image: python:latest
          command:
            - "/bin/sh"
            - "-ec"
          args:
            - "python3 -u /mnt/sevarr-configuration.py <service>-operation-2"

Each parameter passed to the script will result in one/few API calls to configure a single feature.

Multiple Scripts

One job per service, each service will be like:

        env:
          - name: ENV_VAR
            value: VALUE
        command:
          - "/bin/sh"
          - "-ec"
        args:
          - "python3 -u /mnt/init-<service>.py 2>&1;"

And the /mnt/init-<service>.py will execute everything needed for that service (for example, prowlarr, with 4 or 5 APIs)

What do you think? I would personally go for the second approach 😄

@imgios
Copy link
Collaborator

imgios commented Apr 11, 2024

Thanks for your POV on that, @fonzdm. I also like the second approach if we do it for each app/service that should be configured, e.g.,:

  • init-prowlarr.py: this will be used to configure everything related to Prowlarr and not just a part of it
  • init-radarr.py: this will be used to configure everything related to Radarr and not just a part of it
  • and so on.

Otherwise, if we have to create one script per operation (e.g., one script per API request) then this doesn't make so sense to me, because it means having a lot of small script to maintain and a bunch of duplicated code 😊

@imgios
Copy link
Collaborator

imgios commented Apr 12, 2024

Hey @fonzdm, during the Jellyfin refactoring I noticed that there is the APIKEY env variable being load in each job container, but actually no one is really using it - so I didn't add it to the script anymore. Moreover, I have seen that you configure both the COUNTRY_CODE and the PREFERRED_LANGUAGE. I made the script to load them from the env and assign a default value if they are missing:

COUNTRY_CODE = os.getenv("COUNTRY_CODE", "IT")
PREFERRED_LANGUAGE = os.getenv("PREFERRED_LANGUAGE", "it")

We should probably think about it and maybe expose them in the Chart values.

Furthermore, I have a question regarding init-downloaders job: is there any precise sequence that must be followed? Can we safely split those configuration into three different scripts (one for Prowlarr, one for Servarr, and one for Radarr)?

@fonzdm
Copy link
Owner Author

fonzdm commented Apr 12, 2024

Hey @fonzdm, during the Jellyfin refactoring I noticed that there is the APIKEY env variable being load in each job container, but actually no one is really using it - so I didn't add it to the script anymore.

Nice, it was a leftover.

Moreover, I have seen that you configure both the COUNTRY_CODE and the PREFERRED_LANGUAGE. I made the script to load them from the env and assign a default value if they are missing:

COUNTRY_CODE = os.getenv("COUNTRY_CODE", "IT")
PREFERRED_LANGUAGE = os.getenv("PREFERRED_LANGUAGE", "it")

We should probably think about it and maybe expose them in the Chart values.

I can work on that, no worries.

Furthermore, I have a question regarding init-downloaders job: is there any precise sequence that must be followed? Can we safely split those configuration into three different scripts (one for Prowlarr, one for Servarr, and one for Radarr)?

I would say to keep the sequence of the job as it is but just split into different scripts.
In other words, the init-prwolarr.py script will have the same sequence of the APIs towards prowlarr in the current job (and only them).
The real sequence is between services:

  1. Torrent (it's actually a pre-install hook and does not make use of APIs, for now...)
  2. Radarr/Sonarr (can be done in parallel)
  3. Prowlarr
  4. Jellyfin
  5. Jellyseerr

@fonzdm
Copy link
Owner Author

fonzdm commented Apr 17, 2024

@imgios , just tried to deploy the service, I'm getting the following error:

k-user@k-node-0:~$ kubectl logs -n servarr radarr-init-9gcrp
Defaulted container "initialize-radarr" out of: initialize-radarr, wait-for-radarr (init)
2024-04-17 09:00:36,254 - __main__ - INFO - Setup qBitTorrent in Radarr
2024-04-17 09:00:36,254 - __main__ - DEBUG - POST http://servarr-radarr.servarr.svc.cluster.local:7878/api/v3/downloadclient c: o: n: t: e: n: t: -: t: y: p: e, x: -: a: p: i: -: k: e: y, x: -: r: e: q: u: e: s: t: e: d: -: w: i: t: h {'enable': 'true', 'protocol': 'torrent', 'priority': 1, 'removeCompletedDownloads': 'true', 'removeFailedDownloads': 'true', 'name': 'qBittorrent', 'fields': [{'name': 'host', 'value': 'servarr-qbittorrent'}, {'name': 'port', 'value': '10095'}, {'name': 'useSsl', 'value': 'false'}, {'name': 'urlBase'}, {'name': 'username', 'value': 'admin'}, {'name': 'password', 'value': 'v1c2X3Z4'}, {'name': 'movieCategory', 'value': 'radarr'}, {'name': 'movieImportedCategory'}, {'name': 'recentMoviePriority', 'value': 0}, {'name': 'olderMoviePriority', 'value': 0}, {'name': 'initialState', 'value': 0}, {'name': 'sequentialOrder', 'value': 'false'}, {'name': 'firstAndLast', 'value': 'false'}, {'name': 'contentLayout', 'value': 0}], 'implementationName': 'qBittorrent', 'implementation': 'QBittorrent', 'configContract': 'QBittorrentSettings', 'infoLink': 'https://wiki.servarr.com/radarr/supported#qbittorrent', 'tags': []}
Traceback (most recent call last):
  File "/mnt/init-radarr.py", line 139, in <module>
    post(
  File "/mnt/init-radarr.py", line 49, in post
    logger.debug(" ".join([
TypeError: sequence item 1: expected str instance, int found

Let me know if you need something else!

@imgios
Copy link
Collaborator

imgios commented Apr 17, 2024

@imgios , just tried to deploy the service, I'm getting the following error:

k-user@k-node-0:~$ kubectl logs -n servarr radarr-init-9gcrp
Defaulted container "initialize-radarr" out of: initialize-radarr, wait-for-radarr (init)
2024-04-17 09:00:36,254 - __main__ - INFO - Setup qBitTorrent in Radarr
2024-04-17 09:00:36,254 - __main__ - DEBUG - POST http://servarr-radarr.servarr.svc.cluster.local:7878/api/v3/downloadclient c: o: n: t: e: n: t: -: t: y: p: e, x: -: a: p: i: -: k: e: y, x: -: r: e: q: u: e: s: t: e: d: -: w: i: t: h {'enable': 'true', 'protocol': 'torrent', 'priority': 1, 'removeCompletedDownloads': 'true', 'removeFailedDownloads': 'true', 'name': 'qBittorrent', 'fields': [{'name': 'host', 'value': 'servarr-qbittorrent'}, {'name': 'port', 'value': '10095'}, {'name': 'useSsl', 'value': 'false'}, {'name': 'urlBase'}, {'name': 'username', 'value': 'admin'}, {'name': 'password', 'value': 'v1c2X3Z4'}, {'name': 'movieCategory', 'value': 'radarr'}, {'name': 'movieImportedCategory'}, {'name': 'recentMoviePriority', 'value': 0}, {'name': 'olderMoviePriority', 'value': 0}, {'name': 'initialState', 'value': 0}, {'name': 'sequentialOrder', 'value': 'false'}, {'name': 'firstAndLast', 'value': 'false'}, {'name': 'contentLayout', 'value': 0}], 'implementationName': 'qBittorrent', 'implementation': 'QBittorrent', 'configContract': 'QBittorrentSettings', 'infoLink': 'https://wiki.servarr.com/radarr/supported#qbittorrent', 'tags': []}
Traceback (most recent call last):
  File "/mnt/init-radarr.py", line 139, in <module>
    post(
  File "/mnt/init-radarr.py", line 49, in post
    logger.debug(" ".join([
TypeError: sequence item 1: expected str instance, int found

Let me know if you need something else!

Hey @fonzdm, thanks for checking it! Will let you know 😄

@imgios
Copy link
Collaborator

imgios commented Apr 17, 2024

@imgios , just tried to deploy the service, I'm getting the following error:

k-user@k-node-0:~$ kubectl logs -n servarr radarr-init-9gcrp
Defaulted container "initialize-radarr" out of: initialize-radarr, wait-for-radarr (init)
2024-04-17 09:00:36,254 - __main__ - INFO - Setup qBitTorrent in Radarr
2024-04-17 09:00:36,254 - __main__ - DEBUG - POST http://servarr-radarr.servarr.svc.cluster.local:7878/api/v3/downloadclient c: o: n: t: e: n: t: -: t: y: p: e, x: -: a: p: i: -: k: e: y, x: -: r: e: q: u: e: s: t: e: d: -: w: i: t: h {'enable': 'true', 'protocol': 'torrent', 'priority': 1, 'removeCompletedDownloads': 'true', 'removeFailedDownloads': 'true', 'name': 'qBittorrent', 'fields': [{'name': 'host', 'value': 'servarr-qbittorrent'}, {'name': 'port', 'value': '10095'}, {'name': 'useSsl', 'value': 'false'}, {'name': 'urlBase'}, {'name': 'username', 'value': 'admin'}, {'name': 'password', 'value': 'v1c2X3Z4'}, {'name': 'movieCategory', 'value': 'radarr'}, {'name': 'movieImportedCategory'}, {'name': 'recentMoviePriority', 'value': 0}, {'name': 'olderMoviePriority', 'value': 0}, {'name': 'initialState', 'value': 0}, {'name': 'sequentialOrder', 'value': 'false'}, {'name': 'firstAndLast', 'value': 'false'}, {'name': 'contentLayout', 'value': 0}], 'implementationName': 'qBittorrent', 'implementation': 'QBittorrent', 'configContract': 'QBittorrentSettings', 'infoLink': 'https://wiki.servarr.com/radarr/supported#qbittorrent', 'tags': []}
Traceback (most recent call last):
  File "/mnt/init-radarr.py", line 139, in <module>
    post(
  File "/mnt/init-radarr.py", line 49, in post
    logger.debug(" ".join([
TypeError: sequence item 1: expected str instance, int found

Let me know if you need something else!

The Python join() works on the type Iterable[str] while I was passing also the reponse status code that is an int.

Now, casting it to string should have fixed it:

logger.debug(" ".join([
"Status Code:",
str(response.status_code),
"Response body:",
response.text
]))

@fonzdm
Copy link
Owner Author

fonzdm commented Apr 17, 2024

Tried again, here's the result:

k get pods -n servarr
NAME                                   READY   STATUS      RESTARTS   AGE
pre-install-job-2rlbp                  0/1     Completed   0          9m39s
servarr-flaresolverr-7857b84d4-xhz2r   1/1     Running     0          8m11s
servarr-qbittorrent-5f4f99d95c-gpbcq   1/1     Running     0          8m11s
servarr-jellyfin-7cfdc9466c-pxvtc      1/1     Running     0          8m12s
servarr-jellyseerr-74896db869-s5blz    1/1     Running     0          8m12s
servarr-prowlarr-9fb778f47-4mxj7       1/1     Running     0          8m12s
servarr-radarr-75f6fb5bcc-hgbw9        1/1     Running     0          8m12s
radarr-init-zq6m9                      0/1     Completed   0          8m11s
servarr-sonarr-858d56ccc5-4xh9f        1/1     Running     0          8m12s
sonarr-init-p5ncs                      0/1     Completed   0          5m33s
prowlarr-init-p9zzk                    0/1     Completed   0          5m11s
jellyfin-init-h2m4b                    0/1     Error       0          5m4s
jellyfin-init-87px2                    0/1     Error       0          4m41s

But i get 401 on sonarr, radarr, prwolarr (seems like an API key not set):

kubectl logs -n servarr sonarr-init-p5ncs
Defaulted container "initialize-sonarr" out of: initialize-sonarr, wait-for-sonarr (init)
2024-04-17 14:31:19,592 - __main__ - INFO - Setup Sonarr and qBitTorrent interworking
2024-04-17 14:31:19,592 - __main__ - DEBUG - POST http://servarr-sonarr.servarr.svc.cluster.local:8989/api/v3/downloadclient content-type: application/json, x-api-key: None, x-requested-with: XMLHttpRequest {'enable': 'true', 'protocol': 'torrent', 'priority': 1, 'removeCompletedDownloads': 'true', 'removeFailedDownloads': 'true', 'name': 'qBittorrent', 'fields': [{'name': 'host', 'value': 'servarr-qbittorrent'}, {'name': 'port', 'value': '10095'}, {'name': 'useSsl', 'value': 'false'}, {'name': 'urlBase'}, {'name': 'username', 'value': 'admin'}, {'name': 'password', 'value': '<pass>'}, {'name': 'movieCategory', 'value': 'radarr'}, {'name': 'movieImportedCategory'}, {'name': 'recentMoviePriority', 'value': 0}, {'name': 'olderMoviePriority', 'value': 0}, {'name': 'initialState', 'value': 0}, {'name': 'sequentialOrder', 'value': 'false'}, {'name': 'firstAndLast', 'value': 'false'}, {'name': 'contentLayout', 'value': 0}], 'implementationName': 'qBittorrent', 'implementation': 'QBittorrent', 'configContract': 'QBittorrentSettings', 'infoLink': 'https://wiki.servarr.com/radarr/supported#qbittorrent', 'tags': []}
2024-04-17 14:31:19,648 - __main__ - DEBUG - Status Code: 401 Response body:

and jellyfin is completely in error.

kubectl logs -n servarr jellyfin-init-h2m4b
Defaulted container "initialize-jellyfin" out of: initialize-jellyfin, wait-for-jellyfin (init)
Traceback (most recent call last):
  File "/mnt/init-jellyfin.py", line 21, in <module>
    def post(url: str, headers: dict, body: dict | None): # -> str | dict
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'

Hint: We should make the job fail if an API returns an erronous status cose. What do you think?

@imgios
Copy link
Collaborator

imgios commented Apr 17, 2024

Thanks for testing it again! I'll dig into it and see what's happening.

Hint: We should make the job fail if an API returns an erronous status cose. What do you think?

The idea is to make it fail based on the response status code (e.g., if it's not equal to 200/201 based on the API definition), that's why I still have few TO-DOs for each service init script, e.g.:

# TO-DO: Check for response status code and decide what to do

@imgios
Copy link
Collaborator

imgios commented Apr 22, 2024

While performing the refactoring I found out that we have an issue with the qBitTorrent integration in Prowlarr.

This is the container performing the action:

- name: prowlarr-vs-qbt-finalizer
image: "rapidfort/curl:latest"
command:
- "/bin/sh"
- "-ec"
args:
- "curl -k -X POST \"http://servarr-prowlarr.{{ .Release.Namespace }}.svc.cluster.local:9696/api/v1/downloadclient\" -H 'content-type: application/json' -H 'x-api-key: $(APIKEY)' -H 'x-requested-with: XMLHttpRequest' -d '{\"enable\":true,\"protocol\":\"torrent\",\"priority\":1,\"removeCompletedDownloads\":true,\"removeFailedDownloads\":true,\"name\":\"qBittorrent\",\"fields\":[{\"name\":\"host\",\"value\":\"servarr-qbittorrent\"},{\"name\":\"port\",\"value\":\"10095\"},{\"name\":\"useSsl\",\"value\":false},{\"name\":\"urlBase\"},{\"name\":\"username\",\"value\":\"$(TORRENT_ADMIN)\"},{\"name\":\"password\",\"value\":\"$(TORRENT_PASSWORD)\"},{\"name\":\"movieCategory\",\"value\":\"radarr\"},{\"name\":\"movieImportedCategory\"},{\"name\":\"recentMoviePriority\",\"value\":0},{\"name\":\"olderMoviePriority\",\"value\":0},{\"name\":\"initialState\",\"value\":0},{\"name\":\"sequentialOrder\",\"value\":false},{\"name\":\"firstAndLast\",\"value\":false},{\"name\":\"contentLayout\",\"value\":0}],\"implementationName\":\"qBittorrent\",\"implementation\":\"QBittorrent\",\"configContract\":\"QBittorrentSettings\",\"infoLink\":\"https://wiki.servarr.com/radarr/supported#qbittorrent\",\"tags\":[]}'"
env:
- name: APIKEY
value: "{{ .Values.global.apikey }}"
- name: TORRENT_ADMIN
value: "{{ .Values.torrent.username }}"
- name: TORRENT_PASSWORD
value: "{{ .Values.torrent.password }}"

This is the API answer:

~$ kubectl logs -n servarr downloaders-post-install-job-mpj59 -c prowlarr-vs-qbt-finalizer                                                                                                                         % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
[
  {
    "isWarning": false,
    "propertyName": "",
    "errorMessage": "Test was aborted due to an error: Object reference not set to an instance of an object.",
    "severity": "error"
  }
100  1033    0   194  100   839    539   2333 --:--:-- --:--:-- --:--:--  2869
]

And those are the pod logs:

[Error] QBittorrent: Test aborted due to exception

[v1.14.0.4286] System.NullReferenceException: Object reference not set to an instance of an object.
   at NzbDrone.Core.Download.DownloadClientBase`1.ValidateCategories(List`1 failures) in ./Prowlarr.Core/Download/DownloadClientBase.cs:line 97
   at NzbDrone.Core.Download.DownloadClientBase`1.Test() in ./Prowlarr.Core/Download/DownloadClientBase.cs:line 112


[Warn] ProwlarrErrorPipeline: Invalid request Validation failed:
 -- : Test was aborted due to an error: Object reference not set to an instance of an object.

@fonzdm
Copy link
Owner Author

fonzdm commented Apr 23, 2024

Hi @imgios, issue has been addressed in #22 and resolved in #23.

@fonzdm fonzdm removed the needs triage This issue may need triage. Remove it if it has been sufficiently triaged label Apr 23, 2024
@imgios imgios mentioned this issue Apr 23, 2024
2 tasks
@imgios imgios linked a pull request Apr 23, 2024 that will close this issue
2 tasks
@fonzdm
Copy link
Owner Author

fonzdm commented Apr 24, 2024

Closed by #24

@fonzdm fonzdm closed this as completed Apr 24, 2024
@imgios imgios added this to the first release milestone May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants