-
Notifications
You must be signed in to change notification settings - Fork 180
Fix argument list too long and Porter installation document handling for parameter sets #4904
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
f7094ef
53f315f
029877a
2ee02db
12e741d
2a6b3c4
e7c1eed
f6aeecf
62afd6b
14f86ee
7543bdb
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 |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| import json | ||
| import base64 | ||
| import logging | ||
| import tempfile | ||
| from urllib.parse import urlparse | ||
|
|
||
| from shared.logging import logger, shell_output_logger | ||
|
|
@@ -78,7 +79,7 @@ def azure_acr_login_command(config): | |
|
|
||
| async def build_porter_command(config, msg_body, custom_action=False): | ||
| porter_parameter_keys = await get_porter_parameter_keys(config, msg_body) | ||
| porter_parameters = [] | ||
| param_set_entries = [] | ||
|
|
||
| if porter_parameter_keys is None: | ||
| logger.warning("Unknown porter parameters - explain probably failed.") | ||
|
|
@@ -117,9 +118,34 @@ async def build_porter_command(config, msg_body, custom_action=False): | |
| val_base64_bytes = base64.b64encode(val_bytes) | ||
| parameter_value = val_base64_bytes.decode("ascii") | ||
|
|
||
| porter_parameters.extend(["--param", f"{parameter_name}={parameter_value}"]) | ||
| param_set_entries.append({ | ||
| "name": parameter_name, | ||
| "source": {"value": str(parameter_value)} | ||
| }) | ||
|
|
||
| installation_id = msg_body['id'] | ||
| param_set_name = f"tre-params-{installation_id}" | ||
|
|
||
| # Write parameters to a temporary parameter set file to avoid ARG_MAX / MAX_ARG_STRLEN limits | ||
|
Member
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. Don't need these comments.
Contributor
Author
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. Removed in commit |
||
| # when many workspaces are deployed and parameter values (e.g. base64-encoded rule_collections) | ||
| # exceed the Linux execve limits. Use tempfile for secure file creation (mode 0o600, no | ||
| # predictable path) then apply the set to Porter's store via `porter parameters apply`. | ||
| param_set_file = None | ||
| if param_set_entries: | ||
| param_set = { | ||
| "schemaType": "ParameterSet", | ||
| "schemaVersion": "1.0.1", | ||
| "name": param_set_name, | ||
| "namespace": "", | ||
| "parameters": param_set_entries | ||
| } | ||
| with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as f: | ||
| json.dump(param_set, f) | ||
| param_set_file = f.name | ||
|
|
||
| commands = [] | ||
| if param_set_file: | ||
| commands.append(["porter", "parameters", "apply", param_set_file]) | ||
|
|
||
| command = ["porter"] | ||
| if custom_action: | ||
|
|
@@ -131,15 +157,18 @@ async def build_porter_command(config, msg_body, custom_action=False): | |
| "--reference", | ||
| f"{config['registry_server']}/{msg_body['name']}:v{msg_body['version']}" | ||
| ]) | ||
| command.extend(porter_parameters) | ||
| if param_set_file: | ||
| command.extend(["--parameter-set", param_set_name]) | ||
| command.append("--force") | ||
| command.extend(["--credential-set", "arm_auth"]) | ||
| command.extend(["--credential-set", "aad_auth"]) | ||
|
|
||
| if msg_body['action'] == 'upgrade': | ||
| command.append("--force-upgrade") | ||
|
|
||
| return [command] | ||
| commands.append(command) | ||
|
|
||
| return (commands, param_set_file, param_set_name) | ||
|
|
||
|
|
||
| async def build_porter_command_for_outputs(msg_body): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| from multiprocessing import Process | ||
| import json | ||
| import asyncio | ||
| import os | ||
| import sys | ||
| from helpers.commands import azure_acr_login_command, azure_login_command, build_porter_command, build_porter_command_for_outputs, apply_porter_credentials_sets_command, run_command_helper | ||
| from shared.config import get_config | ||
|
|
@@ -161,6 +162,15 @@ def service_bus_message_generator(sb_message: dict, status: str, deployment_mess | |
| return resource_request_message | ||
|
|
||
|
|
||
| async def _cleanup_param_set(param_set_name: str, param_set_file: str, config: dict): | ||
| """Remove a Porter parameter set from its local store and delete the temp file.""" | ||
| await run_command_helper(["porter", "parameters", "delete", param_set_name], config, "Delete parameter set") | ||
| try: | ||
|
marrobi marked this conversation as resolved.
Outdated
|
||
| os.unlink(param_set_file) | ||
| except OSError: | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
|
||
| pass | ||
|
|
||
|
|
||
| async def invoke_porter_action(msg_body: dict, sb_client: ServiceBusClient, config: dict) -> bool: | ||
| """ | ||
| Handle resource message by invoking specified porter action (i.e. install, uninstall) | ||
|
|
@@ -178,10 +188,15 @@ async def invoke_porter_action(msg_body: dict, sb_client: ServiceBusClient, conf | |
|
|
||
| # Build and run porter command (flagging if its a built-in action or custom so we can adapt porter command appropriately) | ||
| is_custom_action = action not in ["install", "upgrade", "uninstall"] | ||
| porter_command = await build_porter_command(config, msg_body, is_custom_action) | ||
| porter_command, param_set_file, param_set_name = await build_porter_command(config, msg_body, is_custom_action) | ||
|
|
||
| logger.debug("Starting to run porter execution command...") | ||
| returncode, _, err = await run_porter(porter_command, config) | ||
| try: | ||
| returncode, _, err = await run_porter(porter_command, config) | ||
| finally: | ||
| # Clean up the temporary parameter set file and remove it from Porter's store | ||
| if param_set_file: | ||
| await _cleanup_param_set(param_set_name, param_set_file, config) | ||
| logger.debug("Finished running porter execution command.") | ||
|
Comment on lines
195
to
205
|
||
|
|
||
| action_completed_without_error = False | ||
|
|
@@ -203,8 +218,13 @@ async def invoke_porter_action(msg_body: dict, sb_client: ServiceBusClient, conf | |
| if "upgrade" == action and ("could not find installation" in err or "The installation cannot be upgraded, because it is not installed." in err): | ||
| logger.warning("Upgrade failed, attempting install...") | ||
| msg_body['action'] = "install" | ||
| porter_command = await build_porter_command(config, msg_body, False) | ||
| returncode, _, err = await run_porter(porter_command, config) | ||
| porter_command, param_set_file, param_set_name = await build_porter_command(config, msg_body, False) | ||
| try: | ||
| returncode, _, err = await run_porter(porter_command, config) | ||
| finally: | ||
| # Clean up the temporary parameter set file for the fallback install command | ||
|
Member
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. dont need comment
Contributor
Author
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. Removed in commit |
||
| if param_set_file: | ||
| await _cleanup_param_set(param_set_name, param_set_file, config) | ||
| if returncode == 0: | ||
| action_completed_without_error = True | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.