Project

General

Profile

Actions

Bug #6304

closed

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

Added by Tom Clegg almost 9 years ago. Updated about 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
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 3 (0 open3 closed)

Task #6246: Add test case for dns configsClosed05/23/2015Actions
Task #6245: Improve race and exception handling in dns updatesClosed05/23/2015Actions
Task #11382: Review 6304-dns-update-fixResolvedLucas Di Pentima05/23/2015Actions
Actions #1

Updated by Tom Clegg almost 9 years ago

  • Description updated (diff)
Actions #2

Updated by Tom Clegg about 7 years ago

  • Description updated (diff)
Actions #3

Updated by Tom Morris about 7 years ago

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

Updated by Lucas Di Pentima about 7 years ago

  • Assigned To set to Lucas Di Pentima
Actions #5

Updated by Lucas Di Pentima about 7 years ago

  • Status changed from New to In Progress
Actions #6

Updated by Lucas Di Pentima about 7 years ago

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

Actions #7

Updated by Tom Clegg about 7 years 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".

Actions #8

Updated by Tom Clegg about 7 years ago

  • Description updated (diff)
Actions #9

Updated by Lucas Di Pentima about 7 years 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.

Actions #10

Updated by Lucas Di Pentima about 7 years 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.

Actions #11

Updated by Tom Clegg about 7 years 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!

Actions #12

Updated by Lucas Di Pentima about 7 years ago

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

Applied in changeset arvados|commit:be2a34c57eab7cc184c08ec1059554cb97911276.

Actions

Also available in: Atom PDF