Repository: skypilot-org/skypilot
Pull request #1857 (2023-04-13 19:26:05)

[Storage] Improve UX for error message when bucket is not accessible
Changes the error message when a bucket is not accessible to focus on fix rather than the problem. Considered moving the root error (boto3/gcloud) to after our exception or removing it altogether, but decided it's better to let it come first, followed by our exception.

Before:
```
botocore.exceptions.ClientError: An error occurred (403) when calling the HeadBucket operation: Forbidden

The above exception was the direct cause of the following exception:

sky.exceptions.StorageBucketGetError: Failed to connect to an existing bucket 'mybucket'.
Please check if:
  1. the bucket name is taken and/or 
  2. the bucket permissions are not setup correctly. To debug, consider using aws s3 ls checkpoint.
```

After:
```
botocore.exceptions.ClientError: An error occurred (403) when calling the HeadBucket operation: Forbidden

The above exception was the direct cause of the following exception:

sky.exceptions.StorageBucketGetError: Failed to connect to an existing bucket 'mybucket'. This is likely because it is a private bucket you do not have access to.
To fix: 
  1. If you are trying to create a new bucket - use a different name.
  2. If you are trying to connect to an existing bucket - make sure you have access to it. To debug, consider running `aws s3 ls mybucket`.
```
Tested (run the relevant ones):

- [x] Test with a common bucket name like `checkpoint` for s3, gcs and r2.
- [x] Relevant individual smoke tests: `pytest tests/test_smoke.py::TestStorageWithCredentials`



