Repository: skypilot-org/skypilot
Pull request: [R2] Fix bug on credential sync when AWS is disabled
<!-- Describe the changes in this PR -->
This PR resolves #1844 

When AWS is not enabled, `~/.aws/credentials` does not get sync to the remote cluster. As 1 of 2 credentials r2 requires is stored in `~/.aws/credentials` as well, r2 was not functioning when AWS was disabled. Hence, in this PR, it fixes to sync the `~/.aws/credentials` and `~/.cloudflare/accountid` to the remote cluster when r2 is enabled and AWS is disabled.

Note: Currently, Skypilot uses `global_user_state.get_enabled_clouds()` to check what clouds are enabled by the user. However, `get_enabled_clouds()` only checks for the clouds that are supporting computing instances. The approach used in this PR maybe fixed in the future when r2 gets incorporated as part of the `sky/clouds.py` family.

<!-- 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):

- [x] Ran the exact same scenario as the issue and confirmed it works

Problem statement:
R2 storage copy failed with return code 255.
Tried the newly R2 storage support, following setup as described in the documentation got failed error ```failed with return code 255.``` 

tried to run command from the sky.exception.CommandError directly
```
mkdir -p ~/.sky/file_mounts/nli-datasets && aws --version >/dev/null 2>&1 || pip3 install awscli && aws s3 sync --no-follow-symlinks s3://<bucketname>/datasets/nli ~/.sky/file_mounts/nli-datasets --endpoint https:/<cloudflare-account-id>.r2.cloudflarestorage.com --profile=r2
```
and its working fine and the file from r2 cloned into ```~/.sky/file_mounts/nli-datasets``` directory

Versions:
skypilot, version 1.0.0-dev0
aws-cli/1.27.109 Python/3.9.15 Darwin/21.4.0 botocore/1.29.109

Cloud:
Lambdalabs

Hints from issue discussion:
Thanks for the report @yudanta. This is a bug - we'll fix it. As a temporary workaround, if you have an AWS account, enabling [AWS support](https://skypilot.readthedocs.io/en/latest/getting-started/installation.html#aws) should let you use R2 on Lambda cloud.

## Reproduction
I can confirm I can reproduce this bug with the following minimal example run in a docker container (to emulate a fresh skypilot install without aws):

```
# docker run --platform linux/amd64 -it --rm continuumio/miniconda3 /bin/bash
# Install SkyPilot
git clone https://github.com/skypilot-org/skypilot
cd skypilot 
pip install -e .
apt-get update
apt-get install rsync

# Setup lambda cloud
mkdir -p ~/.lambda_cloud
echo "api_key =<key>" > ~/.lambda_cloud/lambda_keys

# Setup R2
aws configure --profile r2
mkdir -p ~/.cloudflare
echo <account_id> > ~/.cloudflare/accountid

# Verify
sky check

# Run task
cat <<EOF >>mytask.yaml
file_mounts:
  /mymnt:
    source: r2://extbucket
    mode: COPY

run: |
  ls /mymnt
EOF
sky launch -c myclus mytask.yaml
```

This fails with:

```
I 04-10 06:27:02 backend_utils.py:1210] Syncing (to 1 node): r2://extbucket -> /mymnt
⠼ SyncingE 04-10 06:27:03 subprocess_utils.py:70]
E 04-10 06:27:03 subprocess_utils.py:70] The config profile (r2) could not be found
E 04-10 06:27:03 subprocess_utils.py:70]
Clusters
NAME           LAUNCHED        RESOURCES                          STATUS  AUTOSTOP  COMMAND
sky-efa5-root  a few secs ago  1x Lambda(gpu_1x_a10, {'A10': 1})  UP      -         sky launch -c sky-efa5-root...

sky.exceptions.CommandError: Command mkdir -p ~/.sky/file_mounts/mymnt && aws --version >/dev/null 2>&1 || pip3 install awscli && aws s3 sync --no-follow-symlinks s3://extbucket ~/.sky/file_mounts/mymnt --endpoint https://ec9997b59e5296acc10fed5cd4945e6e.r2.cloudflarestorage.com --profile=r2 failed with return code 255.
Failed to run command before rsync r2://extbucket -> /mymnt. Ensure that the network is stable, then retry.
```

## Cause
This is happening because `~/.aws/credentials` stores R2 credentials when `aws configure --profile r2` is used. However, it is not synced when AWS is not an enabled cloud. Thus the remote cluster is unable to access the R2 bucket. In the above example, I was able to fix the error by manually scping my `~/.aws/credentials` to the remote cluster.

## Fix
We need to sync `~/.aws/credentials` to remote cluster when R2 is used, even if AWS support may not be setup. Alternatively, we can see if `aws configure --profile r2` can write to another file that we can sync to remote cluster.

cc @landscapepainter 
thanks @romilbhardwaj  for the update, will try to manually copy the ~/.aws/credentials to the remote cluster for the quick workaround.

thanks!

Commit message: fix

Patch to review:
diff --git a/sky/adaptors/cloudflare.py b/sky/adaptors/cloudflare.py
index 145665d701b..960a075f5a1 100644
--- a/sky/adaptors/cloudflare.py
+++ b/sky/adaptors/cloudflare.py
@@ -5,11 +5,13 @@
 import functools
 import threading
 import os
+from typing import Dict, Optional, Tuple
 
 boto3 = None
 botocore = None
 _session_creation_lock = threading.RLock()
 ACCOUNT_ID_PATH = '~/.cloudflare/accountid'
+AWS_R2_PROFILE_PATH = '~/.aws/credentials'
 R2_PROFILE_NAME = 'r2'
 
 
