Skip to content

Add start time to .last_run_stats #288

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.

Sign up for GitHub

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

Open
urosgruber wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add start time to .last_run_stats #288

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion share/pot/start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,8 +699,9 @@ _js_start()

wait "$_wait_pid"
_exit_code=$?
_epoch=$(date +%s)

echo "{ \"ExitCode\": $_exit_code }" > "$_confdir/.last_run_stats"
echo "{ \"ExitCode\": $_exit_code, \"StartedAt\": $_epoch }" > "$_confdir/.last_run_stats"
Copy link
Copy Markdown
Collaborator

@grembo grembo Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, last-run-stats is only used programmatically by the nomad pot driver, which extracts the exit code of the container from this file in case the container already exited (this is important, as restarting the nomad pot driver means it needs to re-attach and could miss the exit code otherwise). Therefore it contains not more information. In general, it could contain more things (including various timestamps), but in case of non-persistent containers (which is why the feature was implemented in the first place), it will be written when the container exits. That's also why it's called .last_run_stats and not .current_run_stats.

I admit that this is a bit confusing, since in all cases the file contains the exit code of the configured pot.cmd - with persistent jails that's usually (but not always an necessarily) the exit code of running rc (so that's usually written after "booting" the jail), while for non-persistent jails, that's always when the container exited, which makes no sense.

Long story short, writing StartedAt into this file is not universally useful, as it's not always written while the jail is running, but after it started. So for non-persistent jails, it could be interesting to have some statistics in there about for long it ran (which would hard to write for persistent ones, as it's written after rc started).

I hope the above makes sense. So basically, to be more useful, the last_run_stats feature would first need to be refined, so it means the same for persistent and non-persistent jails. When I introduced it, I had mostly non-persistent jails in mind. I didn't realize that for persistent jails the file is written much earlier in their lifetime and therefore has a different meaning.

What you still could do without making things worse:

  1. Rename the variable _epoch to _started_at
  2. Add it to local variables at the top of the _js_start function (this needs to happen with all local variables)
  3. Move _started_at=$(date +%s) to line 654

This way it reflects the real "started at" time of the jail and would be correct in all cases (even though having it for non-persistent jails would be "after the fact" and for persistent jails, it only becomes available after the rc script ran).

Copy link
Copy Markdown
Contributor Author

@urosgruber urosgruber Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, I'll amend as instructed.


if [ "$_persist" = "NO" ]; then
rm -f "${POT_TMP:-/tmp}/pot_main_pid_${_pname}"
Expand Down