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

Current Symlink & Restart == Chicken & Egg #68

Open
tomduckering opened this issue May 22, 2013 · 17 comments
Open

Current Symlink & Restart == Chicken & Egg #68

tomduckering opened this issue May 22, 2013 · 17 comments

Comments

@tomduckering
Copy link

Hi,

We have a slight frustration with the fact that the restart occurs before the "current" symlink is in place. We like to refer to the app for it's service control via that symlink.

Obviously there are ways around it but I wonder if you might be able to offer the ability to have the symlink creation somewhere else?

Thanks

@KAllan357
Copy link
Contributor

Hi @tomduckering, thanks for the report. I'd like to make sure I understand what you are describing.

You are encountering a situation where you are writing out a service to somewhere like /etc/init.d/myservice and in that file, it references the "${deploy_to}/current" directory that is eventually created by the artifact_deploy resource. So when the restart proc executes, you have a resource that attempts to start that service, and it cannot because the current symlink has not been created yet. Is that about right?

@tomduckering
Copy link
Author

Hey @KAllan357,

Yeah - some differences in the tools but you've got the gist of it. I'm not so much as raising an issue with the code but wondering if you have any particular thoughts on a good approach to overcome this issue.

In the past - using a similar approach - I've had a means to start a named release of the application. e.g. /etc/init.d/myservice start 1.234 - without a version number specified it uses the current link. In that case the current symlink was a sibling of the version directories to make this trivial to code.

I was interested to know if you had encountered this conundrum too and if you had any advice on dealing with it since your documentation doesn't allude to it.

Cheers,
Tom

@KAllan357
Copy link
Contributor

I was chatting about this issue with @ivey and we both agreed on the same conclusion, that in general, it seemed like the best practice to reference the release_path of the artifact when configuring files inside the deploy_to directory and to reference the current_path of the artifact when configuring files outside.

I think that logic also applies here - we're configuring a file that will exist outside of deploy_to and it would be nice if it could reference and use current_path potentially during the artifact_deploy resource's execution. Unfortunately, I don't think there is an obviously documented way to do this.

I have an idea, and a few questions, so I will start with the questions.

  1. I wasn't aware that you could use Chef to pass parameters to a service. Are you actually able to tell the service resource to start a specific version using Chef or do you have some other way? I don't think I've seen any documentation about something like this:
service "myservice" do
  options "1.234" # this is made up
  action :start
end
  1. Would there be a difference between calling your restart after the artifact_deploy resource. If the restart didn't occur in the restart proc, the only other proc that executes is after_deploy. Could you do something like this?
artifact_deploy "my-artifact" do
  ...
end

service "myservice" do
  action :restart
end

Finally, I have on suggestion about a possible workaround. I allude to it in the documentation, that once you are inside any of the Procs, you have access to any of the internal methods of the Resource class. With that, I think you could do your restart logic in the after_deploy proc, and also save yourself from not always restarting your application when a Chef run happens and doesn't make any changes. Here is an example:

artifact_deploy "my-artifact" do
  ...
  after_deploy Proc.new {
    service "myservice" do
      action :restart
      only_if { current_symlink_changing? }
    end
  }
end

That might get you the idempotency you need. To note, I think at one point we already moved the current symlink creation time logic higher in the artifact_deploy flow, so I'm not adverse to the idea of changing it. Perhaps @ivey can weigh in on that discussion.

Kyle

@JeanMertz
Copy link

@KAllan357, running into the same issue as @tomduckering, although our use case exactly matches your assumption:

  1. init.d script that references /current
  2. restart proc that gets called before the symlink
  3. on initial boot, the app doesn't even start (given that /current isn't even present)
  4. on subsequent boots, an old symlink is used

Using a conditional after_deploy works, but it feels like the restart proc should really be called after symlinking of current occurred.

@mgreensmith
Copy link

Am running into this issue as well, and attempted @KAllan357's workaround (testing current_symlink_changing? in an after_deploy proc). However, this failed for me because comparing the location of the current symlink after the link action has already been completed will always result in equality.

I ended up testing against deploy? and manifest_differences? instead. This doesn't test the same cases as the restart action (which tests all three), but covers most of my use case.