@@ -119,3 +121,52 @@ def create_endpoint():
     endpoint = 'https://' + accountid + '.r2.cloudflarestorage.com'
 
     return endpoint
+
+
+def check_credentials() -> Tuple[bool, Optional[str]]:
+    """Checks if the user has access credentials to this cloud."""
+
+    hints = None
+    accountid_path = os.path.expanduser(ACCOUNT_ID_PATH)
+    if not os.path.exists(accountid_path):
+        hints = 'Account ID from R2 dashboard is not set.'
+    if not r2_profile_in_aws_cred():
+        if hints:
+            hints += ' And '
+        else:
+            hints = ''
+        hints += '[r2] profile is not set in ~/.aws/credentials.'
+    if hints:
+        hints += (
+            '\n      Please follow the instructions in:'
+            '\n      https://skypilot.readthedocs.io/en/latest/getting-started/installation.html#cloudflare-r2'
+        )
+    return (False, hints) if hints else (True, hints)
+
+
+def r2_profile_in_aws_cred() -> bool:
+    """Checks if Cloudflare R2 profile is set in aws credentials"""
+
+    profile_path = os.path.expanduser(AWS_R2_PROFILE_PATH)
+    r2_profile_exists = False
+    if os.path.isfile(profile_path):
+        with open(profile_path, 'r') as file:
+            for line in file:
+                if '[r2]' in line:
+                    r2_profile_exists = True
+    return r2_profile_exists
+
+
+def get_credential_file_mounts() -> Dict[str, str]:
+    """Checks if aws credential file is set and update if not
+       Updates file containing account ID information
+
+    Args:
+        file_mounts: stores path to credential files of clouds
+    """
+
+    r2_credential_mounts = {
+        AWS_R2_PROFILE_PATH: AWS_R2_PROFILE_PATH,
+        ACCOUNT_ID_PATH: ACCOUNT_ID_PATH
+    }
+    return r2_credential_mounts
diff --git a/sky/check.py b/sky/check.py
index 1a359fa7525..463cde9c9fb 100644
--- a/sky/check.py
+++ b/sky/check.py
@@ -5,6 +5,7 @@
 
 from sky import clouds
 from sky import global_user_state
+from sky.adaptors import cloudflare
 
 
 def check(quiet: bool = False) -> None:
@@ -61,4 +62,12 @@ def get_cloud_credential_file_mounts() -> Dict[str, str]:
     for cloud in enabled_clouds:
         cloud_file_mounts = cloud.get_credential_file_mounts()
         file_mounts.update(cloud_file_mounts)
+    # Currently, get_enabled_clouds() does not support r2
+    # as only clouds with computing instances are marked
+    # as enabled by skypilot. This will be removed when
+    # cloudflare/r2 is added as a 'cloud'.
+    r2_is_enabled, _ = cloud = cloudflare.check_credentials()
+    if r2_is_enabled:
+        r2_credential_mounts = cloudflare.get_credential_file_mounts()
+        file_mounts.update(r2_credential_mounts)
     return file_mounts


Code review comments:
diff_hunk: @@ -119,3 +121,52 @@ def create_endpoint():
     endpoint = 'https://' + accountid + '.r2.cloudflarestorage.com'
 
     return endpoint
+
+
+def check_credentials() -> Tuple[bool, Optional[str]]:
+    """Checks if the user has access credentials to this cloud."""
+
+    hints = None
+    accountid_path = os.path.expanduser(ACCOUNT_ID_PATH)
+    if not os.path.exists(accountid_path):
+        hints = 'Account ID from R2 dashboard is not set.'
+    if not r2_profile_in_aws_cred():
+        if hints:
+            hints += ' And '
+        else:
+            hints = ''
+        hints += '[r2] profile is not set in ~/.aws/credentials.'
+    if hints:
+        hints += (
+            '\n      Please follow the instructions in:'
+            '\n      https://skypilot.readthedocs.io/en/latest/getting-started/installation.html#cloudflare-r2'
+        )
+    return (False, hints) if hints else (True, hints)
+
+
+def r2_profile_in_aws_cred() -> bool:
+    """Checks if Cloudflare R2 profile is set in aws credentials"""
+
+    profile_path = os.path.expanduser(AWS_R2_PROFILE_PATH)
+    r2_profile_exists = False
+    if os.path.isfile(profile_path):
+        with open(profile_path, 'r') as file:
+            for line in file:
+                if '[r2]' in line:
path: sky/adaptors/cloudflare.py
text: @user1:
Can we put this in a constant at the top of the file, such as `R2_AWSCLI_PROFILE_NAME='r2'` and make this line (and L138) `if f'[{R2_AWSCLI_PROFILE_NAME}]' in line:`? That way if someone happens to use a different profile name, it isn't too hard to change.

@author:
Cool! Thanks for the reminder. I actually already had `R2_PROFILE_NAME = 'r2'` at L15, so I'll reuse that.

diff_hunk: @@ -119,3 +121,52 @@ def create_endpoint():
     endpoint = 'https://' + accountid + '.r2.cloudflarestorage.com'
 
     return endpoint
+
+
+def check_credentials() -> Tuple[bool, Optional[str]]:
+    """Checks if the user has access credentials to this cloud."""
+
+    hints = None
+    accountid_path = os.path.expanduser(ACCOUNT_ID_PATH)
+    if not os.path.exists(accountid_path):
+        hints = 'Account ID from R2 dashboard is not set.'
+    if not r2_profile_in_aws_cred():
+        if hints:
+            hints += ' And '
path: sky/adaptors/cloudflare.py
text: @user1:
nit: `and`

@author:
Using `And` to have the first `hints` to be complete by itself with a period `.` 

@user1:
I see, in that case can we use ` Additionally, `