Arvados

23027-ansible-service-ports @ 67e9e90d0e6b058912f5556d7333bf44994590a5

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • Note the original math in the comment refers to "concurrent clients." What number you expect there is site-specific and hard for us (and any administrator) to predict, you can really only measure it per site. For now I went with "worker_connections is 150% the listening ports; worker_rlimit_nofile is 200%." This roughly tracks the suggested ratio without getting too wound up about a specific value for concurrent clients and keeps the math simple and easy to follow. I suspect as we prepare the Ansible installer for production multi-node clusters, we may need to revisit this. But since the current implementation is aimed at single-node installs (just like the rest of the whole external service ports feature), I hope this is okay for version one.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • N/A
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • Deployed a new single-node cluster with 1000 ports, 100 ports, and no external ports. Got the expected results in each case: the controller site adapted to match no matter what, and the core worker configuration only adapted for the 1000 ports case.
• New or changed UI/UX and has gotten feedback from stakeholders.
  • N/A - no new Ansible configuration, just reads new settings in the Arvados configuration
• Documentation has been updated.
  • #22866 will provide an example configuration file that can illustrate the expected use.
• Behaves appropriately at the intended scale (describe intended scale).
  • Only affects nginx scaling in ways that are expected and necessary for functionality.
• Considered backwards and forwards compatibility issues between client and server.
  • Only manages more configuration, so should deploy okay on any existing Ansible-deployed clusters. Other use cases are out of scope for the Ansible installer at this time.
• Follows our coding standards and GUI style guidelines.
  • N/A for these languages

A couple of suggestions:

• Instead of arvados-ansible.conf, do you think it can be more informative to call it something like arvados-worker-conn.conf? The comment that it's managed by Ansible is already included inside it, so it isn't necessary in the name.
• In the arvados-nginx-site template there's some validation on port numbers. I think it would also be useful to do validation to catch typos in the playbook task, so that it fails when max-port < min-port, so the error is catched earlier and more explicitly, wdyt?

Lucas Di Pentima wrote in #note-3:

A couple of suggestions:

• Instead of arvados-ansible.conf, do you think it can be more informative to call it something like arvados-worker-conn.conf? The comment that it's managed by Ansible is already included inside it, so it isn't necessary in the name.

I'm open to changing the name but I wanted to leave a little flexibility. While right now it only adjusts worker configuration, in the future we could put any nginx core directives we want in here, and I could see that happening. What do you think about arvados-core? Something else?

• In the arvados-nginx-site template there's some validation on port numbers. I think it would also be useful to do validation to catch typos in the playbook task, so that it fails when max-port < min-port, so the error is catched earlier and more explicitly, wdyt?

I can add the 0 < min_port < max_port condition to the playbook, so it behaves consistently with the template. Beyond that, I think the best place to do this check is when Arvados itself validates the config, and I would prefer to leave this to #22862. That way we can fail immediately rather than mid-playbook, which always has the chance to leave the cluster in a bad half-configured state. Sound good?

Brett Smith wrote in #note-4:

I'm open to changing the name but I wanted to leave a little flexibility. While right now it only adjusts worker configuration, in the future we could put any nginx core directives we want in here, and I could see that happening. What do you think about arvados-core? Something else?

I see! ansible-core is a good option, thanks!

I can add the 0 < min_port < max_port condition to the playbook, so it behaves consistently with the template. Beyond that, I think the best place to do this check is when Arvados itself validates the config, and I would prefer to leave this to #22862. That way we can fail immediately rather than mid-playbook, which always has the chance to leave the cluster in a bad half-configured state. Sound good?

Yes, I wasn't thinking on a first config-check stage. That's a lot better!

With those changes, it LGTM.

Now at fe2467c3dfd4f46759953eab4e49d517390c5626 with the agreed changes. Tested again with 1000 ports, still adds the desired configuration.

Thank you. LGTM