Arvados

Add note in application.default.yml that slot_numbers assigned by clients are exempt from max_compute_nodes.

Notes regarding branch 6156-hostnames-in-nodes:

• All stated items in the description are implemented. Unit tests are added.

• Currently, the code supports both these formats: "compute{slot_number}" and "compute%<slot_number>". Or with padding: 'compute#{slot_number.to_s.rjust(4, "0")}' and 'compute%<slot_number>04d'. However, I think it is desirable that we support only one of these formats and keep the code maintenance simple.

Radhika Chippada wrote:

• Currently, the code supports both these formats: "compute{slot_number}" and "compute%<slot_number>". Or with padding: 'compute#{slot_number.to_s.rjust(4, "0")}' and 'compute%<slot_number>04d'. However, I think it is desirable that we support only one of these formats and keep the code maintenance simple.

The only functional requirement is that we use a format that supports arbitrary zero padding. As long as we have that, I have no more stake in which format(s) we support.

Reviewing 451a6d3

On the formatting question: we already have a similar configuration string for Node DNS settings, dns_server_update_command. It uses String#% to interpolate the configuration string. Let's use the same format and method for assign_node_hostname. That will provide consistency for users, it means you only have to worry about one method to interpolate strings without scanning their contents, you don't have to use eval, and it supports zero padding. It uses the same formatting that's documented in the link in the existing comments, so that works out great too.

If interpolating the string raises an exception, I think the server should return some kind of error response, rather than silently handling the request without assigning a proper hostname. If the string can't be interpolated, it means the server is badly misconfigured, and I think it's appropriate to make that very visible to the server administrator. I would be fine with it if you just let the string interpolate exception bubble up to our stock error handlers. If you'd rather catch it and turn it into a nicer error response, go for it.

After that, just one style thing that wouldn't block the merge: in the loop that checks the DNS configuration, Rails.configuration.assign_node_hostname will never change, so checking it inside the loop seems slightly wasteful (and makes a bigger diff) than checking it outside the loop along with Rails.configuration.dns_server_conf_dir and Rails.configuration.dns_server_conf_template. Would it be cleaner to add the condition to the if that's already there?

Thanks.

Thanks Brett for the feedback. Isn't silly that I didn't see that about "in the loop that checks the DNS configuration ..." :).

All suggested updates are made. Assigning the review task back to you, in case you want to take another quick look. Thanks.

82a3d53 is good to merge. I would only suggest a couple of small test improvements from here.

There's a test "ping node with no hostname and malformed config and expect nil for hostname." After the change made from the last round of comments, this title no longer accurately reflects the desired behavior. Updating the test title would be a good start.

I also wonder if we really want to assert that the method raises ArgumentError. To me, that seems a little too specific: our main concern is that the server returns an error, and less about what specific exception causes it. This test might be more accurately written as a controller test which asserts that the server responds with 422 or a 5xx status code. That test would continue to work even if later on we catch the exception and render a specific error.

Thanks.

Thanks Brett. Converted that unit test into a controller test.