Build: use rclone for sync #9842

stsewd · 2022-12-23T00:46:39Z

Put this new feature under a feature flag.
Works out of the box with our current settings, no rclone configuration file required.
Uses the local filesystem when running tests, uses minion during dev.
We need to install rclone in our builders for this to work.
I'm using the checks implemented in Build storage: add additional checks for the source dir #9890, that needs to be merged first.
If we want even faster upload times for sphinx, we can merge Remove build-time field to avoid changing all pages on each build readthedocs-sphinx-ext#119,
since right now we are re-uploading all files.

To test this, you need to re-build your docker containers.

stsewd · 2023-01-11T23:42:31Z

readthedocs/storage/rclone.py

+            # TODO: Fail or let the caller decide what to do?
+            check=True,


Not sure about this one, we could manually check the exit code or catch the exception

I'm not sure to understand the doubt here. What are the different scenarios and their pros/cons?

If the command exists with a status different than 0 with check=True it will raise an exception, if false (the default), then it won't raise an exception and continue normally (but you can still see check for the exit code manually)

I think we should wrap it into a try/except block and raise a custom exception that we can handle from outside properly.

humitos · 2023-01-12T09:54:59Z

@stsewd

Excellent PR description 💯

We need to install rclone in our builders for this to work.

Do you want to create an issue in -ops and assign it to me? or do you want to modify the Salt recipe yourself?

humitos

I did an initial and quick review and it looks good 💯 ! I have to come back and review some parts more in-depth yet, tho.

What's the plan to understand if it's faster, and how much, than the current approach? I suppose we should dump the current metrics we have in NR somewhere else. Then, enable this feature flag for some of those projects, dump the new data and compare.

readthedocs/projects/tasks/builds.py

readthedocs/storage/rclone.py

humitos · 2023-01-12T10:17:09Z

readthedocs/storage/s3_storage.py

+        provider = "AWS"
+        # If a custom endpoint URL is given and
+        # we are running in DEBUG mode, use minio as provider.
+        if self.endpoint_url and settings.DEBUG:
+            provider = "minio"


Shouldn't this be a setting instead of having this hardcoded here? Like RTD_RCLONE_PROVIDER so we can define different values depending on the environment?

This is already possible by using a different storage backend.

The rclone class should be attached to the storage backend chosen, so I don't think there need for a different setting.

OK, I will ask this in a different way. Why do we need a settings.DEBUG here? I'd like to avoid using this setting in a conditional and be able to define this in a setting from the environment.

Minio is only used during development, doesn't seem to be a reason why we wouldn't use minion during development.

MinIO is fine. My point is about avoid using settings.DEBUG in the code.

readthedocs/builds/storage.py

readthedocs/storage/rclone.py

humitos

This is great! I'm suggesting thinking about changing the approach here and instead of using subprocess.run to execute the command from the host, I'd propose to do it from inside the container instead. That will solve lot of issues regarding symlinks and accessing un-authorized files from the host where our Python application is running.

readthedocs/storage/rclone.py

humitos · 2023-01-24T15:51:03Z

readthedocs/storage/rclone.py

+            "--",
+            *args,
+        ]
+        env = os.environ.copy()


Is there any reason why we want to use the same environment than where the process is running? can't we just pass only the variable from self.env_vars only?

When you pass additional env vars, they override ALL the env variables, this means that other env vars like PATH will be undefined.

readthedocs/storage/rclone.py

humitos · 2023-01-24T15:54:32Z

readthedocs/storage/rclone.py

+        env.update(self.env_vars)
+        log.info("Executing rclone command.", command=command)
+        log.debug("env", env=env)
+        result = subprocess.run(


subprocess.run will execute the command from the host machine. Wouldn't it be better to execute the command from inside the container instead? That will give us a lot more security, and reduce the attack vector since users won't be able to access any file from the host at all.

I think it's possible since we have the files to be uploaded in the host, and we only need to pass the correct environment variables only to that particular command.

We discussed this with Eric some weeks ago, to make it secure it should be run from another container, otherwise the user could manipulate the executable to expose the secret env vars we pass to it. I'll be +1 on exploring that idea later.

Good point! 💯 We should have some integrity checking before executing or similar. Sounds good to explore in a future iteration 👍🏼

humitos · 2023-01-24T15:55:42Z

readthedocs/storage/rclone.py

+            # TODO: Fail or let the caller decide what to do?
+            check=True,


I think we should wrap it into a try/except block and raise a custom exception that we can handle from outside properly.

readthedocs/storage/rclone.py

stsewd force-pushed the rclone branch from 1ccc43b to edc9797 Compare January 10, 2023 00:15

Build: use rclone for sync

05d44e3

stsewd force-pushed the rclone branch from edc9797 to 05d44e3 Compare January 10, 2023 00:31

Force to always use checksum

3427b61

stsewd force-pushed the rclone branch 6 times, most recently from 67fb57d to 4382b7c Compare January 11, 2023 23:24

Tests

9d29550

stsewd force-pushed the rclone branch from 4382b7c to 9d29550 Compare January 11, 2023 23:35

stsewd marked this pull request as ready for review January 11, 2023 23:39

stsewd requested a review from a team as a code owner January 11, 2023 23:39

stsewd requested a review from humitos January 11, 2023 23:39

auto-assign bot assigned stsewd Jan 11, 2023

stsewd commented Jan 11, 2023

View reviewed changes

humitos reviewed Jan 12, 2023

View reviewed changes

Updates from review

6102976

humitos mentioned this pull request Jan 23, 2023

Build: handle symlinks in a centralized way #9800

Closed

humitos approved these changes Jan 24, 2023

View reviewed changes

stsewd added 3 commits January 24, 2023 14:40

Updates from review

3580d4d

Merge branch 'main' into rclone

8d81c89

Make use of checks from #9890

3c47422

stsewd merged commit c48e5eb into main Jan 25, 2023

stsewd deleted the rclone branch January 25, 2023 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: use rclone for sync #9842

Build: use rclone for sync #9842

stsewd commented Dec 23, 2022 •

edited

Loading

stsewd Jan 11, 2023

humitos Jan 12, 2023

stsewd Jan 12, 2023

humitos Jan 24, 2023

humitos commented Jan 12, 2023

humitos left a comment

humitos Jan 12, 2023

stsewd Jan 12, 2023

stsewd Jan 12, 2023

humitos Jan 24, 2023

stsewd Jan 24, 2023

humitos Jan 24, 2023

humitos left a comment

humitos Jan 24, 2023

stsewd Jan 24, 2023

humitos Jan 24, 2023

stsewd Jan 24, 2023

humitos Jan 24, 2023 •

edited

Loading

humitos Jan 24, 2023

		# TODO: Fail or let the caller decide what to do?
		check=True,

Build: use rclone for sync #9842

Build: use rclone for sync #9842

Conversation

stsewd commented Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Jan 12, 2023

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Dec 23, 2022 •

edited

Loading

humitos Jan 24, 2023 •

edited

Loading