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.

Sign up for GitHub

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: Package building platform's /etc/os-release and determine execution_container from the file. #322

Merged
merged 7 commits into from
Mar 12, 2024
Merged

clp-package: Package building platform's /etc/os-release and determine execution_container from the file. #322

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8c85ad
Package building platform's `/etc/os-release` and determine `executio…
junhaoliao Mar 10, 2024
6a92955
Lint code.
junhaoliao Mar 10, 2024
6b4bd88
Remove `os-release` from task `package-tar` sources since its depende…
junhaoliao Mar 11, 2024
4d6d552
Refactor os_release_file_path to private attribute in CLPConfig.
junhaoliao Mar 11, 2024
2068fec
Refactor OS release file parser with dotenv.
junhaoliao Mar 11, 2024
c107dc0
Remove `/etc/os-release` from task `core-submodules` sources in Taskf…
junhaoliao Mar 11, 2024
2deaa20
Refactor `_os_release_file_path`.
junhaoliao Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Taskfile.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ tasks:
sources:
- "{{.BUILD_DIR}}/package.md5"
- "{{.TASKFILE_DIR}}/Taskfile.yml"
- "/etc/os-release"
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
generates:
- "{{.VERSIONED_PACKAGE_NAME}}.tar.gz"

Expand All @@ -78,6 +79,7 @@ tasks:
- task: "clean-package"
- "mkdir -p '{{.OUTPUT_DIR}}'"
- "rsync -a components/package-template/src/ '{{.OUTPUT_DIR}}'"
- "rsync -L /etc/os-release '{{.OUTPUT_DIR}}'/etc/os-release"
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
- "mkdir -p '{{.OUTPUT_DIR}}/lib/python3/site-packages'"
- |-
. "{{.PACKAGE_VENV_DIR}}/bin/activate"
Expand Down Expand Up @@ -120,6 +122,7 @@ tasks:
- "components/clp-py-utils/dist/*.whl"
- "components/job-orchestration/dist/*.whl"
- "components/package-template/src/**/*"
- "/etc/os-release"
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
status:
- "test -f '{{.CHECKSUM_FILE}}'"
- >-
Expand All @@ -141,6 +144,7 @@ tasks:
- "{{.SRC_DIR}}/CMakeLists.txt"
- "{{.SRC_DIR}}/src/**/*"
- "{{.TASKFILE_DIR}}/Taskfile.yml"
- "/etc/os-release"
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
generates:
- "{{.CORE_COMPONENT_BUILD_DIR}}/clg"
- "{{.CORE_COMPONENT_BUILD_DIR}}/clo"
Expand Down Expand Up @@ -247,6 +251,7 @@ tasks:
- "{{.TASKFILE_DIR}}/Taskfile.yml"
- ".gitmodules"
- "tools/scripts/deps-download/**/*"
- "/etc/os-release"
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
status:
- "test -f '{{.CHECKSUM_FILE}}'"
- >-
Expand All @@ -272,6 +277,7 @@ tasks:
sources:
- "{{.TASKFILE_DIR}}/requirements.txt"
- "{{.TASKFILE_DIR}}/Taskfile.yml"
- "/etc/os-release"
status:
- "test -f '{{.CHECKSUM_FILE}}'"
- >-
Expand Down Expand Up @@ -307,6 +313,7 @@ tasks:
- "{{.TASKFILE_DIR}}/requirements.txt"
- "{{.TASKFILE_DIR}}/Taskfile.yml"
- "pyproject.toml"
- "/etc/os-release"
generates:
- "dist/*.whl"

Expand All @@ -332,6 +339,7 @@ tasks:
- "{{.TASKFILE_DIR}}/requirements.txt"
- "{{.TASKFILE_DIR}}/Taskfile.yml"
- "pyproject.toml"
- "/etc/os-release"
status:
- "test -f '{{.CHECKSUM_FILE}}'"
- >-
Expand Down
2 changes: 2 additions & 0 deletions components/clp-package-utils/clp_package_utils/scripts/start_clp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,8 @@ def main(argv):
config_file_path, default_config_file_path, clp_home
)

clp_config.load_execution_container_name()