after_deploy Proc.new {
  if deploy? || manifest_differences?
    service "foo" do
      action :restart
    end
  end
end

@KAllan357
Copy link
Contributor

Since it sounds like we already have an init.d type template that is already being managed by Chef, I still think the appropriate action is to reference release_path instead of current_path in the file. Is there a reason that approach doesn't work?

@mgreensmith
Copy link

I'm deploying a ruby app running under unicorn, and sending a USR2 reload to unicorn (via bluepill). Unicorn working directory is current. If I had to change the working directory on every deploy, I would lose the ability to do the inline reload (unicorn USR2 exec's the newly-forked master in the working dir of the original process), and would be forced to explicitly stop and start, causing downtime.

@JeanMertz
Copy link

@KAllan357 using release_path is a possibility, but this means the init.d script controlling the service needs to be updated every time to point to the new release_path. Furthermore, it requires some extra work to handle rollback cases. All in all, at least in my usecase, that would cause more problems than having the restart callback being fired after current_path symlinking.

Of course, there's probably a good reason why it was placed before the symlink in the first case, I just haven't run into that use-case yet.

@JeanMertz
Copy link

@mgreensmith could I ask you how you are handling Ruby/Rails deployments using the artifact cookbook? I would like to use it for Rails apps as well, but we're currently using the application_ruby cookbook, because this cookbook (as the name implies) only supports artifact downloads, whereas we'd rather deploy if a git branch has changes in them.

@KAllan357
Copy link
Contributor

I think I'm convinced. I'm not sure how disruptive it will be to change, but I will target this and some other issues in a 2.0.0 release. I think I will just change the ordering as discussed above.

@mgreensmith
Copy link

@KAllan357 Awesome, thanks. At the cost of increasing complexity, you could consider making the ordering of some actions configurable for multiple use cases.

@JeanMertz We "build" our ruby app in Jenkins (bundle install into a subdirectory, build assets, etc.) and then tar up the entire application directory including the bundled gems. This makes our deploy process simple and fast: use artifact_deploy to download/expand the tarball and trigger a unicorn restart. Deploys happen in <1min.

@tarrall
Copy link

tarrall commented Jul 7, 2014

Any movement on this? I'd love to help test any changes.

For us, moving the if deploy? ... run_proc :restart block to after the "current" symlink creation looks like it should be sufficient.

This will make your artifact_deploy provider match the behavior of Chef's deploy provider which would be a huge win. Doesn't seem like it should break anyone's existing deploys (is anyone depending on the "current" symlink to point to the previous revision at restart?) so hopefully no drama.

@issacg
Copy link

issacg commented Aug 18, 2014

I'm running around in circles on this. I'm using chef 11.14.2 with artifact 1.12.1 and running into this issue.

I've tried using

after_deploy Proc.new {  
    service "nodejs" do
        action :restart
        only_if { current_symlink_changing? }
      end
    }

But get the error:

           NoMethodError
           -------------
           service[nodejs] (/tmp/kitchen/cache/cookbooks/mybook/recipes/deploy.rb line 28) had an error: NoMethodError: undefined method `current_symlink_changing?' for Chef::Resource::Service

I then tried

  after_deploy Proc.new {
    if current_symlink_changing?
      service "nodejs" do
        action :restart
      end
    end
  }

but nothing ever happens.

I tried looking at the same code that seems to trigger the restart proc and did:

  after_deploy Proc.new {
    if current_symlink_changing? || deploy? || manifest_differences?
      service "nodejs" do
        action :restart
      end
    end
  }

that did work, but is triggering the restart more often then it should: I deploy from a tarball, so nothing local should change, but the restart is often triggered even when the tarball path remains the same, which I would like to avoid.

Any ideas what I might be doing wrong?

@fujin
Copy link

fujin commented Sep 5, 2014

Just hit this one trying to use the 'current' symlink in a systemd unit file to a binary included in the deployment artifact. 🎱

@fujin
Copy link

fujin commented Sep 6, 2014

I submitted a change which moves the callback execution timing for both migration and restart to after the current symlink is generated @ #153.

@JeanMertz
Copy link

still in favour of this change. Any update on this recently @KAllan357 ?

@kainosnoema
Copy link

Also in favor as this is preventing us from moving entirely to artifact. @tarrall's suggestion seems simplest. Would rather not fork if we don't have to.

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 a pull request may close this issue.

8 participants