Health check on container instances by cptanalatriste · Pull Request #2570 · alan-turing-institute/data-safe-haven

cptanalatriste · 2026-02-26T16:51:38Z

✅ Checklist

You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

N/A

⤴️ Summary

We have added a new CLI command: dsh sre healthcheck (see sre.py).
We are also extending the AzureContainerInstane type, so it can now return all the containers in the container group (see azure_container_instance.py). We'll use this method in assertions (see healthcheck_utils.py).
The health check logic is built using pytest (see sre_healtchcheck_runner.py). To that end, we have developed a plugin to provide tests with information of the stack, the subscription, and the SRE config file (see healthcheck_plugin.py).
This first round of tests just check that every SRE container is in running state (see test_container_instance.py and pyproject.toml). We need to add some logic to handle Nexus containers, since it's not deployed in T0/T1.
We need to modify the Pulumi code of all the container instance resources, to expose the container group name and resource group (see infrastructure/programs/sre/*.py). We use this information to gather container group information.

🌂 Related issues

Contributes towards #2568

🔬 Tests

Tested on a Tier 2 and a Tier 0 SREs (below):

github-actions · 2026-02-27T15:10:22Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
data_safe_haven/commands
sre.py					280-316
data_safe_haven/external/interface
azure_container_instance.py					92-93, 98-107
data_safe_haven/healthcheck
__init__.py
check_container_instance.py					14-50, 58-70
healthcheck_plugin.py					17, 21, 25
healthcheck_utils.py					27-48, 56-66
sre_healthcheck_runner.py					28-52
data_safe_haven/infrastructure/programs
declarative_sre.py					525-532
data_safe_haven/infrastructure/programs/sre
apt_proxy_server.py					206-211
clamav_mirror.py					180-185
gitea_server.py					421-426
hedgedoc_server.py					351-356
identity.py					255-260
remote_desktop.py					424
data_safe_haven/provisioning
sre_provisioning_manager.py					51-63
Project Total

_{This report was generated by python-coverage-comment-action}

JimMadge

Oh, interesting 🤔.

Where were you seeing this going in the medium/long term?

Some things feel a bit odd,

using pytest for these checks
A CLI for this rather than a dashboard

Those things feel a bit more reasonable when we think about scaling this to other tests though. That said, we are probably only using 1% of pytest and it might be simpler to write test class of our own.

When talking about containers, it would be good to add a health check too, on top of checking the containers are running. I'm not sure if all of the containers we use have an appropriate HEALTHCHECK defined, but we could add them. Looks like in ACI this is called liveness probe.

cptanalatriste · 2026-03-04T10:33:38Z

Hi @JimMadge , thanks for your comments. Regarding your questions:

In the short term, I see the healthcheck feature as quick holistic quality check on deployed SRE. For example, the tests contained in this PR would detect if an automatic PR broke a container (like Updating Nexus to v3.88.0 breaks the container #2557 ). Or if we're facing an already known container-related issue, like OAuth client secret starting with dash breaks identity container #2502. I envision this tool will be really helpful for the admin teams working with production TREs (maybe @helen-d-little and @llewelld can comment).
This PR is meant to be just a basic scaffolding setup, and further tests/checks are coming. On the medium term, I would like to port most of the tests in the security checklist to the healtcheck CLI. That would also speed-up DSH releases.
On the long term, as discussed before, the logic built for this feature can serve as basis for a CI/CD pipeline that deploys automatically to a Development TRE. It can even, for example, run periodically and populate a dashboard.
I also felt weird using pytest outside a testing context, but it had so many convenient features that made working on this PR very easy. To mention a few: 1) Using plugins to deliver testing dependencies, 2) Parametrised tests, 3) Configurable output and reporting. I'll argue building those features is not trivial, and I don't see a strong argument for not using a mature library that already offers them.
It would definitely be great to implement liveness probes to our container instances! We would need to check into each image documentation to see what's already on offer, or maybe setup a custom one ourselves. However, at the moment the container state gaves me all the signal I need for the issues I'm aware of (see item 1.). Happy to add this to backlog, and work on it later.

Hopefully this makes sense. Please let me know if you have further questions.

JimMadge · 2026-03-09T11:21:27Z

I think the longer term plan makes sense, but we need to do more now to plan what that will look like.

Container status/healthchecks feels like a sensible place to start. Running pulumi refresh in check mode also feels like an obvious starting point.

I feel pytest is the wrong library to use here. It is really focused on code testing, which isn't what we are doing. Parametrising test functions and formatting the output shouldn't be very difficult, and I think going with pytest we are likely to run in to problems in the future with the way it handles exceptions and reports errors.

I think what we really want is a function with returns a test result (or even just a boolean) and then format sensible output from that.

cptanalatriste · 2026-03-09T12:01:58Z

@JimMadge , I'm still not entirely clear on the concrete technical problems we would encounter in a hypothetical future by using pytest (or if they're unavoidable), and I feel what you're suggesting we'll require re-writing/re-implementing mature features already available and battle-tested.

However, this feature is not been progressed for two weeks and I'm feeling some urgency for getting closer to the medium/long term plan. Also, as you mentioned, the pytest features I'm using at the moment are not that complicated. So, I'll prepare a pytest-free version of this PR shortly. I'll let you know when it's ready.

cptanalatriste · 2026-03-10T12:34:44Z

@JimMadge here's a pytest-free version of this PR. Still working as expected:

JimMadge

I think this is looking good. I'm finding the structure quite complex and maybe over engineered though.

Couldn't we just have a HealthCheck ABC, and sublcass it as we need, maybe like ContainerHealthCheck(HeathCheck) PackageContainerHealthCheck(ContainerHealthCheck). Which is, more or less what is here, but I don't like the implicit way typing.Protocol works.

We could also make factories, but I think classes make sense because we are likely to want helper methods as well as the run method.

For executing, I have used decorators before to add functions to a list. So you could have something like,

health_check_instances = []

class MyCheck(HealthCheck):
    def run(self, ...):
        ...

@health_check_instance
MyCheck(case1)

@health_check_instance
MyCheck(case2)


for instances in health_check_instances:
    result = instance()
    handle_result(result)

How does that sound to you?

JimMadge · 2026-03-23T09:26:10Z

+            container_group_name=self.container_group_name,
+        )
+
+        return typing.cast(list[Container], container_group.containers)


Is this because container_group.containers isn't actually a list[Container]?

JimMadge · 2026-03-23T09:26:50Z

@@ -0,0 +1,70 @@
+from attrs import define


Any reason to use this over dataclasses.dataclass?
attrs isn't an explicit dependency, and we are already using dataclasses elsewhere.

JimMadge · 2026-03-23T09:42:39Z

-        self.software_repository_params: dict[str, str] | None = sre_stack.output(
-            "software_repositories"
-        )
+        self.software_repository_params: dict[str, str] | None = None
+
+        try:
+            self.software_repository_params = sre_stack.output("software_repositories")
+        except KeyError:
+            self.logger.info(
+                "There's no software repositories information in the current SRE"
+            )


Is this a bug fix, can we put it in a different PR?
We shouldn't use exception handling for flow control if we can avoid it. Hopefully we can do a if "software_repositories" in ... here.

JimMadge · 2026-03-23T09:45:44Z

+@define
+class SREHeathCheckPlugin:
+
+    _sre_project_manager: SREProjectManager
+    _subscription_name: AzureSubscriptionName
+    _sre_config: SREConfig
+
+    @property
+    def project_manager(self) -> SREProjectManager:
+        return self._sre_project_manager
+
+    @property
+    def subscription_name(self) -> AzureSubscriptionName:
+        return self._subscription_name
+
+    @property
+    def sre_config(self) -> SREConfig:
+        return self._sre_config


What is this class for? It is just aliasing three variables.

JimMadge · 2026-03-23T09:48:59Z

+class HealthCheckTest(Protocol):
+
+    def check(self, plugin: SREHeathCheckPlugin) -> str: ...


I don't like the implicit way classes are assigned. I think it would be better to make an ABC for health checks

JimMadge · 2026-03-23T09:49:27Z

+class HealthCheckError(Exception):
+    pass


We collect our exceptions in the exceptions module.

JimMadge · 2026-03-23T09:51:05Z

+
+
+@define
+class SREHealthCheckRunner:


Is there a benefit to this being a class? Feels like it is just a function.

JimMadge · 2026-03-23T09:52:23Z

+                self._logger.info(f"\u2705 {success_message}")
+            except HealthCheckError as error:
+                self._logger.info(f"\u274c {error.args[0]}")


Rich has support for emoji codes that are more human readable.

cptanalatriste added 5 commits February 26, 2026 16:49

Initial and successful POC

b69ae65

Checking for container state

d065709

Refactor and reorganisation

fbbca4c

Adding extra containers to test

ab8a595

Merge branch 'develop' into 2568-sre-healthchecks

b947402

cptanalatriste added 12 commits February 27, 2026 15:14

Fixing linting errors

f642bd9

Enabling assertions in healthchecks

3f5cd32

Temporarily disable lint alerts

c6c87c2

Fixing linting error

edfea55

Adding pytest as CLI dependency

afe971b

Checking the state of the clamav-mirror container

3788f31

Fixing lint errors

b5f4b7f

Checking the state of the Gitea container

700bfee

All containers are tested now!

913ec59

Fixing linting errors

3b99153

Fixing more linting errors

3b7fa69

Fixing error on T0/T1 deployments

fcdc208

cptanalatriste marked this pull request as ready for review March 3, 2026 13:36

cptanalatriste requested a review from a team as a code owner March 3, 2026 13:36

JimMadge reviewed Mar 3, 2026

View reviewed changes

cptanalatriste added 6 commits March 9, 2026 15:45

[WIP] Removing Pytest dependency

6957454

Further removing the Pytest dependency

16143c8

More Pytest dependency removal

a4b8fa6

Fixing linting problems

2b0d994

Removing pending assertions

e7ff44c

Minor Renaming

da99a58

cptanalatriste added 3 commits March 10, 2026 12:21

More renamings!

6a3cbbf

No need for a verbose option

7a70214

Fixing linting error

80339fd

JimMadge requested changes Mar 23, 2026

View reviewed changes

		class HealthCheckTest(Protocol):

		def check(self, plugin: SREHeathCheckPlugin) -> str: ...

Uh oh!

Conversation

cptanalatriste commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Checklist

🚦 Depends on

⤴️ Summary

🌂 Related issues

🔬 Tests

Uh oh!

github-actions Bot commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

JimMadge left a comment

Choose a reason for hiding this comment

Uh oh!

cptanalatriste commented Mar 4, 2026

Uh oh!

JimMadge commented Mar 9, 2026

Uh oh!

cptanalatriste commented Mar 9, 2026

Uh oh!

cptanalatriste commented Mar 10, 2026

Uh oh!

JimMadge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cptanalatriste commented Feb 26, 2026 •

edited

Loading

github-actions Bot commented Feb 27, 2026 •

edited

Loading