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

Adding a persistent installer and launch agent #29

Merged
merged 2 commits into from
Feb 11, 2017

Conversation

knorby
Copy link
Contributor

@knorby knorby commented Jan 30, 2017

Hey! We've been using d4m-nfs at IFTTT, and I wanted to pass back this installer and launch agent. The installer and runner are mostly cobbled together from the existing script, but as it no longer uses /tmp, I removed a few elements. The mount seems to come back after Docker/machine restarts quite reliably, and we've had a few testers internally.

I haven't updated docs in the PR, but the instructions for use are

  • Run ./install.sh and follow prompts
  • Add '/opt/d4m-nfs' to the list of File Shares on Docker, and hit apply and restart.

Please let me know if I've blown away something important. Thanks for this project.

@chytreg
Copy link

chytreg commented Feb 7, 2017

@knorby I like very much you PR, will try it right away.
@knorby or @if-kenn Have you thought about creating brew package?

brew install d4m-nfs
brew services # as formula service on login or boot

@knorby
Copy link
Contributor Author

knorby commented Feb 7, 2017

I don't really see this as a great candidate for homebrew as is, though I suppose it could be made into one. I think the installer method here is an improvement on just running the script in the repo directory each time docker gets restarted, but it isn't that clean of an install. It would need new support for NFS mount configs (at IFTTT, we have engineers use a fork of the d4m-nfs repo that includes a NFS config for our environment), multi-user handling (right now /opt/d4m-nfs is owned by the user who runs the installer), and perhaps some improved flow to add /opt/d4m-nfs to Docker osxfs shares.

@if-kenn
Copy link
Collaborator

if-kenn commented Feb 9, 2017

@knorby I was finally able to look into this PR. There are some obvious good additions in it, thanks for your contribution.

I do however have a few concerns; moving from event based waits to estimated time waits, backwards compatibility, code reuse for maintainability, honoring users systems/not forcing out of repo directories, keeping choice of install versus run on demand.

So with that in mind, I was thinking of refactoring some of the features to a function library, implementing the use of a d4m-xfer directory if it is shared instead of tmp, and a few others.

I don't want to steal your thunder, so after that bit of refactoring, either I could implement the launch agent or I pause and you use the refactored code to submit a PR with the launch agent. What would be your preference?

@knorby
Copy link
Contributor Author

knorby commented Feb 9, 2017

@if-kenn I mostly care that it is easy to install and works well, so if you have a plan, please just move ahead and don't wait for me, but I'm happy to help. This was far more of a quick hack on my part than something thoroughly planned, and moving to a single function library, handling around available directories, etc... all sound like good ideas. It would be nice to make this installable through homebrew, as suggested, and that sounds like a route, among other reasons.

I share your dislike of the time-based setup, but I did it partly due to lack of events to key off of. The VM's tty file gets updated sometime while Docker for Mac shows it as "pending," and I didn't find any simple ways of introspecting the state of the VM to determine when it is usable (or when Docker makes this determination). The initial sleep, the retry loop around setting up screen, and the retry on the VM setup command (it always gets sent, but sometimes before a prompt is available) all got added due to separate issues from running in this startup phase. I'm sure the timing could be optimized, but I'm skeptical you'll have much luck finding events to key off aside from the initial VM startup. I poked around in Docker and the VM for a bit, and I came to the conclusion even if I could find it, it would be too internal and unstable to trust. Based on what I saw, even if you find something, I'd suggest focusing more on replacing the initial sleep than the retries.

I'd personally prefer that you go with the route that gets a launch agent into this repo sooner rather than later, so we can have it available in the IFTTT fork. I'm not sure if you are thinking of merging and then refactoring or just doing a fresh implementation. I'd prefer if you merged and refactored, and I'd be happy to implement the refactor if you want, but it is up to you.

@if-kenn if-kenn merged commit 18e769f into mineshaftgap:master Feb 11, 2017
@if-kenn
Copy link
Collaborator

if-kenn commented Feb 11, 2017

@knorby I have merged the request. I will look to refactor both together, once that is done. I would love if you could provide some copy for the README about usage.

@if-kenn
Copy link
Collaborator

if-kenn commented Feb 12, 2017

@knorby I have a few questions/issues in order to try and refactor your code:

  • Why do you have to copy d4m-nfs-start.sh in install.sh, can it not be used from it's original location in the repo?
  • I am added a log directory in the repo, can I have the %StandardOutPath%/launchd.log be set to there?
  • I eventually am planning on adding a check for d4m-xfer directory that is used for all files between the VM and host and if it doesn't find it, it will fall back to /tmp.
  • I would like to move my d4m-nfs.sh and your install.sh into the bin directory.
  • I will be breaking out HEREDOCs (com.ifsight.d4m-nfs.plist & d4m-mount-nfs.sh) to separate template files so that it is easier to maintain.

@if-kenn
Copy link
Collaborator

if-kenn commented Feb 12, 2017

I think I have come up with a way to eliminate the use of the transfer directory, will let you know more.

@knorby
Copy link
Contributor Author

knorby commented Feb 12, 2017

@if-kenn Sure no concern from me on most directory locations. I had install.sh place d4m-nfs-start.sh in the shared directory mostly to follow the notion of installation, and since the launch agent and the VM script are both in directory sensitive locations, I didn't see a strong reason to preserve the connection to the repo directory. I found the log useful, but I think it could be removed or moved with no issue. The rest sounds great.

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