Repository: skypilot-org/skypilot
Pull request: [UX] Remove warning for gcp check
<!-- Describe the changes in this PR -->

Fixes #3060

This also adds the detailed error for GCP operation in the exception raised to make it easier to debug.

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [ ] Code formatting: `bash format.sh`
- [x] Any manual or new tests for this PR (please specify below)
  - [x] `sky check gcp`
- [ ] All smoke tests: `pytest tests/test_smoke.py` 
- [ ] Relevant individual smoke tests: `pytest tests/test_smoke.py::test_fill_in_the_name` 
- [ ] Backward compatibility tests: `conda deactivate; bash -i tests/backward_compatibility_tests.sh`


Problem statement:
[UX] `sky check` shows warning from GCP package
The following message shows during the `sky check`, which may related to some deprecation/update from google API. We should check the cause and fix them.

```console
$ sky check
Checking credentials to enable clouds for SkyPilot.
  Checking GCP...INFO:googleapiclient.discovery_cache:file_cache is only supported with oauth2client<4.0.0
  GCP: enabled                              
  AWS: enabled                              
  Azure: enabled
  ...
```

**Versions:**
``` console
$ gcloud --version
Google Cloud SDK 461.0.0
beta 2024.01.22
bundled-python3-unix 3.11.6
core 2024.01.22
Updates are available for some Google Cloud CLI components.  To install them,
please run:
  $ gcloud components update

$ pip list | grep google
google-api-core                         2.14.0
google-api-python-client                2.107.0
google-auth                             2.23.4
google-auth-httplib2                    0.1.1
google-cloud-core                       2.3.3
google-cloud-dataproc                   5.8.0
google-cloud-storage                    2.13.0
google-crc32c                           1.5.0
google-resumable-media                  2.6.0
googleapis-common-protos                1.61.0
grpc-google-iam-v1                      0.13.0
```

Hints from issue discussion:
Related UX: now that we have many clouds being checked here, most are likely to show as disabled. It'd be great to print a `<checkbox> Enabled clouds: <clouds list>` or something like that at the end of the message.

Commit message: Add detailed reason in operation failure

Patch to review:
diff --git a/sky/clouds/gcp.py b/sky/clouds/gcp.py
index 93260533f27..9b2a1494030 100644
--- a/sky/clouds/gcp.py
+++ b/sky/clouds/gcp.py
@@ -742,7 +742,8 @@ def check_credentials(cls) -> Tuple[bool, Optional[str]]:
         credentials, project = google.auth.default()
         crm = googleapiclient.discovery.build('cloudresourcemanager',
                                               'v1',
-                                              credentials=credentials)
+                                              credentials=credentials,
+                                              cache_discovery=False)
         gcp_minimal_permissions = gcp_utils.get_minimal_permissions()
         permissions = {'permissions': gcp_minimal_permissions}
         request = crm.projects().testIamPermissions(resource=project,
diff --git a/sky/provision/gcp/instance_utils.py b/sky/provision/gcp/instance_utils.py
index dde0918274d..59b94a983b7 100644
--- a/sky/provision/gcp/instance_utils.py
+++ b/sky/provision/gcp/instance_utils.py
@@ -100,8 +100,8 @@ def _generate_node_name(cluster_name: str, node_suffix: str,
     return node_name
 
 
-def _log_errors(errors: List[Dict[str, str]], e: Any,
-                zone: Optional[str]) -> None:
+def _format_message_from_errors(errors: List[Dict[str, str]], e: Any,
+                                zone: Optional[str]) -> str:
     """Format errors into a string."""
     if errors:
         plural = 's' if len(errors) > 1 else ''
@@ -109,10 +109,11 @@ def _log_errors(errors: List[Dict[str, str]], e: Any,
         messages = '; '.join(
             repr(e.get('message', 'N/A').strip('.')) for e in errors)
         zone_str = f' in {zone}' if zone else ''
-        logger.warning(f'Got return code{plural} {codes}'
-                       f'{zone_str}: {messages}')
+        msg = f'Got return code{plural} {codes}{zone_str}: {messages}'
     else:
-        logger.warning(f'create_instances: Failed with reason: {e}')
+        msg = f'create_instances: Failed with reason: {e}'
+    logger.warning(msg)
+    return msg
 
 
 def selflink_to_name(selflink: str) -> str:
@@ -441,8 +442,9 @@ def call_operation(fn, timeout: int):
                     logger.debug(
                         'wait_operations: Failed to create instances. Reason: '
                         f'{errors}')
-                    _log_errors(errors, result, zone)
+                    msg = _format_message_from_errors(errors, result, zone)
                     error = common.ProvisionerError('Operation failed')
+                    setattr(error, 'detailed_reason', msg)
                     error.errors = errors
                     raise error
                 return
@@ -462,8 +464,9 @@ def call_operation(fn, timeout: int):
                 'message': f'Timeout waiting for operation {operation["name"]}',
                 'domain': 'wait_for_operation'
             }]
-            _log_errors(errors, None, zone)
+            msg = _format_message_from_errors(errors, None, zone)
             error = common.ProvisionerError('Operation timed out')
+            setattr(error, 'detailed_reason', msg)
             error.errors = errors
             raise error
 
@@ -819,7 +822,7 @@ def _handle_http_error(e):
                 })
             logger.debug(
                 f'create_instances: googleapiclient.errors.HttpError: {e}')
