[Lambda] Add open port support #5124
Conversation
|
/smoke-test --lambda -k test_lambda_cloud_open_ports |
| lambda_client.create_firewall_rule(port_range=[port, port], | ||
| protocol='tcp') |
There was a problem hiding this comment.
Lambda API docs say: Firewall rules do not apply to the us-south-1 region
Do we know what's the behavior in SkyPilot if we try to open instances on an instance in us-south-1? E.g., is it silently ignored with the warning in the except below?
There was a problem hiding this comment.
Good catch! Added the skip.
| Lambda Cloud firewall rules are global to the account, not cluster-specific. | ||
| We skip cleanup because rules may be used by other clusters. |
There was a problem hiding this comment.
Does this mean we'll accumulate rules over time?
This may be ok for now, but we should leave a TODO here. Perhaps in the future we can store the instances using the firewall rule in the "description" field of a firewall rule
There was a problem hiding this comment.
Added a TODO. Yes, it is a bit challenging, as users may also implicitly rely on some of the firewall rules, as it is global.
| for port in ports_to_open: | ||
| logger.info(f'Opening port {port}/tcp') |
There was a problem hiding this comment.
Would it be useful to have smarter logic here to consolidate contiguous ports numbers into a single firewall rule? E.g., if. ports_to_open is 8000-8100, we should probably create one rule with the range instead of creating 100 rules.
Ok to leave as a TODO.
There was a problem hiding this comment.
Good point! Fixed. Thanks!
| for port in ports_to_open: | ||
| logger.info(f'Opening port {port}/tcp') |
There was a problem hiding this comment.
Good point! Fixed. Thanks!
| lambda_client.create_firewall_rule(port_range=[port, port], | ||
| protocol='tcp') |
There was a problem hiding this comment.
Good catch! Added the skip.
| Lambda Cloud firewall rules are global to the account, not cluster-specific. | ||
| We skip cleanup because rules may be used by other clusters. |
There was a problem hiding this comment.
Added a TODO. Yes, it is a bit challenging, as users may also implicitly rely on some of the firewall rules, as it is global.
| lambda_utils._try_request_with_backoff( | ||
| 'put', | ||
| f'{lambda_utils.API_ENDPOINT}/firewall-rules', | ||
| data=data, | ||
| headers=lambda_client.headers, | ||
| ) |
There was a problem hiding this comment.
side comment: wow Lambda really needs to add a DELETE function instead of clients needing to fetch and update all rules...
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)