-
Notifications
You must be signed in to change notification settings - Fork 83
refactor(package): Standardize how configuration and credentials are passed to containers (resolves #1149). #1152
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
Changes from 6 commits
b143237
6cddeda
f9d5b59
489476f
0e3f412
05283d5
34c5e8f
842efac
05715fb
87d07c6
73334ed
866334d
dff9bef
00ceeb8
cc9a7c7
4e1c272
7d82bec
1b7c70c
781bb16
bf63d6e
469bc65
8654a56
03881cc
0c9a035
6f8879c
3850b2d
96735b1
365b5d6
c84836c
04c5c98
33e11ac
6e06740
6ef77dd
36af52d
d85085b
e371aa8
5fa14f8
9eba523
312445e
335f25e
3cc3d44
02f393d
8bcae09
4a9bf99
fc27205
40e387d
ed58dcc
89f001f
95b55ef
fa81a9d
4743566
0a8de41
cbdd9fb
67d048a
f852ca4
0acb960
d21efbf
d446e52
c0949cc
f723512
38c75ee
a11b314
4124b60
3fe8368
ce143c6
e15547e
4bba4ed
6ab2f80
cd4437d
14e0219
33bc068
ae8f581
0e52cb1
f063bd9
04f81b7
34c430f
31342c3
ca2db62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| import enum | ||||||||||||||||||||||||||||||||||||||||||||||
| import errno | ||||||||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||
| import pathlib | ||||||||||||||||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -36,6 +37,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
| from strenum import KebabCaseStrEnum | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| logger = logging.getLogger(__file__) | ||||||||||||||||||||||||||||||||||||||||||||||
haiqi96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| # CONSTANTS | ||||||||||||||||||||||||||||||||||||||||||||||
| EXTRACT_FILE_CMD = "x" | ||||||||||||||||||||||||||||||||||||||||||||||
| EXTRACT_IR_CMD = "i" | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -94,6 +97,7 @@ def __init__(self, clp_home: pathlib.Path, docker_clp_home: pathlib.Path): | |||||||||||||||||||||||||||||||||||||||||||||
| self.archives_output_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||
| self.stream_output_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||
| self.aws_config_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||
| self.generated_config_dir: typing.Optional[DockerMount] = None | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| def get_clp_home(): | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -278,6 +282,19 @@ def generate_container_config( | |||||||||||||||||||||||||||||||||||||||||||||
| clp_config.aws_config_directory, | ||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.aws_config_directory, | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
| if not is_path_already_mounted( | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_home, | ||||||||||||||||||||||||||||||||||||||||||||||
| CONTAINER_CLP_HOME, | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.generated_config_file_path, | ||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.generated_config_file_path, | ||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||
| docker_mounts.generated_config_dir = DockerMount( | ||||||||||||||||||||||||||||||||||||||||||||||
| DockerMountType.BIND, | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.generated_config_file_path, | ||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config.generated_config_file_path, | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
haiqi96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mount the parent directory of the config file, not the file itself. The code attempts to mount Apply this diff to fix the mounting issue: if not is_path_already_mounted(
clp_home,
CONTAINER_CLP_HOME,
- clp_config.generated_config_file_path,
- container_clp_config.generated_config_file_path,
+ clp_config.generated_config_file_path.parent,
+ container_clp_config.generated_config_file_path.parent,
):
docker_mounts.generated_config_dir = DockerMount(
DockerMountType.BIND,
- clp_config.generated_config_file_path,
- container_clp_config.generated_config_file_path,
+ clp_config.generated_config_file_path.parent,
+ container_clp_config.generated_config_file_path.parent,
)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are using --mount which seems to work for file.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @haiqi96 You're right—Docker bind mounts support files with --mount as long as the source exists. No change is required to switch to the parent directory. Two small hardening tweaks you might consider:
Otherwise, keeping the file mount is fine given dump_container_config runs before container start. 🧠 Learnings used |
||||||||||||||||||||||||||||||||||||||||||||||
| return container_clp_config, docker_mounts | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -313,11 +330,15 @@ def dump_container_config( | |||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| def generate_container_start_cmd( | ||||||||||||||||||||||||||||||||||||||||||||||
| container_name: str, container_mounts: List[Optional[DockerMount]], container_image: str | ||||||||||||||||||||||||||||||||||||||||||||||
| container_name: str, | ||||||||||||||||||||||||||||||||||||||||||||||
| extra_env_vars: dict[str, str], | ||||||||||||||||||||||||||||||||||||||||||||||
haiqi96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
haiqi96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| container_mounts: List[Optional[DockerMount]], | ||||||||||||||||||||||||||||||||||||||||||||||
| container_image: str, | ||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| Generates the command to start a container with the given mounts and name. | ||||||||||||||||||||||||||||||||||||||||||||||
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| :param container_name: | ||||||||||||||||||||||||||||||||||||||||||||||
| :param extra_env_vars: | ||||||||||||||||||||||||||||||||||||||||||||||
| :param container_mounts: | ||||||||||||||||||||||||||||||||||||||||||||||
| :param container_image: | ||||||||||||||||||||||||||||||||||||||||||||||
| :return: The command. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -335,6 +356,8 @@ def generate_container_start_cmd( | |||||||||||||||||||||||||||||||||||||||||||||
| "--name", container_name, | ||||||||||||||||||||||||||||||||||||||||||||||
| "--log-driver", "local" | ||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||
| for key, value in extra_env_vars.items(): | ||||||||||||||||||||||||||||||||||||||||||||||
| container_start_cmd.extend(["-e", f"{key}={value}"]) | ||||||||||||||||||||||||||||||||||||||||||||||
| for mount in container_mounts: | ||||||||||||||||||||||||||||||||||||||||||||||
| if mount: | ||||||||||||||||||||||||||||||||||||||||||||||
| container_start_cmd.append("--mount") | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -377,6 +400,11 @@ def load_config_file( | |||||||||||||||||||||||||||||||||||||||||||||
| hostname = socket.gethostname() | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.data_directory /= hostname | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.logs_directory /= hostname | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.generated_config_file_path = ( | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_config.generated_config_file_path.parent | ||||||||||||||||||||||||||||||||||||||||||||||
| / hostname | ||||||||||||||||||||||||||||||||||||||||||||||
| / clp_config.generated_config_file_path.name | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| return clp_config | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -590,3 +618,59 @@ def validate_dataset_name(clp_table_prefix: str, dataset_name: str) -> None: | |||||||||||||||||||||||||||||||||||||||||||||
| f"Invalid dataset name: `{dataset_name}`. Names can only be a maximum of" | ||||||||||||||||||||||||||||||||||||||||||||||
| f" {dataset_name_max_len} characters long." | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| def generate_common_environment_variables( | ||||||||||||||||||||||||||||||||||||||||||||||
kirkrodrigues marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config, | ||||||||||||||||||||||||||||||||||||||||||||||
haiqi96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| include_clp_home=True, | ||||||||||||||||||||||||||||||||||||||||||||||
| include_db_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||
| include_queue_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||
| include_redis_credentials=False, | ||||||||||||||||||||||||||||||||||||||||||||||
| include_celery_connection_params=False, | ||||||||||||||||||||||||||||||||||||||||||||||
| extra_vars=None, | ||||||||||||||||||||||||||||||||||||||||||||||
haiqi96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| ) -> List[str]: | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| Generate a list of common environment variables for Docker containers. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| :param container_clp_config: The CLPConfig object for the container. | ||||||||||||||||||||||||||||||||||||||||||||||
| :param include_clp_home: | ||||||||||||||||||||||||||||||||||||||||||||||
kirkrodrigues marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| :param include_db_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||
| :param include_queue_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||
| :param include_redis_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||
| :param include_celery_connection_params: | ||||||||||||||||||||||||||||||||||||||||||||||
| :param extra_vars: Additional environment variables to include in the format "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||
| :return: List of environment variable strings in the format "KEY=VALUE". | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| clp_site_packages_dir = CONTAINER_CLP_HOME / "lib" / "python3" / "site-packages" | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars = [f"PYTHONPATH={clp_site_packages_dir}"] | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if include_clp_home: | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_HOME={CONTAINER_CLP_HOME}") | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if include_db_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_DB_USER={container_clp_config.database.username}") | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_DB_PASS={container_clp_config.database.password}") | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if include_queue_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_QUEUE_USER={container_clp_config.queue.username}") | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_QUEUE_PASS={container_clp_config.queue.password}") | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if include_redis_credentials: | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append(f"CLP_REDIS_PASS={container_clp_config.redis.password}") | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if include_celery_connection_params: | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append( | ||||||||||||||||||||||||||||||||||||||||||||||
| f"BROKER_URL=amqp://" | ||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.queue.username}:{container_clp_config.queue.password}@" | ||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.queue.host}:{container_clp_config.queue.port}" | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.append( | ||||||||||||||||||||||||||||||||||||||||||||||
| f"RESULT_BACKEND=redis://default:{container_clp_config.redis.password}@" | ||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.redis.host}:{container_clp_config.redis.port}/" | ||||||||||||||||||||||||||||||||||||||||||||||
| f"{container_clp_config.redis.query_backend_database}" | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if extra_vars: | ||||||||||||||||||||||||||||||||||||||||||||||
| env_vars.extend(extra_vars) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| return env_vars | ||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -206,14 +206,17 @@ def main(argv: typing.List[str]) -> int: | |||||
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||||||
| container_clp_config, clp_config, container_name | ||||||
| ) | ||||||
|
|
||||||
| extra_env_vars = { | ||||||
| "CLP_DB_USER": clp_config.database.username, | ||||||
| "CLP_DB_PASS": clp_config.database.password, | ||||||
| } | ||||||
haiqi96 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| necessary_mounts: typing.List[CLPDockerMounts] = [ | ||||||
|
||||||
| necessary_mounts: typing.List[CLPDockerMounts] = [ | |
| necessary_mounts: typing.List[DockerMount] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually have the fix in #1186
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||
| import argparse | ||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||
| import shutil | ||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||
| import typing | ||||||||||||||||||||||||
|
|
@@ -188,6 +189,12 @@ def main(argv: typing.List[str]) -> int: | |||||||||||||||||||||||
| except: | ||||||||||||||||||||||||
| logger.exception("Failed to load config.") | ||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||
| clp_config.database.username = os.environ["CLP_DB_USER"] | ||||||||||||||||||||||||
| clp_config.database.password = os.environ["CLP_DB_PASS"] | ||||||||||||||||||||||||
| except KeyError as e: | ||||||||||||||||||||||||
| logger.error(f"Missing environment variable: {e}") | ||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| try: | |
| clp_config.database.username = os.environ["CLP_DB_USER"] | |
| clp_config.database.password = os.environ["CLP_DB_PASS"] | |
| except KeyError as e: | |
| logger.error(f"Missing environment variable: {e}") | |
| return -1 | |
| try: | |
| clp_config.load_database_credentials_from_env() | |
| except ValueError as err: | |
| logger.error(err) | |
| return -1 |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py
around lines 191 to 196, replace the direct os.environ lookups and KeyError
handling with the standardized CLPConfig.load_database_credentials_from_env()
call to populate clp_config.database (so missing or empty vars are validated
consistently); catch ValueError from that loader instead of KeyError, log the
error message with logger.error(...), and return -1 on failure.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import argparse | ||
| import asyncio | ||
| import logging | ||
| import os | ||
| import pathlib | ||
| import subprocess | ||
| import sys | ||
|
|
@@ -112,11 +113,18 @@ def handle_extract_stream_cmd( | |
| :return: 0 on success, -1 otherwise. | ||
| """ | ||
| # Validate and load config file | ||
| clp_config = validate_and_load_config_file( | ||
| clp_config = load_and_validate_config_file( | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| clp_home, pathlib.Path(parsed_args.config), default_config_file_path | ||
| ) | ||
| if clp_config is None: | ||
| return -1 | ||
| # Override credentials with environment variables | ||
| try: | ||
| clp_config.database.username = os.environ["CLP_DB_USER"] | ||
| clp_config.database.password = os.environ["CLP_DB_PASS"] | ||
| except KeyError as e: | ||
| logger.error(f"Missing environment variable: {e}") | ||
| return -1 | ||
|
||
|
|
||
| command = parsed_args.command | ||
|
|
||
|
|
@@ -222,6 +230,13 @@ def handle_extract_file_cmd( | |
| ) | ||
| if clp_config is None: | ||
| return -1 | ||
| # Override credentials with environment variables | ||
| try: | ||
| clp_config.database.username = os.environ["CLP_DB_USER"] | ||
| clp_config.database.password = os.environ["CLP_DB_PASS"] | ||
| except KeyError as e: | ||
| logger.error(f"Missing environment variable: {e}") | ||
| return -1 | ||
|
|
||
| paths = parsed_args.paths | ||
| list_path = parsed_args.files_from | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.