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 8 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
05/23/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

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 8 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 9 months ago

  • Description updated (diff)

#3 Updated by Tom Morris 9 months ago

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

#4 Updated by Lucas Di Pentima 9 months ago

  • Assigned To set to Lucas Di Pentima

#5 Updated by Lucas Di Pentima 9 months ago

  • Status changed from New to In Progress

#6 Updated by Lucas Di Pentima 8 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 8 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 8 months ago

  • Description updated (diff)

#9 Updated by Lucas Di Pentima 8 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 8 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 8 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 8 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