fix: fix added for moneo install path#59
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the Moneo installation handling so it continues to reside under the Azure tools directory variable while adding a backward-compatible symlink from the legacy /opt/azurehpc/tools path when needed, ensuring configure_service.sh keeps working with the new directory layout. Sequence diagram for Moneo installation and legacy symlink handlingsequenceDiagram
participant AnsibleTask as Ansible_play
participant Vars as Ansible_vars
participant FS as Filesystem
participant MoneoService as configure_service_sh
AnsibleTask->>Vars: Read __hpc_install_prefix
AnsibleTask->>Vars: Resolve __hpc_azure_resource_dir
AnsibleTask->>Vars: Resolve __hpc_azure_tools_dir
AnsibleTask->>Vars: Resolve __hpc_moneo_install_dir from __hpc_azure_tools_dir
AnsibleTask->>FS: Ensure Moneo is installed at __hpc_moneo_install_dir/Moneo
alt __hpc_moneo_install_dir != /opt/azurehpc/tools
AnsibleTask->>FS: Create directory /opt/azurehpc/tools
AnsibleTask->>FS: Create symlink /opt/azurehpc/tools/Moneo -> __hpc_moneo_install_dir/Moneo
else __hpc_moneo_install_dir == /opt/azurehpc/tools
AnsibleTask-->>FS: Skip legacy symlink creation
end
AnsibleTask->>MoneoService: Execute __hpc_moneo_install_dir/Moneo/linux_service/configure_service_sh
MoneoService-->>FS: Use resolved Moneo path (direct or via symlink)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The legacy Moneo path
/opt/azurehpc/toolsis hardcoded in both thewhencondition anddest; consider defining a dedicated variable for the legacy path and using it consistently so future path changes only need to be updated in one place. - The current symlink creation does not specify behavior if
/opt/azurehpc/tools/Moneoalready exists as a directory or incorrect link; consider explicitly handling that case (e.g., withforceor a guard) to ensure idempotent and predictable behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The legacy Moneo path `/opt/azurehpc/tools` is hardcoded in both the `when` condition and `dest`; consider defining a dedicated variable for the legacy path and using it consistently so future path changes only need to be updated in one place.
- The current symlink creation does not specify behavior if `/opt/azurehpc/tools/Moneo` already exists as a directory or incorrect link; consider explicitly handling that case (e.g., with `force` or a guard) to ensure idempotent and predictable behavior.
## Individual Comments
### Comment 1
<location> `tasks/main.yml:841-845` </location>
<code_context>
+ owner: root
+ group: root
+
+ - name: Create symlink from legacy path to actual installation
+ file:
+ src: "{{ __hpc_moneo_install_dir }}/Moneo"
+ dest: /opt/azurehpc/tools/Moneo
+ state: link
- name: Configure Moneo service
</code_context>
<issue_to_address>
**suggestion:** Clarify behavior when /opt/azurehpc/tools/Moneo already exists (e.g., force or fail-fast)
If that path already exists (as a dir or different link), this task can fail or leave an inconsistent state. Consider either adding `force: yes` to always replace it, or gating the task with a `when`/`creates`-style check to preserve an existing install. This will keep reruns and upgrades predictable.
```suggestion
- name: Create symlink from legacy path to actual installation
file:
src: "{{ __hpc_moneo_install_dir }}/Moneo"
dest: /opt/azurehpc/tools/Moneo
state: link
force: true
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ggoklani
changed the title
vars/main.yml
Outdated
| __hpc_azure_resource_dir: "{{ __hpc_install_prefix }}/hpc/azure" | ||
| __hpc_azure_tools_dir: "{{ __hpc_azure_resource_dir }}/tools" | ||
| __hpc_azure_tests_dir: "{{ __hpc_azure_resource_dir }}/tests" | ||
| __hpc_moneo_install_dir: "{{ __hpc_azure_tools_dir }}" |
There was a problem hiding this comment.
Why do you need to have two variables with the same value? __hpc_azure_tools_dir and __hpc_moneo_install_dir. Why not just use __hpc_moneo_install_dir?
There was a problem hiding this comment.
__hpc_azure_tools_dir is generic for all the tools which will be installed.
__hpc_moneo_install_dir: is specifically for monoe it path will be "/opt/hpc/azure/tools/moneo"
There was a problem hiding this comment.
Currently their values are the same. I'd say in this case the code should use the original __hpc_azure_tools_dir variable for moneo.
__hpc_moneo_install_dir: is specifically for monoe it path will be "/opt/hpc/azure/tools/moneo"
Did you want to do this?
__hpc_moneo_install_dir: "{{ __hpc_azure_tools_dir }}/moneo"
There was a problem hiding this comment.
it is auto creating moneo directory instead tools.
- name: Copy Moneo files to install directory
copy:
src: "{{ __hpc_pkg_extracted.path }}/"
remote_src: true
dest: "{{ __hpc_moneo_install_dir }}/Moneo"
mode: '0755'
owner: root
group: root
ggoklani
force-pushed
the
fix_moneo_path
branch
from
f53a30f to
df7bcf0
Compare
tasks/main.yml
Outdated
| group: root | ||
|
|
||
| - name: Create legacy Moneo symlink for configure_service.sh compatibility | ||
| block: |
There was a problem hiding this comment.
Do not need a block now that there is no condition
tasks/main.yml
Outdated
| block: | ||
| - name: Ensure legacy Moneo parent directory exists | ||
| file: | ||
| path: /opt/azurehpc/tools |
There was a problem hiding this comment.
Can be "{{ __hpc_azure_tools_dir }}"
tasks/main.yml
Outdated
| - name: Create symlink from legacy path to actual installation | ||
| file: | ||
| src: "{{ __hpc_moneo_install_dir }}/Moneo" | ||
| dest: /opt/azurehpc/tools/Moneo |
There was a problem hiding this comment.
| dest: /opt/azurehpc/tools/Moneo | |
| dest: "{{ __hpc_azure_tools_dir }}/Moneo" |
There was a problem hiding this comment.
Changes done, Testing in Progress with new changes
ggoklani
force-pushed
the
fix_moneo_path
branch
from
df7bcf0 to
65213bd
Compare
tasks/main.yml
Outdated
| file: | ||
| src: "{{ __hpc_azure_tools_dir }}/Moneo" | ||
| dest: /opt/azurehpc/tools/Moneo | ||
| state: link |
There was a problem hiding this comment.
I think I already commented on this - we should not be creating this /opt/azurehpc directory or using symlinks to point to the installation directory. If there are hard coded paths in the installed scripts (or the install script) then they need to be converted to a template and have /opt/azurehpc replaced with {{ __hpc_azure_tools_dir }} so they refer directly to the installed location in the installed scripts.
As a last resort, if we have to support /opt/azurehpc, then it needs to be defined by a variable similar to __hpc_azure_resource_dir rather than being hardcoded everywhere that needs it.
There was a problem hiding this comment.
@dgchinner I have added the patch to change the default install location to custom one.. and tested it works well.
please check and suggest if these method works or else we can do
" support /opt/azurehpc, then it needs to be defined by a variable similar to __hpc_azure_resource_dir rather than being hardcoded everywhere that needs it"
ggoklani
force-pushed
the
fix_moneo_path
branch
from
65213bd to
937b1f9
Compare
ggoklani
force-pushed
the
fix_moneo_path
branch
from
937b1f9 to
5adbf6c
Compare
|
Looks good to me now. Thanks! |
3 participants
Enhancement: Change installation path for moneo monitoring tool
Reason:
The structure we want to use for static files follows this template:
{{/opt/hpc//bin/ # one-off binaries and scripts
/opt/hpc//lib/... # resources and libraries
/opt/hpc//tools/... # standalone tools
/opt/hpc//tests/ # test scripts for local/CI testing}}
For runtime files (e.g. configuration files set up by boot services), we will store them in:
/var/hpc//....
These common directories will be defined by the following set of variables:
{{_hpc_resource_dir # /opt/hpc//
_hpc_tools_dir # /opt/hpc//tools/
_hpc_tests_dir # /opt/hpc//tests/
_hpc_runtime_dir # /var/hpc//}}
Result: NA
Issue Tracker Tickets (Jira or BZ if any): https://issues.redhat.com/browse/RHELHPC-140
Summary by Sourcery
Update Moneo installation handling to support the new install path while preserving compatibility with existing service configuration scripts.
Enhancements: