Bug #6304

[API] Fix config file race condition and improve test cases for DNS update hooks

Added by Tom Clegg over 2 years ago. Updated 6 months ago.

Status:ResolvedStart date:05/23/2015
Priority:NormalDue date:
Assignee:Lucas Di Pentima% Done:

100%

Category:API
Target version:2017-04-12 sprint
Story points0.5Remaining (hours)0.00 hour
Velocity based estimate0 days

Description

Node.dns_server_update() in source:services/api/app/models/node.rb is currently vulnerable to the following race (fixable by using a tempfile library):
  • Request handler A writes hostname.conf.tmp.
  • Request handler B opens hostname.conf.tmp for writing.
  • Request handler A issues the move command.

Catch only the expected exceptions on the various write and update operations (IOError and SystemCallError?), not unexpected ones.


Subtasks

Task #6246: Add test case for dns configsClosed

Task #6245: Improve race and exception handling in dns updatesClosed

Task #11382: Review 6304-dns-update-fixResolvedLucas Di Pentima

Associated revisions

Revision be2a34c5
Added by Lucas Di Pentima 6 months ago

Merge branch '6304-dns-update-fix'
Closes #6304

History

#1 Updated by Tom Clegg over 2 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg 7 months ago

  • Description updated (diff)

#3 Updated by Tom Morris 7 months ago

  • Target version changed from Arvados Future Sprints to 2017-04-12 sprint

#4 Updated by Lucas Di Pentima 7 months ago

  • Assignee set to Lucas Di Pentima

#5 Updated by Lucas Di Pentima 7 months ago

  • Status changed from New to In Progress

#6 Updated by Lucas Di Pentima 7 months ago

Updates at branch 6304-dns-update-fix - 6da9f3666
Test run: https://ci.curoverse.com/job/developer-run-tests/220/

#7 Updated by Tom Clegg 7 months ago

An ensure block should delete the stray temp file if we got as far as creating one (e.g., if f.puts fails). Something like this?

tmpfile = nil
begin
  ...
  Tempfile.open(...) do |f|
    tmpfile = f.path
    f.puts ...
  end
  File.rename tmpfile, hostfile
  tmpfile = nil
rescue ...
  ...
ensure
  if tmpfile
    File.unlink tmpfile
  end
end

It seems that the new "dns update with dir configured but no command configured" test actually tests "dns_server_update is a no-op when dns_server_conf_template is not set".

#8 Updated by Tom Clegg 7 months ago

  • Description updated (diff)

#9 Updated by Lucas Di Pentima 6 months ago

Updates at 2f46fb576

Ensure dangling temp files are cleaned up.
Pending: test case for this is not working properly. Tom will give it a look.

#10 Updated by Lucas Di Pentima 6 months ago

Updates at f08a738f1565b2dabde28dc9e8d1bad9622d5529
Test run: https://ci.curoverse.com/job/developer-run-tests/225/

As per Tom's suggestion, added the server_dns_template_conf setting so the test can pass, thanks Tom!
Added - char as a separator between the compute node's name and the rest of the temp file, to improve temp file name readability.

#11 Updated by Tom Clegg 6 months ago

nit: prefer "assert_empty foo" to "assert foo.empty?" so the failure message says what unexpected stuff was found instead of just "false is not true".

LGTM, thanks!

#12 Updated by Lucas Di Pentima 6 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:be2a34c57eab7cc184c08ec1059554cb97911276.

Also available in: Atom PDF