# Validate and load necessary credentials
if target in (
ALL_TARGET_NAME,
Expand Down
28 changes: 27 additions & 1 deletion components/clp-py-utils/clp_py_utils/clp_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
COMPRESSION_JOBS_TABLE_NAME = "compression_jobs"
COMPRESSION_TASKS_TABLE_NAME = "compression_tasks"

OS_RELEASE_FILE_PATH = pathlib.Path("etc") / "os-release"

CLP_DEFAULT_CREDENTIALS_FILE_PATH = pathlib.Path("etc") / "credentials.yml"
CLP_METADATA_TABLE_PREFIX = "clp_"

Expand Down Expand Up @@ -301,7 +303,8 @@ def validate_logging_level(cls, field):


class CLPConfig(BaseModel):
execution_container: str = "ghcr.io/y-scope/clp/clp-execution-x86-ubuntu-focal:main"
execution_container: typing.Optional[str]
os_release_file_path: pathlib.Path = OS_RELEASE_FILE_PATH
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved

input_logs_directory: pathlib.Path = pathlib.Path("/")

Expand All @@ -322,6 +325,7 @@ class CLPConfig(BaseModel):
logs_directory: pathlib.Path = pathlib.Path("var") / "log"

def make_config_paths_absolute(self, clp_home: pathlib.Path):
self.os_release_file_path = make_config_path_absolute(clp_home, self.os_release_file_path)
self.input_logs_directory = make_config_path_absolute(clp_home, self.input_logs_directory)
self.credentials_file_path = make_config_path_absolute(clp_home, self.credentials_file_path)
self.archive_output.make_config_paths_absolute(clp_home)
Expand Down Expand Up @@ -355,6 +359,27 @@ def validate_logs_dir(self):
except ValueError as ex:
raise ValueError(f"logs_directory is invalid: {ex}")

def load_execution_container_name(self):
if self.execution_container is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for sanity check, have you tested and made sure that the container specified in the clp-config will be properly picked up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I have updated the "Validation" section in the PR description to include the related instructions.

# Accept configured value in clp-config.yml for debug purposes
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
return

with open(self.os_release_file_path) as os_release_file:
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
parsed = {}
for line in os_release_file:
var, val = line.strip().split("=")
Copy link
Contributor

Choose a reason for hiding this comment

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

technically we can add a guard to ensure split does return two items, but since the os release file should have a relatively fixed format, this code should be fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I was thinking about adding try ... except blocks to handle such errors / FileNotFoundError, but since users are not expected to modify this <clp-package>/etc/os-release file, such handlings could be redundant. In any case, users will get notified by the Exception stack.

parsed[var] = val

self.execution_container = "ghcr.io/y-scope/clp/"
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
if "ubuntu" == parsed["ID"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we store parsed["ID"] in a local variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I drafted the code, I thought about extracting those into variables parsed_id, parsed_version_id / parsed_version_codename, and even better we break the for-loop if all info we need has been parsed. However, that might be less clean and less extensible if we (ever) plan to support CentOS / RHEL in the future. That's because VERSION_ID is common across all Linux distributions, but VERSION_CODENAME is specific to Ubuntu / Debian distributions.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add Debian into the if condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about adding Debian and other RHEL distributions here, but that does not seem meaningful until we add (building support and most importantly,) execution container support in tools/docker-images. For now, I think raising an NotImplementedError here is a good notification to users.

self.execution_container += (
f"clp-execution-x86-{parsed['ID']}-{parsed['VERSION_CODENAME']}:main"
)
else:
raise NotImplementedError(
f"Unsupported OS {parsed['ID']} in {OS_RELEASE_FILE_PATH}"
)

def load_database_credentials_from_file(self):
config = read_yaml_config_file(self.credentials_file_path)
if config is None:
Expand Down Expand Up @@ -394,6 +419,7 @@ def dump_to_primitive_dict(self):
d = self.dict()
d["archive_output"] = self.archive_output.dump_to_primitive_dict()
# Turn paths into primitive strings
d["os_release_file_path"] = str(self.os_release_file_path)
d["input_logs_directory"] = str(self.input_logs_directory)
d["credentials_file_path"] = str(self.credentials_file_path)
d["data_directory"] = str(self.data_directory)
Expand Down
Loading