Patch:
diff --git a/sky/cli.py b/sky/cli.py
index 12ed9ba11..75948ca58 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -1694,6 +1694,7 @@ def cost_report(all: bool):  # pylint: disable=redefined-builtin
       - clusters that were terminated/stopped on the cloud console.
     """
     cluster_records = core.cost_report()
+
     nonreserved_cluster_records = []
     reserved_clusters = dict()
     for cluster_record in cluster_records:
@@ -1701,17 +1702,35 @@ def cost_report(all: bool):  # pylint: disable=redefined-builtin
         if cluster_name in backend_utils.SKY_RESERVED_CLUSTER_NAMES:
             cluster_group_name = backend_utils.SKY_RESERVED_CLUSTER_NAMES[
                 cluster_name]
-            reserved_clusters[cluster_group_name] = cluster_record
+            # to display most recent entry for each reserved cluster
+            # TODO(sgurram): fix assumption of sorted order of clusters
+            if cluster_group_name not in reserved_clusters:
+                reserved_clusters[cluster_group_name] = cluster_record
         else:
             nonreserved_cluster_records.append(cluster_record)
 
-    status_utils.show_cost_report_table(nonreserved_cluster_records, all)
+    total_cost = status_utils.get_total_cost_of_displayed_records(
+        nonreserved_cluster_records, all)
 
+    status_utils.show_cost_report_table(nonreserved_cluster_records, all)
     for cluster_group_name, cluster_record in reserved_clusters.items():
         status_utils.show_cost_report_table(
             [cluster_record], all, reserved_group_name=cluster_group_name)
+        total_cost += cluster_record['total_cost']
+
+    click.echo(f'\n{colorama.Style.BRIGHT}'
+               f'Total Cost: ${total_cost:.2f}{colorama.Style.RESET_ALL}')
+
+    if not all:
+        click.secho(
+            f'Showing up to {status_utils.NUM_COST_REPORT_LINES} '
+            'most recent clusters. '
+            'To see all clusters in history, '
+            'pass the --all flag.',
+            fg='yellow')
+
     click.secho(
-        'NOTE: This feature is experimental. '
+        'This feature is experimental. '
         'Costs for clusters with auto{stop,down} '
         'scheduled may not be accurate.',
         fg='yellow')
diff --git a/sky/data/storage.py b/sky/data/storage.py
index 10d087520..a2355bac5 100644
--- a/sky/data/storage.py
+++ b/sky/data/storage.py
@@ -1,6 +1,7 @@
 """Storage and Store Classes for Sky Data."""
 import enum
 import os
+import re
 import subprocess
 import time
 import typing
@@ -45,9 +46,12 @@ STORE_ENABLED_CLOUDS = [clouds.AWS(), clouds.GCP()]
 _MAX_CONCURRENT_UPLOADS = 32
 
 _BUCKET_FAIL_TO_CONNECT_MESSAGE = (
-    'Failed to connect to an existing bucket {name!r}.\n'
-    'Please check if:\n  1. the bucket name is taken and/or '
-    '\n  2. the bucket permissions are not setup correctly.')
+    'Failed to access existing bucket {name!r}. '
+    'This is likely because it is a private bucket you do not have access to.\n'
+    'To fix: \n'
+    '  1. If you are trying to create a new bucket: use a different name.\n'
+    '  2. If you are trying to connect to an existing bucket: make sure '
+    'your cloud credentials have access to it.')
 
 
 class StoreType(enum.Enum):
@@ -562,6 +566,27 @@ class Storage(object):
         """
         Validates the storage spec and updates local fields if necessary.
         """
+
+        def validate_name(name):
+            """ Checks for validating the storage name.
+
+            Checks if the name starts the s3, gcs or r2 prefix and raise error
+            if it does. Store specific validation checks (e.g., S3 specific
+            rules) happen in the corresponding store class.
+            """
+            prefix = name.split('://')[0]
+            prefix = prefix.lower()
+            if prefix in ['s3', 'gs', 'r2']:
+                with ux_utils.print_exception_no_traceback():
+                    raise exceptions.StorageNameError(
+                        'Prefix detected: `name` cannot start with '
+                        f'{prefix}://. If you are trying to use an existing '
+                        'bucket created outside of SkyPilot, please specify it '
+                        'using the `source` field (e.g. '
+                        '`source: s3://mybucket/`). If you are trying to '
+                        'create a new bucket, please use the `store` field to '
+                        'specify the store type (e.g. `store: s3`).')
+
         if self.source is None:
             # If the mode is COPY, the source must be specified
             if self.mode == StorageMode.COPY:
@@ -584,6 +609,7 @@ class Storage(object):
                         raise exceptions.StorageSpecError(
                             'Storage source or storage name must be specified.')
             assert name is not None, handle
+            validate_name(name)
             self.name = name
             return
         elif self.source is not None:
@@ -606,6 +632,7 @@ class Storage(object):
                 if is_local_source:
                     # If name is specified and source is local, upload to bucket
                     assert name is not None, source
+                    validate_name(name)
                     self.name = name
                     return
                 else:
@@ -828,7 +855,7 @@ class S3Store(AbstractStore):
     for S3 buckets.
     """
 
