Skip to content
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.

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

Improve Nomad Docs #660

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Improve Nomad Docs #660

merged 1 commit into from
Sep 5, 2024

Conversation

mpass99
Copy link
Contributor

@mpass99 mpass99 commented Aug 20, 2024

to describe the behavior of running executions during Nomad restarts.

Related to #651

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.19%. Comparing base (d83407e) to head (f72f611).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #660   +/-   ##
=======================================
  Coverage   76.19%   76.19%           
=======================================
  Files          43       43           
  Lines        3592     3592           
=======================================
  Hits         2737     2737           
  Misses        625      625           
  Partials      230      230           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@MrSerth MrSerth left a comment

Choose a reason for hiding this comment

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

Would you mind to copy the table added in #651 (comment)? This might be helpful and summarizes our findings.

Further, I was wondering about a "consequence" after a problematic restart. Shouldn't be there some note on the runner being clean / used / marked as used (in case an agent is restarted)? And maybe a word on the coupling of the Nomad service and Docker service that we added (to restart one if the other restarts). For servers, I would love to see another sentence about reconnecting, recovering the event stream, ...

By the way, the event stream usage (the why, the benefits, the pitfalls) is not yet documented here either, is it? Just saying that this might be useful, too, especially as a handover.

@mpass99 mpass99 force-pushed the docs/#651-nomad-server-restart branch 2 times, most recently from d5b8fab to 7d528a0 Compare September 3, 2024 11:26
Copy link
Member

@MrSerth MrSerth left a comment

Choose a reason for hiding this comment

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

Thank you, so much! This makes the documentation much more valuable and I you greatly addressed all my previous review points. A few minor comments left, but nothing big.

@MrSerth
Copy link
Member

MrSerth commented Sep 3, 2024

I am continue to feed my issue #669, but I assume those should be fixed soon with your latest PR.

@mpass99 mpass99 force-pushed the docs/#651-nomad-server-restart branch 3 times, most recently from 255e173 to 31134cb Compare September 5, 2024 07:58
@mpass99 mpass99 force-pushed the docs/#651-nomad-server-restart branch from ee7d2c7 to 22f377b Compare September 5, 2024 13:26
to describe the behavior of running executions during Nomad restarts.

Co-authored-by: Sebastian Serth <[email protected]>
@mpass99 mpass99 force-pushed the docs/#651-nomad-server-restart branch from 22f377b to f72f611 Compare September 5, 2024 13:29
Copy link
Member

@MrSerth MrSerth left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to write additional documentation; I really appreciate it! 🌍

@mpass99 mpass99 enabled auto-merge (rebase) September 5, 2024 13:30
@mpass99 mpass99 merged commit a035d62 into main Sep 5, 2024
10 checks passed
@mpass99 mpass99 deleted the docs/#651-nomad-server-restart branch September 5, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants