Skip to content

fix: skip unavailable optional appliances and add user_inputs_values fallback #PROESP-2753 - #29

Open
raulluzon wants to merge 18 commits into
mainfrom
fix/uma-installer-patches
Open

fix: skip unavailable optional appliances and add user_inputs_values fallback #PROESP-2753#29
raulluzon wants to merge 18 commits into
mainfrom
fix/uma-installer-patches

Conversation

@raulluzon

@raulluzon raulluzon commented Jun 22, 2026

Copy link
Copy Markdown

Summary

  • installer.py: Bypass the marketplace lookup for Technitium DNS and route-manager-api. Both appliances are absent from the UMA OpenNebula marketplace (6G Sandbox PRODUCTION), so the previous onemarketapp_instantiate calls were failing. They are now hardcoded to False/None with a warning log, preserving all downstream conditional logic that already handles the not-instantiated case.
  • utils/one.py — oneflow_custom_attr_value_by_id: Add a fallback from custom_attrs_values to user_inputs_values (with case-insensitive key lookup). Under OpenNebula 7, OneFlow services store their input values in user_inputs_values rather than custom_attrs_values, causing the existing-service flow to fail when reading toolkit secrets.
  • utils/one.py — onemarketapp_instantiate: Remove onetemplate_chown, oneimage_chown, oneflow_template_chown, and the associated image/template enumeration from the existing-service path. Only oneflow_chown_by_id is required; the removed calls were hitting ownership errors on service instance IDs that do not correspond to template IDs.

Validation

Patches were applied and validated on the UMA OpenNebula frontend (/opt/toolkit-installer/) during a live debugging session:

Test plan

  • Run toolkit-installer in existing-service mode against a live OpenNebula 7 environment where the toolkit service is already instantiated.
  • Confirm installer completes without errors on the Technitium and route-manager-api steps.
  • Confirm installer correctly reads all toolkit secrets via oneflow_custom_attr_value_by_id.
  • Confirm site config is written and pushed correctly.
  • Trigger a TN deployment and verify Jenkins succeeds.

🤖 Generated with Claude Code

@raulluzon @claude
fix: skip unavailable optional appliances and add user_inputs_values …
6b94dff 
…fallback

Technitium DNS and route-manager-api are not present in the UMA OpenNebula
marketplace so the installer was failing trying to fetch them. Bypass the
marketplace lookup for both appliances and hard-code them as skipped.

In oneflow_custom_attr_value_by_id, add a fallback to user_inputs_values
(with case-insensitive key lookup) so the existing-service flow works under
OpenNebula 7 where custom_attrs_values may be empty and values land in
user_inputs_values instead.

In onemarketapp_instantiate, remove onetemplate_chown, oneimage_chown,
oneflow_template_chown, and the image/template enumeration from the
existing-service path. Only oneflow_chown_by_id is needed and the removed
calls were causing ownership failures on service instance IDs.

Validated during the UMA deployment session (TN lmql, Jenkins build #5).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon
raulluzon requested a review from rmacian June 22, 2026 09:27
@raulluzon raulluzon changed the title fix: skip unavailable optional appliances and add user_inputs_values fallback fix: skip unavailable optional appliances and add user_inputs_values fallback #PROESP-2753 Jun 22, 2026
@raulluzon @claude
fix: remove unused import and dead code left by previous patch
f5ce414 
Remove onevm_user_input_by_id import from installer.py (unused after the
route-manager-api marketplace call was removed) and remove the dead
template/image enumeration loop in onemarketapp_instantiate VM path
(unreachable after onetemplate_chown and oneimage_chown were removed).

Fixes ruff F401 and F841 CI failures on PR #29.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rmacian
rmacian requested a review from Copilot June 22, 2026 09:42
Copilot AI reviewed Jun 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

raulluzon and others added 3 commits June 22, 2026 12:26
@raulluzon @claude
fix: remove spurious onemarketapp_add call in existing-service path
0dd814f 
When the user answers yes to "Do you have the appliance instantiated?",
the code correctly chowns the existing service but then unconditionally
calls onemarketapp_add before setting is_instantiated=True. This causes
an export attempt that fails with "NAME is already taken" when the toolkit
service template was previously exported. The call serves no purpose in
this path and must only exist in the else branch (new installation).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix: pass validate=True (capital T) and deployment_site_token to TNLC…
58881b0 
…M create endpoint

TNLCM reqparse defines choices=["True", "False"]; sending lowercase "true"
caused BadRequest which was wrapped as 500. Also removed the explicit
Content-Type header that overrides curl's auto-generated multipart boundary,
and added the required deployment_site_token field (validate=True requires all
three: sites_branch, deployment_site, deployment_site_token).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix: derive MinIO endpoint scheme from ONEAPP_MINIO_TLS_ENABLED servi…
bca8609 
…ce attribute

The endpoint was hardcoded as https:// regardless of whether MinIO TLS is
enabled in the toolkit service. Read ONEAPP_MINIO_TLS_ENABLED via
oneflow_custom_attr_value_by_id and use http:// when TLS is disabled (NO,
FALSE, 0) and https:// otherwise.

Also add TOOLKIT_SERVICE_MINIO_TLS_ENABLED to .env so the attribute key name
is configurable alongside the other toolkit service keys.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rmacian

rmacian commented Jun 22, 2026

Copy link
Copy Markdown

LGTM

raulluzon and others added 13 commits June 23, 2026 12:34
@raulluzon @claude
fix: add missing TOOLKIT_SERVICE_MINIO_TLS_ENABLED to .env and abort …
723ef9c 
…on missing env var

Add TOOLKIT_SERVICE_MINIO_TLS_ENABLED to .env so users upgrading from
pre-bca8609 deployments have the required variable. Also make
get_dotenv_var raise SystemExit explicitly after logging the error so
static analysis can confirm the function never returns None.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix: tolerate missing ONEAPP_MINIO_TLS_ENABLED on older appliance ver…
735bbc0 
…sions

Appliances released before the TLS attribute was added (e.g. 20250915-1622-v1.0.0)
do not expose ONEAPP_MINIO_TLS_ENABLED in custom_attrs_values or user_inputs_values.
Add optional=True support to oneflow_custom_attr_value_by_id so the caller receives
None instead of a hard abort; the existing http/https derivation already handles None
by defaulting to http.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix: default minio_scheme to https when TLS attribute is absent
f81634d 
The MinIO appliance script defaults ONEAPP_MINIO_TLS_ENABLED to YES, so
an older appliance without the attribute in its service template still
runs MinIO over HTTPS. Writing http:// to core.yaml caused the Ansible
s3_bucket/s3_object tasks to send plaintext HTTP to an HTTPS endpoint,
producing a 400 Bad Request with empty headers.

Flip the conditional so None (attribute absent) maps to https, matching
the appliance script default. Only an explicit NO/FALSE/0 maps to http.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
feat: create MinIO S3 bucket automatically during installation
fa3aa9a 
The toolkit installer now creates the trial-networks bucket in MinIO
immediately after the endpoint is established, so users never have to
create it manually before deploying Trial Networks.

Introduces utils/s3.py with s3_ensure_bucket (boto3, path-style,
verify=False for self-signed certs). MinIO root credentials are read
from the OneFlow service custom attributes via two new .env keys:
TOOLKIT_SERVICE_MINIO_ROOT_USER and TOOLKIT_SERVICE_MINIO_ROOT_PASSWORD.
Adds boto3 to the project dependencies.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix(lint): sort import blocks in installer.py and utils/s3.py
7e64385 
Resolves ruff I001 violations: reorders utils.questionary before utils.s3
in installer.py, and boto3 before urllib3 in utils/s3.py.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix(one): compatibility fixes for OpenNebula 7.0 API changes
aaf5b76 
- Handle renamed custom_attrs -> user_inputs in OneFlow service templates
- Handle renamed vm_template -> template_id in role definitions
- Resolve appliance and datastore to numeric IDs in onemarketapp export to avoid Ruby nil error
- Guard against missing IMAGE/VMTEMPLATE sections in export output
- Skip re-export when matching service template already exists in OneFlow

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix(one): skip stale admin entries in check_group_admin
a568208 
oneusername() calls sys.exit(1) when the user ID does not exist in
OpenNebula (e.g. a deleted user still listed as group admin). Replace
it with oneuser_show() in check_group_admin, which returns None for
missing users, so stale references are skipped instead of crashing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix(one): normalize single-user pool to list in oneusernames
09fb0e9 
OpenNebula returns USER_POOL["USER"] as a plain dict when there is
exactly one user, instead of a list of dicts. Iterating over a dict
yields string keys, causing "NAME key not found in user". Wrap the
dict in a list before iteration, consistent with the same pattern
already applied to HOST_POOL at line 1748.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@raulluzon @claude
fix(one): normalize single-item pools to list before iteration
c7d4bd6 
OpenNebula XML-to-JSON converts a pool with one item to a plain dict
instead of a list. Iterating over a dict yields string keys, triggering
"NAME key not found" errors. Wrap GROUP, IMAGE, MARKETPLACE, VMTEMPLATE,
VM, and VNET pools in a list when they are a dict, consistent with the
existing HOST_POOL normalization at line 1748.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rubenlocru
Add ask_select function for user choice selection
5ac952c
@rubenlocru
Refactor ask_select function in questionary.py
35edafd 
Refactor ask_select function to simplify parameters and improve compatibility.
@rubenlocru
Add ask_select function for user choice selection
d89ff00 
Added a new function 'ask_select' to prompt user for a choice from a list, with optional default and validation.
@rubenlocru
Refactor questionary.select to select in ask method
e425b0d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@rmacian rmacian rmacian approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@raulluzon @rmacian @rubenlocru