-    ACCESS_DENIED_MESSAGE = 'Access Denied'
+    _ACCESS_DENIED_MESSAGE = 'Access Denied'
 
     def __init__(self,
                  name: str,
@@ -859,6 +886,67 @@ class S3Store(AbstractStore):
                 assert data_utils.verify_r2_bucket(self.name), (
                     f'Source specified as {self.source}, a R2 bucket. ',
                     'R2 Bucket should exist.')
+        # Validate name
+        self.name = self.validate_name(self.name)
+
+    @classmethod
+    def validate_name(cls, name) -> str:
+        """Validates the name of the S3 store.
+
+        Source for rules: https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html # pylint: disable=line-too-long
+        """
+
+        def _raise_no_traceback_name_error(err_str):
+            with ux_utils.print_exception_no_traceback():
+                raise exceptions.StorageNameError(err_str)
+
+        if name is not None and isinstance(name, str):
+            if not 3 <= len(name) <= 63:
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must be between 3 (min) '
+                    'and 63 (max) characters long.')
+
+            # Check for valid characters and start/end with a letter or number
+            pattern = r'^[a-z0-9][-a-z0-9.]*[a-z0-9]$'
+            if not re.match(pattern, name):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} can consist only of '
+                    'lowercase letters, numbers, dots (.), and hyphens (-). '
+                    'It must begin and end with a letter or number.')
+
+            # Check for two adjacent periods
+            if '..' in name:
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must not contain '
+                    'two adjacent periods.')
+
+            # Check for IP address format
+            ip_pattern = r'^(?:\d{1,3}\.){3}\d{1,3}$'
+            if re.match(ip_pattern, name):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must not be formatted as '
+                    'an IP address (for example, 192.168.5.4).')
+
+            # Check for 'xn--' prefix
+            if name.startswith('xn--'):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must not start with the '
+                    'prefix "xn--".')
+
+            # Check for '-s3alias' suffix
+            if name.endswith('-s3alias'):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must not end with the '
+                    'suffix "-s3alias".')
+
+            # Check for '--ol-s3' suffix
+            if name.endswith('--ol-s3'):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must not end with the '
+                    'suffix "--ol-s3".')
+        else:
+            _raise_no_traceback_name_error('Store name must be specified.')
+        return name
 
     def initialize(self):
         """Initializes the S3 store object on the cloud.
@@ -966,7 +1054,7 @@ class S3Store(AbstractStore):
                 get_file_sync_command,
                 get_dir_sync_command,
                 self.name,
-                self.ACCESS_DENIED_MESSAGE,
+                self._ACCESS_DENIED_MESSAGE,
                 create_dirs=create_dirs,
                 max_concurrent_uploads=_MAX_CONCURRENT_UPLOADS)
 
@@ -1008,7 +1096,7 @@ class S3Store(AbstractStore):
                 with ux_utils.print_exception_no_traceback():
                     raise exceptions.StorageBucketGetError(
                         _BUCKET_FAIL_TO_CONNECT_MESSAGE.format(name=self.name) +
-                        f' To debug, consider using {command}.') from e
+                        f' To debug, consider running `{command}`.') from e
 
         if isinstance(self.source, str) and self.source.startswith('s3://'):
             with ux_utils.print_exception_no_traceback():
@@ -1112,7 +1200,7 @@ class GcsStore(AbstractStore):
     for GCS buckets.
     """
 
-    ACCESS_DENIED_MESSAGE = 'AccessDeniedException'
+    _ACCESS_DENIED_MESSAGE = 'AccessDeniedException'
     GCSFUSE_VERSION = '0.42.3'
 
     def __init__(self,
@@ -1150,6 +1238,66 @@ class GcsStore(AbstractStore):
                     assert data_utils.verify_r2_bucket(self.name), (
                         f'Source specified as {self.source}, a R2 bucket. ',
                         'R2 Bucket should exist.')
+        # Validate name
+        self.name = self.validate_name(self.name)
+
+    @classmethod
+    def validate_name(cls, name) -> str:
+        """Validates the name of the GCS store.
+
+        Source for rules: https://cloud.google.com/storage/docs/buckets#naming
+        """
+
+        def _raise_no_traceback_name_error(err_str):
+            with ux_utils.print_exception_no_traceback():
+                raise exceptions.StorageNameError(err_str)
+
+        if name is not None and isinstance(name, str):
+            # Check for overall length
+            if not 3 <= len(name) <= 222:
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must contain 3-222 '
+                    'characters.')
+
+            # Check for valid characters and start/end with a number or letter
+            pattern = r'^[a-z0-9][-a-z0-9._]*[a-z0-9]$'
+            if not re.match(pattern, name):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} can only contain '
+                    'lowercase letters, numeric characters, dashes (-), '
+                    'underscores (_), and dots (.). Spaces are not allowed. '
+                    'Names must start and end with a number or letter.')
+
+            # Check for 'goog' prefix and 'google' in the name
+            if name.startswith('goog') or any(
+                    s in name
+                    for s in ['google', 'g00gle', 'go0gle', 'g0ogle']):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} cannot begin with the '
+                    '"goog" prefix or contain "google" in various forms.')
+
+            # Check for dot-separated components length
+            components = name.split('.')
+            if any(len(component) > 63 for component in components):
+                _raise_no_traceback_name_error(
+                    'Invalid store name: Dot-separated components in name '
+                    f'{name} can be no longer than 63 characters.')
+
+            if '..' in name or '.-' in name or '-.' in name:
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} must not contain two '
+                    'adjacent periods or a dot next to a hyphen.')
+
+            # Check for IP address format
+            ip_pattern = r'^(?:\d{1,3}\.){3}\d{1,3}$'
+            if re.match(ip_pattern, name):
+                _raise_no_traceback_name_error(
+                    f'Invalid store name: name {name} cannot be represented as '
+                    'an IP address in dotted-decimal notation '
+                    '(for example, 192.168.5.4).')
+        else:
+            _raise_no_traceback_name_error('Store name must be specified.')
+        return name
 
     def initialize(self):
         """Initializes the GCS store object on the cloud.
@@ -1243,7 +1391,7 @@ class GcsStore(AbstractStore):
                 f'[bold cyan]Syncing '
                 f'[green]{source_message}[/] to [green]gs://{self.name}/[/]'):
             data_utils.run_upload_cli(sync_command,
-                                      self.ACCESS_DENIED_MESSAGE,
+                                      self._ACCESS_DENIED_MESSAGE,
                                       bucket_name=self.name)
 
     def batch_gsutil_rsync(self,
@@ -1293,7 +1441,7 @@ class GcsStore(AbstractStore):
                 get_file_sync_command,
                 get_dir_sync_command,
                 self.name,
-                self.ACCESS_DENIED_MESSAGE,
+                self._ACCESS_DENIED_MESSAGE,
                 create_dirs=create_dirs,
                 max_concurrent_uploads=_MAX_CONCURRENT_UPLOADS)
 
@@ -1342,7 +1490,7 @@ class GcsStore(AbstractStore):
                 with ux_utils.print_exception_no_traceback():
                     raise exceptions.StorageBucketGetError(
                         _BUCKET_FAIL_TO_CONNECT_MESSAGE.format(name=self.name) +
-                        f' To debug, consider using {command}.') from e
+                        f' To debug, consider running `{command}`.') from e
 
     def mount_command(self, mount_path: str) -> str:
         """Returns the command to mount the bucket to the mount_path.
@@ -1436,7 +1584,7 @@ class R2Store(AbstractStore):
     for R2 buckets.
     """
 
-    ACCESS_DENIED_MESSAGE = 'Access Denied'
+    _ACCESS_DENIED_MESSAGE = 'Access Denied'
 
     def __init__(self,
                  name: str,
@@ -1467,6 +1615,7 @@ class R2Store(AbstractStore):
                 assert self.name == data_utils.split_r2_path(self.source)[0], (
                     'R2 Bucket is specified as path, the name should be '
                     'the same as R2 bucket.')
+        self.name = S3Store.validate_name(self.name)
 
     def initialize(self):
         """Initializes the R2 store object on the cloud.
@@ -1580,7 +1729,7 @@ class R2Store(AbstractStore):
                 get_file_sync_command,
                 get_dir_sync_command,
                 self.name,
-                self.ACCESS_DENIED_MESSAGE,
+                self._ACCESS_DENIED_MESSAGE,
                 create_dirs=create_dirs,
                 max_concurrent_uploads=_MAX_CONCURRENT_UPLOADS)
 
@@ -1624,7 +1773,7 @@ class R2Store(AbstractStore):
                 with ux_utils.print_exception_no_traceback():
                     raise exceptions.StorageBucketGetError(
                         _BUCKET_FAIL_TO_CONNECT_MESSAGE.format(name=self.name) +
-                        f' To debug, consider using {command}.') from e
+                        f' To debug, consider running `{command}`.') from e
 
         if isinstance(self.source, str) and self.source.startswith('r2://'):
             with ux_utils.print_exception_no_traceback():
diff --git a/sky/global_user_state.py b/sky/global_user_state.py
index 7570cfe41..4bd6ff679 100644
--- a/sky/global_user_state.py
+++ b/sky/global_user_state.py
@@ -459,6 +459,8 @@ def _get_cluster_duration(cluster_hash: str) -> int:
 
     for i, (start_time, end_time) in enumerate(usage_intervals):
         # duration from latest start time to time of query
+        if start_time is None:
+            continue
         if end_time is None:
             assert i == len(usage_intervals) - 1, i
             end_time = int(time.time())
diff --git a/sky/utils/cli_utils/status_utils.py b/sky/utils/cli_utils/status_utils.py
index a265304ec..a1c1743e2 100644
--- a/sky/utils/cli_utils/status_utils.py
+++ b/sky/utils/cli_utils/status_utils.py
@@ -11,6 +11,7 @@ from sky.utils import common_utils
 from sky.utils import log_utils
 
 _COMMAND_TRUNC_LENGTH = 25
+NUM_COST_REPORT_LINES = 5
 
 # A record in global_user_state's 'clusters' table.
 _ClusterRecord = Dict[str, Any]
@@ -104,6 +105,20 @@ def show_status_table(cluster_records: List[_ClusterRecord],
     return num_pending_autostop
 
 
+def get_total_cost_of_displayed_records(
+        cluster_records: List[_ClusterCostReportRecord], display_all: bool):
+    """Compute total cost of records to be displayed in cost report."""
+    cluster_records.sort(
+        key=lambda report: -_get_status_value_for_cost_report(report))
+
+    displayed_records = cluster_records[:NUM_COST_REPORT_LINES]
+    if display_all:
+        displayed_records = cluster_records
+
+    total_cost = sum(record['total_cost'] for record in displayed_records)
+    return total_cost
+
+
 def show_cost_report_table(cluster_records: List[_ClusterCostReportRecord],
                            show_all: bool,
                            reserved_group_name: Optional[str] = None):
@@ -153,7 +168,15 @@ def show_cost_report_table(cluster_records: List[_ClusterCostReportRecord],
             columns.append(status_column.name)
     cluster_table = log_utils.create_table(columns)
 
-    for record in cluster_records:
+    num_lines_to_display = NUM_COST_REPORT_LINES
+    if show_all:
+        num_lines_to_display = len(cluster_records)
+
+    # prioritize showing non-terminated clusters in table
+    cluster_records.sort(
+        key=lambda report: -_get_status_value_for_cost_report(report))
+
+    for record in cluster_records[:num_lines_to_display]:
         row = []
         for status_column in status_columns:
             if status_column.show_by_default or show_all:
@@ -340,6 +363,14 @@ def _is_pending_autostop(cluster_record: _ClusterRecord) -> bool:
 # ---- 'sky cost-report' helper functions below ----
 
 
+def _get_status_value_for_cost_report(
+        cluster_cost_report_record: _ClusterCostReportRecord) -> int:
+    status = cluster_cost_report_record['status']
+    if status is None:
+        return -1
+    return 1
+
+
 def _get_status_for_cost_report(
         cluster_cost_report_record: _ClusterCostReportRecord) -> str:
     status = cluster_cost_report_record['status']


Test patch:
diff --git a/tests/test_smoke.py b/tests/test_smoke.py
index 023ed7296..c70f85aac 100644
--- a/tests/test_smoke.py
+++ b/tests/test_smoke.py
@@ -1665,6 +1665,46 @@ def test_core_api():
 class TestStorageWithCredentials:
     """Storage tests which require credentials and network connection"""
 
+    AWS_INVALID_NAMES = [
+        'ab',  # less than 3 characters
+        'abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz1',
+        # more than 63 characters
+        'Abcdef',  # contains an uppercase letter
+        'abc def',  # contains a space
+        'abc..def',  # two adjacent periods
+        '192.168.5.4',  # formatted as an IP address
+        'xn--bucket',  # starts with 'xn--' prefix
+        'bucket-s3alias',  # ends with '-s3alias' suffix
+        'bucket--ol-s3',  # ends with '--ol-s3' suffix
+        '.abc',  # starts with a dot
+        'abc.',  # ends with a dot
+        '-abc',  # starts with a hyphen
+        'abc-',  # ends with a hyphen
+    ]
+
+    GCS_INVALID_NAMES = [
+        'ab',  # less than 3 characters
+        'abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz1',
+        # more than 63 characters (without dots)
+        'Abcdef',  # contains an uppercase letter
+        'abc def',  # contains a space
+        'abc..def',  # two adjacent periods
+        'abc_.def.ghi.jklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz1'
+        # More than 63 characters between dots
+        'abc_.def.ghi.jklmnopqrstuvwxyzabcdefghijklmnopqfghijklmnopqrstuvw' * 5,
+        # more than 222 characters (with dots)
+        '192.168.5.4',  # formatted as an IP address
+        'googbucket',  # starts with 'goog' prefix
+        'googlebucket',  # contains 'google'
+        'g00glebucket',  # variant of 'google'
+        'go0glebucket',  # variant of 'google'
+        'g0oglebucket',  # variant of 'google'
+        '.abc',  # starts with a dot
+        'abc.',  # ends with a dot
+        '_abc',  # starts with an underscore
+        'abc_',  # ends with an underscore
+    ]
+
     @pytest.fixture
     def tmp_source(self, tmp_path):
         # Creates a temporary directory with a file in it
@@ -2001,6 +2041,20 @@ class TestStorageWithCredentials:
             'File not found in bucket - output was : {}'.format(out.decode
                                                                 ('utf-8'))
 
+    @pytest.mark.parametrize('invalid_name_list, store_type',
+                             [(AWS_INVALID_NAMES, storage_lib.StoreType.S3),
+                              (GCS_INVALID_NAMES, storage_lib.StoreType.GCS),
+                              pytest.param(AWS_INVALID_NAMES,
+                                           storage_lib.StoreType.R2,
+                                           marks=pytest.mark.cloudflare)])
+    def test_invalid_names(self, invalid_name_list, store_type):
+        # Uses a list in the source field to specify a file and a directory to
+        # be uploaded to the storage object.
+        for name in invalid_name_list:
+            with pytest.raises(sky.exceptions.StorageNameError):
+                storage_obj = storage_lib.Storage(name=name)
+                storage_obj.add_store(store_type)
+
 
 # ---------- Testing YAML Specs ----------
 # Our sky storage requires credentials to check the bucket existance when
diff --git a/tests/test_storage.py b/tests/test_storage.py
index 20e75a1ea..80cd624f6 100644
--- a/tests/test_storage.py
+++ b/tests/test_storage.py
@@ -83,3 +83,20 @@ class TestStorageSpecValidation:
             storage_lib.Storage()
 
         assert 'Storage source or storage name must be specified' in str(e)
+
+    def test_uri_in_name(self):
+        """Tests when name is a URI.
+
+        Other tests for invalid names require store-specific test cases, and
+        are in test_smoke.py::TestStorageWithCredentials"""
+        invalid_names = [
+            's3://mybucket',
+            'gs://mybucket',
+            'r2://mybucket',
+        ]
+
+        for n in invalid_names:
+            with pytest.raises(exceptions.StorageNameError) as e:
+                storage_lib.Storage(name=n)
+
+            assert 'Prefix detected' in str(e)