Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clp-package: Add support for running package as root (fixes #500). #464

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

haiqi96
Copy link
Contributor

@haiqi96 haiqi96 commented Jun 27, 2024

Description

When running container as root user, redis, results cache and rabbitmq containers lose the write permission to log and data directories.
This PR adds a temporary fix. We let the container run with their default user and change the ownership of the log directories when the current user is root.
Also adds a start up check for redis

Validation performed

  1. Run clp package with root, confirmed that package can run properly
  2. Run clp package with non-root. Stop and restart the same package as root, confirmed that package can run properly, indicating that ownership of the folders and their contents has been changed successfully

@@ -268,6 +277,14 @@ def start_queue(instance_id: str, clp_config: CLPConfig):
DockerMount(DockerMountType.BIND, queue_logs_dir, rabbitmq_logs_dir),
]
rabbitmq_pid_file_path = pathlib.Path("/") / "tmp" / "rabbitmq.pid"

container_user = f"{os.getuid()}:{os.getgid()}"
Copy link
Contributor Author

@haiqi96 haiqi96 Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can factor this code into a function as well, but it could be a bit tricky to reason about

def set_permission_and_get_user(list_paths: List[str]):
    container_user = f"{os.getuid()}:{os.getgid()}"
    if os.getuid() == 0:
        # Use the default container user and make the directories
        # writable by the default user if the current user is root
        container_user = "mongodb"
        for path in list_paths:
            chown_recursively(path, 999, 999)
    return container_user

kirkrodrigues
kirkrodrigues previously approved these changes Jul 29, 2024
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the PR title, how about:

clp-package: Add support for running package as root (fixes #500).

@kirkrodrigues kirkrodrigues changed the title clp-package: Fix a bug where containers do not have write permission to directories when run as root. clp-package: Add support for running package as root (fixes #500). Jul 29, 2024
@kirkrodrigues kirkrodrigues enabled auto-merge (squash) July 29, 2024 17:16
@kirkrodrigues kirkrodrigues merged commit bca4c27 into y-scope:main Jul 29, 2024
4 checks passed
@haiqi96 haiqi96 deleted the root_fix branch August 8, 2024 19:50
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants