Skip to content

09 30 feat sparkles adds avalanche service#121

Open
skuznetsov-klika wants to merge 11 commits into
mainfrom
09-30-feat_sparkles_adds_avalanche_service
Open

09 30 feat sparkles adds avalanche service#121
skuznetsov-klika wants to merge 11 commits into
mainfrom
09-30-feat_sparkles_adds_avalanche_service

Conversation

@skuznetsov-klika

Copy link
Copy Markdown

No description provided.

@anthonyra

Copy link
Copy Markdown
Contributor

The following installer script has some gems of things we should also implement or check during this installation.

https://raw.githubusercontent.com/ava-labs/avalanche-docs/master/scripts/avalanchego-installer.sh

@@ -0,0 +1,7 @@
# defaults/main.yml

avalanche_metrics_port: 33006

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.

With the newest code update I did, I moved this to the depin.core.metrics role to ensure that we have a centralized location allowing code review and adding new services to be easier.

metrics_port:
presearch: 33001
autonomi: 33002
hychain: 33003
storagechain: 33004

@skuznetsov-klika skuznetsov-klika Nov 26, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

avalanche_cmd: "{{ depin_cmd | default('install') }}"

avalanche_version: "1.10.0"
avalanche_download_url: "https://github.com/ava-labs/avalanchego/releases/download/v{{ avalanche_version }}/avalanchego-linux-amd64-v{{ avalanche_version }}.tar.gz"

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.

With the addition of the depin.core.installer role - this was modified to a list and the name changed to avalanche_download_urls. Wanted to make sure that we didn't need to update how we installed services in more than one location.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Checked. Probably should involve qa engineer to test it from scratch.

Comment on lines +5 to +64
- name: Create program folder
become: true
ansible.builtin.file:
state: directory
owner: root
group: root
mode: '0755'
dest: /opt/avalanche/{{ avalanche_version }}

- name: Verify that the program folder was created
ansible.builtin.stat:
path: /opt/avalanche/{{ avalanche_version }}
register: avalanche_program_folder

- name: Assert that the program folder exists
ansible.builtin.assert:
that:
- avalanche_program_folder.stat.exists
- avalanche_program_folder.stat.isdir

- name: Download AvalancheGo
become: true
ansible.builtin.get_url:
url: "{{ avalanche_download_url }}"
dest: /tmp/avalanchego.tar.gz
mode: '0644'

- name: Extract AvalancheGo
become: true
ansible.builtin.unarchive:
src: /tmp/avalanchego.tar.gz
dest: /opt/avalanche/{{ avalanche_version }}
remote_src: yes
extra_opts:
- --strip-components=1

- name: Ensure avalanchego binary is executable
become: true
ansible.builtin.file:
path: /opt/avalanche/{{ avalanche_version }}/avalanchego
mode: '0755'

- name: Verify that the AvalancheGo binary exists
ansible.builtin.stat:
path: /opt/avalanche/{{ avalanche_version }}/avalanchego
register: avalanchego_binary

- name: Assert that the AvalancheGo binary exists
ansible.builtin.assert:
that:
- avalanchego_binary.stat.exists
- avalanchego_binary.stat.mode | int == 755

- name: Symlink avalanchego binary
become: true
ansible.builtin.file:
src: /opt/avalanche/{{ avalanche_version }}/avalanchego
dest: /usr/local/bin/avalanchego
state: link
force: true

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.

This could be replaced with depin.core.installer. I'm also down to add those asserts to the new role as well if it makes sense. One concern of having too many asserts though is the time it takes to perform and if it hangs/crashes is that the way it should react.

Also should there be asserts that happen during tests or everytime it runs?

@skuznetsov-klika skuznetsov-klika Nov 26, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was implemented for Xai, however we would like to hear your comments on that before we apply the same logic across other PRs with related comments.

Waiting approve on xai with those changes

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.

This is removed with the addition of depin.core.metrics

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

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.

This is removed with the addition of depin.core.metrics

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok

import socket
import time
import os
import time

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.

Should remove the duplicate import os and import time

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

{% endif %}

hostname = socket.gethostname()
service = '{{ _name }}'

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.

Instead of relying on ansible templating here, I opted to us Path.stem based on the file name.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

REGISTRY.unregister(PROCESS_COLLECTOR)
REGISTRY.register(MetricsCollector())
role_name='role_name'
start_http_server({{ vars[role_name + '_metrics_port'] | int }})

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.

This was updated with the depin.core.metrics change

@skuznetsov-klika skuznetsov-klika Nov 26, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was implemented for Xai, however we would like to hear your comments on that before we apply the same logic across other PRs with related comments.

Waiting approve on xai with those changes

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.

If this empty we can remove it

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

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.

This can be removed since the default molecule scenario is meant for services while the libs scenario is meant for depin.libs.roles

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

avalanche reworked

- name: Start Avalanche node
ansible.builtin.include_tasks:
file: commands/start.yml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The relative path in include_tasks needs to be adjusted to ../metrics.yml since the task is being included from within the commands/ subdirectory. The current path tasks/metrics.yml will fail to locate the metrics file.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.


[Service]
Type=simple
User=nerdnode

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The User directive is hardcoded to nerdnode but should use {{ avalanche_user }} to maintain consistency with the role's parameterization pattern. This allows the service to run under the same user as configured in the role's defaults.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants