[Storage][Spot] Resolving failure to exclude local files for managed spot jobs using git --ignored outputs

sky/data/storage.py

@@ -873,6 +873,75 @@ def add_if_not_none(key: str, value: Optional[Any]):
         return config
 
 
+def format_gitignore_to_exclude_list(src_dir_path: str) -> List[str]:
+    """Returns a list of formatted excluded files read from .gitignore and
+    .git/info/exclude using git. 'git init' is ran when SRC_DIR_PATH
+    is not a git repository and removed after obtaining excluded list
+    """
+    expand_src_dir_path = os.path.expanduser(src_dir_path)
+
+    git_exclude_path = os.path.join(expand_src_dir_path, '.git/info/exclude')
+    gitignore_path = os.path.join(expand_src_dir_path, '.gitignore')
+
+    git_exclude_exists = os.path.isfile(git_exclude_path)
+    gitignore_exists = os.path.isfile(gitignore_path)
+
+    # This command outputs a list to be excluded according to .gitignore
+    # and .git/info/exclude
+    filter_cmd = f'git -C {expand_src_dir_path} status --ignored -s'
+    excluded_list: List[str] = ['.git/*', '.gitignore']
+
+    # pylint: disable=W1510
+    if git_exclude_exists or gitignore_exists:
+        try:
+            output = subprocess.run(filter_cmd,
+                                    shell=True,
+                                    stdout=subprocess.PIPE,
+                                    stderr=subprocess.PIPE,
+                                    check=True,
+                                    text=True)
+        except subprocess.CalledProcessError as e:
+            # when the SRC_DIR_PATH is not a git repo
+            if e.returncode == exceptions.GIT_UNINITIALIZED_CODE:
+                init_cmd = f'git -C {expand_src_dir_path} init'
+                subprocess.run(init_cmd,
+                               shell=True,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE)
+                output = subprocess.run(filter_cmd,
+                                        shell=True,
+                                        stdout=subprocess.PIPE,
+                                        stderr=subprocess.PIPE,
+                                        text=True)
+                if git_exclude_exists:
+                    # removes all the files/dirs created with 'git init'
+                    # under .git/ except .git/info/exclude
+                    remove_files_cmd = (f'find {expand_src_dir_path}/.git ' \
+                                        f'-path {git_exclude_path} -prune -o ' \
+                                        '-type f -exec rm -f {} +')
+                    remove_dirs_cmd = (f'find {expand_src_dir_path}/.git ' \
+                                       f'-path {git_exclude_path} -prune -o' \
+                                       ' -type d -empty -delete')
+                    subprocess.run(remove_files_cmd,
+                                   shell=True,
+                                   stdout=subprocess.PIPE,
+                                   stderr=subprocess.PIPE)
+                    subprocess.run(remove_dirs_cmd,
+                                   shell=True,
+                                   stdout=subprocess.PIPE,
+                                   stderr=subprocess.PIPE)
+
+        output_list = output.stdout.split('\n')
+        for line in output_list:
+            if line.startswith('!!'):
+                to_be_excluded = line[3:]
+                if line.endswith('/'):
+                    to_be_excluded += '*'
+                excluded_list.append(to_be_excluded)
+    # pylint: enable=W1510
+    return excluded_list
+
+
 class S3Store(AbstractStore):
     """S3Store inherits from Storage Object and represents the backend
     for S3 buckets.
@@ -1075,10 +1144,12 @@ def get_file_sync_command(base_dir_path, file_names):
 
         def get_dir_sync_command(src_dir_path, dest_dir_name):
             # we exclude .git directory from the sync
-            sync_command = (
-                'aws s3 sync --no-follow-symlinks --exclude ".git/*" '
-                f'{src_dir_path} '
-                f's3://{self.name}/{dest_dir_name}')
+            excluded_list = format_gitignore_to_exclude_list(src_dir_path)
+            excludes = ' '.join(
+                [f'--exclude "{file_name}"' for file_name in excluded_list])
+            sync_command = (f'aws s3 sync --no-follow-symlinks {excludes} '
+                            f'{src_dir_path} '
+                            f's3://{self.name}/{dest_dir_name}')
             return sync_command
 
         # Generate message for upload
@@ -1498,8 +1569,10 @@ def get_file_sync_command(base_dir_path, file_names):
 
         def get_dir_sync_command(src_dir_path, dest_dir_name):
             # we exclude .git directory from the sync
-            sync_command = (f'gsutil -m rsync -r -x \'.git/*\' {src_dir_path} '
-                            f'gs://{self.name}/{dest_dir_name}')
+            excluded_list = format_gitignore_to_exclude_list(src_dir_path)
+            excludes = '\'(' + '|'.join(excluded_list) + ')\''
+            sync_command = (f'gsutil -m rsync -r -x {excludes} {src_dir_path}'
+                            f' gs://{self.name}/{dest_dir_name}')
             return sync_command
 
         # Generate message for upload
@@ -1821,15 +1894,18 @@ def get_file_sync_command(base_dir_path, file_names):
 
         def get_dir_sync_command(src_dir_path, dest_dir_name):
             # we exclude .git directory from the sync
+            excluded_list = format_gitignore_to_exclude_list(src_dir_path)
+            excludes = ' '.join(
+                [f'--exclude "{file_name}"' for file_name in excluded_list])
             endpoint_url = cloudflare.create_endpoint()
-            sync_command = (
-                'AWS_SHARED_CREDENTIALS_FILE='
-                f'{cloudflare.R2_CREDENTIALS_PATH} '
-                'aws s3 sync --no-follow-symlinks --exclude ".git/*" '
-                f'{src_dir_path} '
-                f's3://{self.name}/{dest_dir_name} '
-                f'--endpoint {endpoint_url} '
-                f'--profile={cloudflare.R2_PROFILE_NAME}')
+
+            sync_command = ('AWS_SHARED_CREDENTIALS_FILE='
+                            f'{cloudflare.R2_CREDENTIALS_PATH} '
+                            f'aws s3 sync --no-follow-symlinks {excludes} '
+                            f'{src_dir_path} '
+                            f's3://{self.name}/{dest_dir_name} '
+                            f'--endpoint {endpoint_url} '
+                            f'--profile={cloudflare.R2_PROFILE_NAME}')
             return sync_command
 
         # Generate message for upload

sky/exceptions.py

@@ -12,6 +12,8 @@
 RSYNC_FILE_NOT_FOUND_CODE = 23
 # Arbitrarily chosen value. Used in SkyPilot's storage mounting scripts
 MOUNT_PATH_NON_EMPTY_CODE = 42
+# Return code when git command is ran in a dir that is not git repo
+GIT_UNINITIALIZED_CODE = 128
 
 
 class ResourcesUnavailableError(Exception):

tests/git_info_exclude_test

@@ -0,0 +1,8 @@
+question_mark/excluded?.txt
+square_bracket/excluded[0-9].txt
+square_bracket_single/excluded[01].txt
+square_bracket_excla/excluded[!01].txt
+square_bracket_alpha/excluded[a-z].txt
+excluded_dir/
+nested_double_asterisk/**/also_exclude.txt
+nested_wildcard_dir/*day/also_exclude.txt

tests/gitignore_test

@@ -0,0 +1,11 @@
+# This test should upload only include.txt, included.log, and included/included.l$
+**/double_asterisk_excluded
+**/double_asterisk_excluded_dir
+**/parent/*.txt
+**/parent/child/*.txt
+*.log
+exp-*/
+!included.log
+excluded.*
+/front_slash_excluded
+no_slash_excluded

tests/test_smoke.py

@@ -25,6 +25,7 @@
 import hashlib
 import inspect
 import pathlib
+import shutil
 import subprocess
 import sys
 import tempfile
@@ -1911,6 +1912,96 @@ class TestStorageWithCredentials:
         'abc_',  # ends with an underscore
     ]
 
+    GITIGNORE_SYNC_TEST_DIR_STRUCTURE = {
+        'double_asterisk': {
+            'double_asterisk_excluded': None,
+            'double_asterisk_excluded_dir': {
+                'dir_excluded': None,
+            },
+        },
+        'double_asterisk_parent': {
+            'parent': {
+                'also_excluded.txt': None,
+                'child': {
+                    'double_asterisk_parent_child_excluded.txt': None,
+                },
+                'double_asterisk_parent_excluded.txt': None,
+            },
+        },
+        'excluded.log': None,
+        'excluded_dir': {
+            'excluded.txt': None,
+            'nested_excluded': {
+                'excluded': None,
+            },
+        },
+        'exp-1': {
+            'be_excluded': None,
+        },
+        'exp-2': {
+            'be_excluded': None,
+        },
+        'front_slash_excluded': None,
+        'included.log': None,
+        'included.txt': None,
+        'include_dir': {
+            'excluded.log': None,
+            'included.log': None,
+        },
+        'nested_double_asterisk': {
+            'one': {
+                'also_exclude.txt': None,
+            },
+            'two': {
+                'also_exclude.txt': None,
+            },
+        },
+        'nested_wildcard_dir': {
+            'monday': {
+                'also_exclude.txt': None,
+            },
+            'tuesday': {
+                'also_exclude.txt': None,
+            },
+        },
+        'no_slash_excluded': None,
+        'no_slash_tests': {
+            'no_slash_excluded': {
+                'also_excluded.txt': None,
+            },
+        },
+        'question_mark': {
+            'excluded1.txt': None,
+            'excluded@.txt': None,
+        },
+        'square_bracket': {
+            'excluded1.txt': None,
+        },
+        'square_bracket_alpha': {
+            'excludedz.txt': None,
+        },
+        'square_bracket_excla': {
+            'excluded2.txt': None,
+            'excluded@.txt': None,
+        },
+        'square_bracket_single': {
+            'excluded0.txt': None,
+        },
+    }
+
+    @staticmethod
+    def create_dir_structure(base_path, structure):
+        for name, substructure in structure.items():
+            path = os.path.join(base_path, name)
+            if substructure is None:
+                # Create a file
+                open(path, 'a').close()
+            else:
+                # Create a subdirectory
+                os.mkdir(path)
+                TestStorageWithCredentials.create_dir_structure(
+                    path, substructure)
+
     @staticmethod
     def cli_delete_cmd(store_type, bucket_name):
         if store_type == storage_lib.StoreType.S3:
@@ -1946,6 +2037,35 @@ def cli_ls_cmd(store_type, bucket_name, suffix=''):
                 url = f's3://{bucket_name}'
             return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3 ls {url} --endpoint {endpoint_url} --profile=r2'
 
+    @staticmethod
+    def cli_count_name_in_bucket(store_type, bucket_name, file_name, suffix=''):
+        if store_type == storage_lib.StoreType.S3:
+            if suffix:
+                return f'aws s3api list-objects --bucket "{bucket_name}" --prefix {suffix} --query "length(Contents[?contains(Key,\'{file_name}\')].Key)"'
+            else:
+                return f'aws s3api list-objects --bucket "{bucket_name}" --query "length(Contents[?contains(Key,\'{file_name}\')].Key)"'
+        elif store_type == storage_lib.StoreType.GCS:
+            if suffix:
+                return f'gsutil ls gs://{bucket_name}/{suffix} | grep "{file_name}" | wc -l'
+            else:
+                return f'gsutil ls gs://{bucket_name} | grep "{file_name}" | wc -l'
+        elif store_type == storage_lib.StoreType.R2:
+            endpoint_url = cloudflare.create_endpoint()
+            if suffix:
+                return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3api list-objects --bucket "{bucket_name}" --prefix {suffix} --query "length(Contents[?contains(Key,\'{file_name}\')].Key)" --endpoint {endpoint_url} --profile=r2'
+            else:
+                return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3api list-objects --bucket "{bucket_name}" --query "length(Contents[?contains(Key,\'{file_name}\')].Key)" --endpoint {endpoint_url} --profile=r2'
+
+    @staticmethod
+    def cli_count_file_in_bucket(store_type, bucket_name):
+        if store_type == storage_lib.StoreType.S3:
+            return f'aws s3 ls s3://{bucket_name} --recursive | wc -l'
+        elif store_type == storage_lib.StoreType.GCS:
+            return f'gsutil ls -r gs://{bucket_name}/** | wc -l'
+        elif store_type == storage_lib.StoreType.R2:
+            endpoint_url = cloudflare.create_endpoint()
+            return f'AWS_SHARED_CREDENTIALS_FILE={cloudflare.R2_CREDENTIALS_PATH} aws s3 ls s3://{bucket_name} --recursive --endpoint {endpoint_url} --profile=r2 | wc -l'
+
     @pytest.fixture
     def tmp_source(self, tmp_path):
         # Creates a temporary directory with a file in it
@@ -2291,6 +2411,85 @@ def test_invalid_names(self, invalid_name_list, store_type):
                 storage_obj = storage_lib.Storage(name=name)
                 storage_obj.add_store(store_type)
 
+    @pytest.mark.parametrize(
+        'gitignore_structure, store_type',
+        [(GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.S3),
+         (GITIGNORE_SYNC_TEST_DIR_STRUCTURE, storage_lib.StoreType.GCS),
+         pytest.param(GITIGNORE_SYNC_TEST_DIR_STRUCTURE,
+                      storage_lib.StoreType.R2,
+                      marks=pytest.mark.cloudflare)])
+    def test_excluded_file_cloud_storage_upload_copy(self, gitignore_structure,
+                                                     store_type):
+        # tests if files included in .gitignore and .git/info/exclude are
+        # excluded from being transferred to Storage
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            # Create files to be uploaded in the Storage
+            self.create_dir_structure(tmpdir, gitignore_structure)
+
+            # Create files to be excluded from .gitignore and list them in the file
+            skypilot_path = os.path.abspath(sys.argv[0])
+            skypilot_path = os.path.dirname(skypilot_path)
+            temp_path = f'{tmpdir}/.gitignore'
+            file_path = os.path.join(skypilot_path, 'tests/gitignore_test')
+            shutil.copyfile(file_path, temp_path)
+
+            # Create files to be excluded from .git/info/exclude and list them in the file
+            temp_path = f'{tmpdir}/.git/info/'
+            os.makedirs(temp_path)
+            temp_exclude_path = os.path.join(temp_path, 'exclude')
+            file_path = os.path.join(skypilot_path,
+                                     'tests/git_info_exclude_test')
+            shutil.copyfile(file_path, temp_exclude_path)
+
+            # Create sky Storage with the files created
+            timestamp = str(time.time()).replace('.', '')
+            bucket_name = f'sky-test-{timestamp}'
+            store_obj = storage_lib.Storage(name=bucket_name,
+                                            source=tmpdir,
+                                            mode=storage_lib.StorageMode.COPY)
+            store_obj.add_store(store_type)
+
+        upload_file_name = 'included'
+        # Count the number of files with the given file name
+        up_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name=upload_file_name)
+        gitignore_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.gitignore')
+        git_exclude_cmd = self.cli_count_name_in_bucket(store_type, \
+            bucket_name, file_name='.git')
+        cnt_num_file_cmd = self.cli_count_file_in_bucket(
+            store_type, bucket_name)
+
+        up_output = subprocess.check_output(up_cmd, shell=True)
+        gitignore_output = subprocess.check_output(gitignore_cmd, shell=True)
+        git_exclude_output = subprocess.check_output(git_exclude_cmd,
+                                                     shell=True)
+        cnt_output = subprocess.check_output(cnt_num_file_cmd, shell=True)
+
+        if store_type == storage_lib.StoreType.GCS:
+            up_dir_cmd = up_cmd = self.cli_count_name_in_bucket(store_type, \
+                bucket_name, file_name=upload_file_name, suffix='include_dir')
+            up_dir_output = subprocess.check_output(up_dir_cmd, shell=True)
+
+        # GCS ls and aws s3 list-objects outputs in a different way
+        # Only 'included.*' files should exist in the cloud object storage
+        if store_type == storage_lib.StoreType.GCS:
+            assert '2' in up_output.decode('utf-8'), \
+                    f'Filese to be uploaded are not uploaded.: {bucket_name}'
+            assert '1' in up_dir_output.decode('utf-8'), \
+                    f'File to be uploaded in include_dir is not uploaded.: {bucket_name}'
+        else:
+            assert '3' in up_output.decode('utf-8'), \
+                    f'{upload_file_name} is not uploaded.: {bucket_name}'
+        assert '0' in gitignore_output.decode('utf-8'), \
+               '.gitignore file should not be uploaded'
+        assert '0' in git_exclude_output.decode('utf-8'), \
+               '.git directory should not be uploaded'
+        ls_cmd = self.cli_ls_cmd(store_type, bucket_name)
+        assert '3' in cnt_output.decode('utf-8'), \
+               f'Some items listed in .gitignore and .git/info/exclude are not ignored - there must be one item in the bucket. Check with {ls_cmd}'
+
 
 # ---------- Testing YAML Specs ----------
 # Our sky storage requires credentials to check the bucket existance when