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

feat: Watchman integration #188

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

cdmistman
Copy link
Contributor

@cdmistman cdmistman commented May 22, 2024

been putting this together, i forked https://github.com/sjansen/watchman for this lmao

primary motivation is as an analog to the Develop spec in the docker-compose spec.

at work, we use process-compose for our dev workflow, and the only part of my workflow that's missing is filesystem watching for restarting services. while this could be implemented as a shell script, i think it would be great if process-compose directly integrated with watchman specifically for this feature so as not to eat up precious inotify allotments (we do a lot of virtualization).

happy to accept feedback - i know that no issue was opened, nor has there been any discussion of such a feature. it's a little project i gave myself for my own sake as i'd like to use process-compose in a personal project :)

@cdmistman
Copy link
Contributor Author

i know it's not working yet, i also want to clean up the watchman library i forked and publish a proper release. figured i'd open this to see if there was interest in the first place for such a feature

@adrian-gierakowski
Copy link
Contributor

so as not to eat up precious inotify allotments

What do you mean by this? Could you explain how adding this functionality to PC directly, vs wrapping individual programs which need to restart on file changes with tools like https://github.com/watchexec/watchexec, would help with that? Thanks!

@cdmistman
Copy link
Contributor Author

cdmistman commented May 22, 2024

What do you mean by this? Could you explain how adding this functionality to PC directly, vs wrapping individual programs which need to restart on file changes with tools like https://github.com/watchexec/watchexec, would help with that? Thanks!

watchman follows a server-client model. the user's filesystem is split into a collection of "projects" (user-configurable, usually means a directory with one of .git, .svn, .hg, or .watchmanconfig). when a client wishes to listen for changes to that directory, it must

  1. establish an ipc connection to the watchman server
  2. request that the project containing that directory be watched
  3. subscribe to the directory within the project being watched

what this means is that the inotify allotments are only ever held by the watchman server. when the server receives an inotify notification, it filters the paths that are irrelevant to a given subscription and sends any relevant paths in a subscription ipc message. effectively, this is inotify in userspace :)

ETA: git and jujutsu both integrate with watchman as well (i've had both options enabled for a while, and rarely encounter issues)

@cdmistman
Copy link
Contributor Author

after saying all of that, i think the way i set up subscriptions for each process wrong. instead of using a single watchman subscription for all processes, i should use a single subscription per process that needs it. then i don't need to do further filtering in the client (like i do in 6653b14#diff-defe37158035c547155c5ae19d82fc87e70e6ccbe0d61b261c201872f92b2d00R125-R185)

github.com/bytedance/sonic v1.11.6 // indirect
github.com/bytedance/sonic/loader v0.1.1 // indirect
github.com/cdmistman/watchman v0.2.1-0.20240521013409-16b1ae353832 // indirect
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this won't be pinned to a revision by the time i mark this pr as ready. once i finish this integration and clean up the library's api and do some more tests i'll cut a 0.3 release

//Wait should wait for I/O consumption, but if the execution is too fast
//e.g. echo 'hello world' the output will not reach the pipe
//TODO Fix this
// Wait should wait for I/O consumption, but if the execution is too fast
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry i have gofumpt enabled with gopls 😅

}

func (p *Process) setStartTime(startTime time.Time) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i should use this value to reset the watchman subscription clock

@@ -254,6 +263,11 @@ func (p *Process) isRestartable() bool {
p.Lock()
exitCode := p.getExitCode()
p.Unlock()

if p.isDevRestart.Swap(false) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i needed a new signal for when the restart is associated with a watch. i thought about changing isStopped to some stopKind enum ie:

type stopKind u8

const (
  notStopped stopKind = iota
  isStopped
  isDevRestart
)

this is probably still the better solution (especially since p.isStopped.Swap(false) is missing in this if)

}

if notif.IsFreshInstance() {
log.Debug().Msg("fresh instance notification, ignoring")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops forgot continues

Comment on lines +97 to +106
files := []string{}
for _, rawFile := range notif.Files() {
// if the query fields change, this will need to be a map[string]any
file := rawFile.(string)
files = append(files, file)
}

for _, sub := range subs {
sub.updates <- files
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should change this to send to the channel associated with a subscription name. the name should be something like process-compose-dev-{process config name}

config *types.Watch
}

func (s *WatchmanSub) Recv() ([]string, bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should change this to only receive on the channel. data associated with a subscription should be tracked by watchman, not in the layer adapting the client

Comment on lines +21 to +22
Glob string `yaml:"path"`
Ignore []string `yaml:"ignore"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not quite the same as docker-compose's. docker-compose uses just a Path where i'm using a Glob - i was thinking it might be more convenient to do a glob, but a path might make more sense?

Copy link

sonarcloud bot commented May 22, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dzmitry-lahoda
Copy link

Our use case for that is next

    settings.processes.rollman = mkIf cfg.rollman.enable {
      command = pkgs.writeShellApplication {
        name = "rollman";
        runtimeInputs = [ config.rustPackage ];
        text = ''
          set -x
          cargo run -p rollman --bin=nord-dev -- start \
            --data-dir ${cfg.rollman.dataDir} \
            --eth-rpc http://localhost:8545 \
            --contract-addr "$(cat /tmp/deloy-testnet-proxy)" \
            --genesis-block 1
        '';
      };
      namespace = "nord";
      depends_on."deploy-local-testnet".condition = "process_completed_successfully";
      depends_on."rollman-genesis".condition = "process_completed_successfully"; 
    };

So we have processes which start just building rust codebases.
I would be interesting to watch files and restart, so getting new builds.

So questions is how watch would interact with

https://f1bonacc1.github.io/process-compose/launcher/

https://f1bonacc1.github.io/process-compose/health/ (in general, and Auto Restart if not Healthy specifically

For example, when restarting, what is live/ready state during and after? What should dependency processes do (cascading)? Should feat just copy docker-compose behavior for that?

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.

3 participants