-            _log_errors(errors, e, zone)
+            _format_message_from_errors(errors, e, zone)
             return errors
 
         # Allow Google Compute Engine instance templates.
@@ -849,7 +852,7 @@ def _handle_http_error(e):
             if errors:
                 logger.debug('create_instances: Failed to create instances. '
                              f'Reason: {errors}')
-                _log_errors(errors, operations, zone)
+                _format_message_from_errors(errors, operations, zone)
                 return errors
 
         logger.debug('Waiting GCP instances to be ready ...')
@@ -1257,7 +1260,7 @@ def create_instances(
                         'domain': 'create_instances',
                         'message': error_details,
                     })
-                    _log_errors(errors, e, zone)
+                    _format_message_from_errors(errors, e, zone)
                     return errors, names
                 for detail in error_details:
                     # To be consistent with error messages returned by operation
@@ -1276,7 +1279,7 @@ def create_instances(
                                 'domain': violation.get('subject'),
                                 'message': violation.get('description'),
                             })
-                _log_errors(errors, e, zone)
+                _format_message_from_errors(errors, e, zone)
                 return errors, names
         errors = []
         for operation in operations:
@@ -1294,7 +1297,7 @@ def create_instances(
         if errors:
             logger.debug('create_instances: Failed to create instances. '
                          f'Reason: {errors}')
-            _log_errors(errors, operations, zone)
+            _format_message_from_errors(errors, operations, zone)
             return errors, names
 
         logger.debug('Waiting GCP instances to be ready ...')
@@ -1336,7 +1339,7 @@ def create_instances(
                 'message': 'Timeout waiting for creation operation',
                 'domain': 'create_instances'
             }]
-            _log_errors(errors, None, zone)
+            _format_message_from_errors(errors, None, zone)
             return errors, names
 
         # NOTE: Error example:
@@ -1353,7 +1356,7 @@ def create_instances(
             logger.debug(
                 'create_instances: Failed to create instances. Reason: '
                 f'{errors}')
-            _log_errors(errors, results, zone)
+            _format_message_from_errors(errors, results, zone)
             return errors, names
         assert all(success), (
             'Failed to create instances, but there is no error. '
@@ -1475,7 +1478,7 @@ def create_tpu_node(project_id: str, zone: str, tpu_node_config: Dict[str, str],
                            'https://console.cloud.google.com/iam-admin/quotas '
                            'for more information.'
             }]
-            _log_errors(provisioner_err.errors, e, zone)
+            _format_message_from_errors(provisioner_err.errors, e, zone)
             raise provisioner_err from e
 
         if 'PERMISSION_DENIED' in stderr:
@@ -1484,7 +1487,7 @@ def create_tpu_node(project_id: str, zone: str, tpu_node_config: Dict[str, str],
                 'domain': 'tpu',
                 'message': 'TPUs are not available in this zone.'
             }]
-            _log_errors(provisioner_err.errors, e, zone)
+            _format_message_from_errors(provisioner_err.errors, e, zone)
             raise provisioner_err from e
 
         if 'no more capacity in the zone' in stderr:
@@ -1493,7 +1496,7 @@ def create_tpu_node(project_id: str, zone: str, tpu_node_config: Dict[str, str],
                 'domain': 'tpu',
                 'message': 'No more capacity in this zone.'
             }]
-            _log_errors(provisioner_err.errors, e, zone)
+            _format_message_from_errors(provisioner_err.errors, e, zone)
             raise provisioner_err from e
 
         if 'CloudTpu received an invalid AcceleratorType' in stderr:
@@ -1506,7 +1509,7 @@ def create_tpu_node(project_id: str, zone: str, tpu_node_config: Dict[str, str],
                 'message': (f'TPU type {tpu_type} is not available in this '
                             f'zone {zone}.')
             }]
-            _log_errors(provisioner_err.errors, e, zone)
+            _format_message_from_errors(provisioner_err.errors, e, zone)
             raise provisioner_err from e
 
         # TODO(zhwu): Add more error code handling, if needed.
@@ -1515,7 +1518,7 @@ def create_tpu_node(project_id: str, zone: str, tpu_node_config: Dict[str, str],
             'domain': 'tpu',
             'message': stderr
         }]
-        _log_errors(provisioner_err.errors, e, zone)
+        _format_message_from_errors(provisioner_err.errors, e, zone)
         raise provisioner_err from e
 
 


Code review comments:
diff_hunk: @@ -100,19 +100,20 @@ def _generate_node_name(cluster_name: str, node_suffix: str,
     return node_name
 
 
-def _log_errors(errors: List[Dict[str, str]], e: Any,
-                zone: Optional[str]) -> None:
+def _format_message_from_errors(errors: List[Dict[str, str]], e: Any,
+                                zone: Optional[str]) -> str:
     """Format errors into a string."""
path: sky/provision/gcp/instance_utils.py
text: @user1:
```suggestion
def _format_message_from_errors_and_log(errors: List[Dict[str, str]], e: Any,
                                zone: Optional[str]) -> str:
    """Format errors into a string and log them to the console."""
```
to better align the actual behaviour of this function?

@author:
Good point! Added. Thanks!