Bug #6304

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

Added by Tom Clegg almost 2 years ago. Updated 16 days 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 estimate-

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 17 days ago

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

History

#1 Updated by Tom Clegg almost 2 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg about 1 month ago

  • Description updated (diff)

#3 Updated by Tom Morris about 1 month ago

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

#4 Updated by Lucas Di Pentima about 1 month ago

  • Assignee set to Lucas Di Pentima

#5 Updated by Lucas Di Pentima 28 days ago

  • Status changed from New to In Progress

#6 Updated by Lucas Di Pentima 23 days 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 22 days 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 22 days ago

  • Description updated (diff)

#9 Updated by Lucas Di Pentima 18 days 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 18 days 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 18 days 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 17 days 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