Improve the hub health check in deployer

[sgibson91]

Summary

In CI/CD we are seeing a lot of failed deployments when we test a hub's health. The most common reason is that the tests triggered a node to be spun and up and things timed out before that node was ready to accept the test pods. Can we add some delays to our testing framework that allow the nodes enough time to be spun up and/or do something like the mybinder.org tests where 3 attempts are made before reporting failure?

https://github.com/jupyterhub/mybinder.org-deploy/blob/4e818778a4d6980427c6a519f99508dcfdbbc8ea/.github/workflows/cd.yml#L138-L144

Tasks to complete

• Investigate the major causes of health check failures that should not be failures, identify one or two that we can design tests around
  • Comment here: Temporarily disable singleuser networkpolicy for Pangeo #819 (comment)
• Update our health check infrastructure to be robust to these failures

[damianavila]

Can we add some delays to our testing framework that allow the nodes enough time to be spun up and/or do something like the mybinder.org tests where 3 attempts are made before reporting failure?

I like the idea!!

[yuvipanda]

I opened catalyst-cooperative/pudl-examples#4 to include dask-gateway in the catalyst coop image, so tests can pass for that.

[choldgraf]

@sgibson91 noted a weird error message we should investigate here: #819 (comment)

[sgibson91]

So I think there are 2 causes for node scale-up when we run our hub health checks:

1. The addition of the deployment-service-check pod (which is mimicking a user) triggers a scale up. We can try to make this more efficient by not timing out tests when the pod is in any other state than Ready, trying multiple times, adding delays as described above. But generally, we should eat this cost I think.
2. When we test a daskhub, we also test that a dask cluster can be requested and scaled up. This causes another scale-up of the dask nodepools. Is it really efficient to be testing that feature every time we deploy a hub? Could we not run that kind of test as a cron job in the cluster instead? And run it manually when we know we've triggered a change in the dask helm chart?

[damianavila]

But generally, we should eat this cost I think.

👍

Could we not run that kind of test as a cron job in the cluster instead? And run it manually when we know we've triggered a change in the dask helm chart?

I would be OK with that, I think...
In fact, I would love to have automatic running of dask tests when we change things that are dask-related, but not sure how realistic that request would be...

[choldgraf]

I think @sgibson91's proposals sound good on both points!

[sgibson91]

Linking another issue in here too as I think it's a sub-issue of this one (if we were to turn this into a project, for instance)

• Refactor for clarity that deployer/tests aren't just developer tests #971

[sgibson91]

Another avenue for testing is being discussed here #1024 (comment)

[sgibson91]

For posterity, I also just received this warning from pytest

===================================================================== warnings summary ======================================================================
../../../../../../usr/local/Caskroom/miniconda/base/envs/pilot-hubs/lib/python3.9/site-packages/pytest_asyncio/plugin.py:191
  /usr/local/Caskroom/miniconda/base/envs/pilot-hubs/lib/python3.9/site-packages/pytest_asyncio/plugin.py:191: DeprecationWarning: The 'asyncio_mode' default value will change to 'strict' in future, please explicitly use 'asyncio_mode=strict' or 'asyncio_mode=auto' in pytest configuration file.
    config.issue_config_time_warning(LEGACY_MODE, stacklevel=2)

[sgibson91]

PR #1185 separated the hub health check from the logic that deploys a hub. This separation will allow to implement retry logic on the hub health check without needing to redeploy a hub with every retry.

[sgibson91]

PR #1189 now means we retry the hub health check a maximum of 3 times with a timeout of 10 minutes before we declare a failure in CI

[sgibson91]

PR #1190 separates the dask scaling test from the dask compute test. I suggest we only use the --check-dask-scaling flag locally when we know we are working on dask-gateway-related features

[sgibson91]

I believe we can also close this as well given recent work. @consideRatio would you mind writing up the remaining todos we had from our little sprint as new issues please?

[damianavila]

@consideRatio would you mind writing up the remaining todos we had from our little sprint as new issues please?

Were those remaining todos already captured, @sgibson91 and @consideRatio?

[consideRatio]

I think we are good to move on, but I'm not sure - I didn't recognize the error linked out to the third checkox.

[sgibson91]

Were those remaining todos already captured

@damianavila the issues are #1206 and #1232