docs: in K8s tutorial, explain that version check might need a retry loop - #2689
docs: in K8s tutorial, explain that version check might need a retry loop#2689dwilding wants to merge 3 commits into
Conversation
| The workload version is available after the workload starts, which happens after Pebble starts the `fastapi` service. We'll use the `src/fastapi_demo.py` helper module for this step. | ||
|
|
||
| In `src/charm.py`, add the following lines to the `_on_demo_server_pebble_ready` function before the final `self.unit.status = ops.ActiveStatus()`: | ||
| In `src/charm.py`, add the following lines to the `_on_demo_server_pebble_ready` function after the `container.replan()` line: |
There was a problem hiding this comment.
I'm changing this part so that talking about replan() later makes more sense.
|
|
||
| We get the workload version over port 8000 because the `fastapi` service runs the app on this port. Then `self.unit.set_workload_version` exposes the workload version to Juju. If the `get_version` call fails (for example, an `URLError` exception is raised), the charm will go into error status. The Juju logs will show the error message, to help you debug the error. | ||
|
|
||
| For a general workload, the `get_version` call could fail because the workload's service hasn't fully started. `container.replan()` tells Pebble to start the service, but doesn't wait to confirm that the service has finished starting up. Pebble actually waits one second to check that the service didn't crash on startup, which in our case is enough time for the `fastapi` service to fully start. |
There was a problem hiding this comment.
I'm not sure whether "general workload" or "production workload" would be better here.
There was a problem hiding this comment.
I think For a general workload, the get_version ... might give the impression that any workload would have the same get_version function. For me, get_version is specific to our tutorial and api_demo_server application. production workload seems to be better here, and consistent with the code comment For a production workload ... above.
Thinking more about it. I wonder if the difference is more about the environment where the application is deployed (in production, fastapi can take longer to start), or more about the type of workload (api_demo_server or another application). I assume it is the first, how about:
# In production, wrap get_version() in a retry loop.
And
In production, the `get_version` call could fail ...
There was a problem hiding this comment.
I was thinking the distinction is more about the type of workload. But my "in production" wording is muddling that, isn't it. I think our tutorial code should be production-ready - it just happens that our workload has a small footprint.
Maybe the best answer is to actually just include the retry loop in the charm code. Or build it into the get_version() function in the first place. What do you you think?
There was a problem hiding this comment.
Thanks for the clarification. I understood it wrong, sorry.
My impression from the internal discussion: we aimed for production ready code in the tutorial, but we wanted to keep the tutorial code simple in this specific case. I still think adding a comment is a sensible option.
it just happens that our workload has a small footprint.
In that case, For a general workload would work well. In our tutorial, this is not a major risk, but Charmers should be careful when working with their own workload. Not sure if this is okay: can we give an example in the tutorial, without having it in the example code ?
What I have in mind:
For a general workload, the
get_versioncall could fail because the workload's service hasn't fully started.container.replan()tells Pebble to start the service, but doesn't wait to confirm that the service has finished starting up. Pebble actually waits one second to check that the service didn't crash on startup, which in our case is enough time for thefastapiservice to fully start.
However, have a retry loop when working with another workload, since it can take longer to fully start:
Example: a small snippet of retry code incharm.py
There was a problem hiding this comment.
Thanks for your help with this. I just made some changes based on your suggestions:
- Changed "For a production workload" to "For a general workload" in the comment.
- Added a generic example of a retry loop after replan, inspired by how the Charmcraft profile does it.
See the preview doc. How does it look now?
|
Moving this back into draft status. An integration test run hit the race condition, so we ought to update the charm code. I'll propose an update. |
…2696) The integration tests for our COS-enabled K8s charm (k8s-5-observe) keep failing in CI. For example, https://github.com/canonical/operator/actions/runs/32157684730. The root cause is an IP range issue with microk8s. See canonical/concierge#251. This PR switches to Canonical K8s, to match our K8s tutorial. I'm using the same custom `k8s` Concierge preset as elsewhere in our CI. Our custom preset allows 30 mins for bootstrap to complete. In addition, I'm setting a 90 min timeout for the whole Concierge job. During testing yesterday, I observed several runs that spun indefinitely - presumably a transient issue with the runner, as the problem didn't appear today. The problem didn't seem to be related to the Concierge preset, so I've set the same timeout for the machine charm job. **[Passing run in my fork](https://github.com/dwilding/operator/actions/runs/32230089789)**. The k8s-4-action failure is unrelated; it's related to #2689.
2 participants
This part of the K8s tutorial charm has a potential race:
In practice we're OK because our workload starts serving with Pebble's 1 second early-exit check. But for a production workload, it would be better to wrap
get_version()in a retry loop.We discussed this internally and decided not to make the tutorial code more complicated by adding a retry loop. Instead, this PR:
get_version()call.Preview doc