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.

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

Jump to bottom

ci: 🧟 summonerd smoke test leaves dangling processes when run locally #4370

Closed
cratelyn opened this issue May 9, 2024 · 1 comment · Fixed by #4358
Closed

ci: 🧟 summonerd smoke test leaves dangling processes when run locally #4370

cratelyn opened this issue May 9, 2024 · 1 comment · Fixed by #4358
Assignees
@cratelyn
Labels
A-CI/CD Relates to continuous integration & deployment of Penumbra _P-low Priority: low
Milestone
Sprint 6

Comments

@cratelyn
Copy link
Contributor

cratelyn commented May 9, 2024

noting for future reference: i found when running this script locally that the trap commands herein weren't working as expected; cometbft and pd were sometimes left running, but i haven't tracked down what causes the kill call to fail. 🤨

originally posted by @cratelyn in #4358 (comment)
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra May 9, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label May 9, 2024
@cratelyn cratelyn added A-CI/CD Relates to continuous integration & deployment of Penumbra _P-low Priority: low and removed needs-refinement unclear, incomplete, or stub issue that needs work labels May 9, 2024
@cratelyn cratelyn mentioned this issue May 9, 2024
Merged
1 task
@conorsch
Copy link
Contributor

Ah, only just noticing it. See here: #4358 (comment) tl;dr is we should append to the trap, not clobber it.

cratelyn added a commit that referenced this issue May 10, 2024
@cratelyn
ci: 📞 smoke-summoner.sh signal handling fixes
c92fc0e 
this squashes two related commits from development:

---

ci: ⭐ `SIGINT` will also kill cometbft, pd, summonerd

when running this locally in a development shell, i found that `ctrl+c`
would not terminate the other running processes. this would stop the
shell script, but logs from pd and cometbft would continue being printed
to the screen.

moreover, after stopping that shell, `ps` showed me that these processes
were left running, causing subsequent runs of the summonerd smoke test
to fail due to ports already being bound.

this adds `SIGINT` to the list of signals that will bring down the
cometbft, pd, and summonerd processes. this way, `ctrl+c` will properly
clean things up.

---

ci: ➕ don't clobber smoke test's child pids

fixes #4370.

this makes the list of process id's provided to `kill -9` when a SIGEXIT
or SIGINT signal is received an _additive_ list. prior, we were
clobbering this list when the phase1 and phase2 steps were run.

now, we append a new id to the list at each point we set a signal
trap.
@cratelyn cratelyn self-assigned this May 10, 2024
@cratelyn cratelyn added this to the Sprint 6 milestone May 10, 2024
@cratelyn cratelyn moved this from Backlog to In progress in Penumbra May 10, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cratelyn cratelyn

Labels
A-CI/CD Relates to continuous integration & deployment of Penumbra _P-low Priority: low
Projects
Penumbra
Archived in project
Milestone
Sprint 6
Development

Successfully merging a pull request may close this issue.

ci: 🚭 fix summonerd smoke tests penumbra-zone/penumbra
2 participants
@conorsch @cratelyn