Page MenuHomePhabricator

[Nix] Add script to start/stop tunnelbroker services
ClosedPublic

Authored by jon on Nov 29 2022, 10:48 AM.
Tags
None
Referenced Files
F3516272: D5758.id19321.diff
Sun, Dec 22, 1:13 PM
Unknown Object (File)
Wed, Dec 18, 2:07 AM
Unknown Object (File)
Nov 22 2024, 5:33 AM
Unknown Object (File)
Nov 22 2024, 5:33 AM
Unknown Object (File)
Nov 22 2024, 5:33 AM
Unknown Object (File)
Nov 22 2024, 5:33 AM
Unknown Object (File)
Nov 22 2024, 5:33 AM
Unknown Object (File)
Nov 22 2024, 5:33 AM
Subscribers

Details

Summary

Tunnelbroker development services have significant
resources costs compared to mariadb or redis. Thus the starting
and stopping of such services shouldn't be done unilaterally with
every 'nix develop'.

First introduce paradigm with rabbitmq.

https://linear.app/comm/issue/ENG-1696

Test Plan

On macOS (intel or m1)

nix develop

comm-dev services stop # ensure rabbitmq is stopped

pgrep beam.smp # should be empty

comm-dev services start # should emit some rabbitmq info
comm-dev services start # should be no-op

pgrep beam.smp # should show a PID

comm-dev services stop # optional, cleanup rabbitmq

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Looks ok to me, but have a few comments:

  • When we are starting by comm-dev services start we have some logs output like killing RabbitMQ:
View RabbitMQ logs: tail -f /Users/max/.local/share/RabbitMQ/logs/comm.log
Kill RabbitMQ server: pkill rabbitmq-server beam.smp
  • When we are stopping by comm-dev services start: there is silence.

Maybe we can just run the comm-dev services stop command when we are starting by the comm-dev services start?
In this case, the output will be consistent and we are avoiding duplication. I assume that we are stopping service first before we are starting it, so maybe we can just call comm-dev services stop.

atul requested changes to this revision.Nov 30 2022, 3:18 PM

Can we update the Test Plan to include what operating systems and architectures the diff was tested on

This revision now requires changes to proceed.Nov 30 2022, 3:18 PM
jon requested review of this revision.EditedNov 30 2022, 6:51 PM

Can we update the Test Plan to include what operating systems and architectures the diff was tested on

Intel mac, my m1 is still doing the "clean run" stuff. However, it should work on m1 unless the build is broken.

When we are starting by comm-dev services start we have some logs output like killing RabbitMQ:

This is by design, if you just want to kill rabbitmq, then it's still appropriate. Eventually localstack will be added in a similar fashion under comm-dev services start.

I assume that we are stopping service first before we are starting it, so maybe we can just call comm-dev services stop.

No. I just check to see if there's a process already running, start it if it's not.

When we are stopping by `comm-dev services start`: there is silence.

That's because the command to stop is comm-dev services stop. I can also emit this if needed.

I assume that we are stopping service first before we are starting it, so maybe we can just call comm-dev services stop.

No. I just check to see if there's a process already running, start it if it's not.

Maybe we can unify this in a way that comm-dev services stop stopping or/and killing services and we just call comm-dev services stop from the inside of comm-dev services start and move all of the "stopping" logic from the beginning of the comm-dev services start to comm-dev services stop?

Intel mac, my m1 is still doing the "clean run" stuff. However, it should work on m1 unless the build is broken.

Can you amend the Test Plan to include testing both Intel and M1?

Maybe we can unify this in a way that comm-dev services stop stopping or/and killing services and we just call comm-dev services stop from the inside of comm-dev services start and move all of the "stopping" logic from the beginning of the comm-dev services start to comm-dev services stop?

How about I add a restart command which does that. I think start should be limited to starting services, and stop should be limited to stoppping services.

Can you amend the Test Plan to include testing both Intel and M1?

Should be the same, but I'll assert that it also works on m1

In D5758#172780, @jon wrote:

Maybe we can unify this in a way that comm-dev services stop stopping or/and killing services and we just call comm-dev services stop from the inside of comm-dev services start and move all of the "stopping" logic from the beginning of the comm-dev services start to comm-dev services stop?

How about I add a restart command which does that. I think start should be limited to starting services, and stop should be limited to stoppping services.

I think this is a good idea!

ashoat requested changes to this revision.Dec 8 2022, 10:46 AM

Can you amend the Test Plan to include testing both Intel and M1?

This revision now requires changes to proceed.Dec 8 2022, 10:46 AM

rabbitmq has an unfree license, so you may need to build the package. But this command should run on m1 or intel

@jon Test Plan still hasn't been amended... is that intentional? I think both @atul and I would love to see you test this diff on Intel macOS and Apple Silicon macOS

Should be the same, was able to get the "clean" mac working after some refinement.

Looks like I was trying to write to the pid file before the entry point had time to make the directory. Locally, it worked for me because I had created the directory when iterating on the task.

Ensure pidfile directory exists before writing pidfile

ashoat requested changes to this revision.Dec 13 2022, 4:15 PM

Requesting changes for Atul's comment earlier:

Can we update the Test Plan to include what operating systems and architectures the diff was tested on

Sounds like it was tested on Intel Mac and maybe Linux, but not clear

This revision now requires changes to proceed.Dec 13 2022, 4:15 PM
ashoat added 1 blocking reviewer(s): max.
This revision now requires review to proceed.Dec 16 2022, 7:06 AM

Accepting, but have a few thoughts:

I think using the same names, but with and without .sh for the comm-dev file makes a mess:

bin/
  comm-dev
comm-dev.sh
  • Maybe we can use some different and more reflecting names for one of these files?
  • The comm-dev file is actually a script, maybe we should use a .sh suffix as well?
This revision is now accepted and ready to land.Dec 18 2022, 9:10 PM

I think using the same names, but with and without .sh for the comm-dev file makes a mess

This is a compromise for getting the shellchecks, but having the correct command name in the shell (e.g. comm-dev services start instead of comm-dev.sh services start).

Created https://linear.app/comm/issue/ENG-2494 to address this

In D5758#178355, @jon wrote:

I think using the same names, but with and without .sh for the comm-dev file makes a mess

This is a compromise for getting the shellchecks, but having the correct command name in the shell (e.g. comm-dev services start instead of comm-dev.sh services start).

Created https://linear.app/comm/issue/ENG-2484 to address this

Makes sense to leave it now as is, thanks for the clarification @jon!