Issue 107: async task processor deployment

[plumdog]

Thanks for submitting a PR! Please check the boxes below:

Fixes #107.

• I have filled in the "Changes" section below?
• I have filled in the "How did you test this code" section below?
• I have bumped the version number in /charts/flagsmith/Chart.yaml in the section version or I'm merging to a release branch

Changes

Add deployment to run the task processor

How did you test this code?

Deployed with the following values:

taskProcessor:
  enabled: true

api:
  extraEnv:
    ENABLE_TASK_PROCESSOR_HEALTH_CHECK: "True"

Then port-forwarded to access the API, and accessed /health?format=json and saw the response included: "TaskProcessorHealthCheckBackend": "working".

Further, when additionally setting

taskProcessor:
  replicacount: 0

Got "TaskProcessorHealthCheckBackend": "unknown error: Task processor is unable to process tasks."

---

Some application-side nice-to-haves:

• I grok what the docs say about the health check not being critical, so 200ing regardless, but if there was a way of getting a 200/500 response for this request, then it would be really easy to add this as a test in the chart. As it stands, this would be a bit awkward and probably brittle. Eg, could /health?check=TaskProcessorHealthCheckBackend&mustSucceed=true return a 200 if that checks passes and a 500 if not?
• elsewhere, make use of https://github.com/Flagsmith/flagsmith/blob/main/api/scripts/run-docker.sh, and this is the entrypoint for the Docker image. If this learnt how to runprocessor and checktaskprocessorthreadhealth (and passed any command line args through, which runprocessor does have), then could remove the references to python manage.py from the templates in this PR. This is probably neater, as it means that run-docker.sh is what define the interface for the Docker image, but in practical terms, in doesn't make much difference.

[plumdog]

If this looks good, probably warrants a PR to https://github.com/Flagsmith/flagsmith-docs.

[matthewelwell]

Added one comment but this looks good to me otherwise.

[matthewelwell]

I grok what the docs say about the health check not being critical, so 200ing regardless, but if there was a way of getting a 200/500 response for this request, then it would be really easy to add this as a test in the chart. As it stands, this would be a bit awkward and probably brittle. Eg, could /health?check=TaskProcessorHealthCheckBackend&mustSucceed=true return a 200 if that checks passes and a 500 if not?

Yep, this makes sense. I will add an issue to handle this.

elsewhere, make use of https://github.com/Flagsmith/flagsmith/blob/main/api/scripts/run-docker.sh, and this is the entrypoint for the Docker image. If this learnt how to runprocessor and checktaskprocessorthreadhealth (and passed any command line args through, which runprocessor does have), then could remove the references to python manage.py from the templates in this PR. This is probably neater, as it means that run-docker.sh is what define the interface for the Docker image, but in practical terms, in doesn't make much difference.

Yeah, this is a good point too. I will add a todo for this as well.

[matthewelwell]

Approved with one minor comment.

[matthewelwell]

I appreciate this is also in other places in this file but I'm not entirely sure I understand what this is waiting on? What is meant be 'tag semver' ?

[plumdog]

There will - presumably - be some application version that means that setting this to true becomes the right thing to do, but before which setting this would break things.

Suppose it was application version 1.2.3 that introduced this change, then would probably want the chart behaviour as follows:

• if runAsNonRoot not set (or set to null) in values, then
  • if image tag >= 1.2.3, do set runAsNonRoot: true for the pods, as this is known to be safe
  • else do not set runAsNonRoot
• if runAsNonRoot set in values to true or false, then set that for the pods, as that is what the user has asked for.

That might be important enough to get ticketed, but maybe not.

[plumdog]

@matthewelwell have rebased and fixed the chart version.