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.

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

Prepare executor VMs #26

Merged
merged 13 commits into from
Jun 20, 2024
Merged

Prepare executor VMs #26

merged 13 commits into from
Jun 20, 2024

Conversation

nesitor
Copy link
Member

@nesitor nesitor commented Jan 11, 2024

Problem: Launch executor VMS the first time, take some time for new executor functions.

Feature: Added a preload script of the executor function to candidate CRNs, and if the CRN doesn't respond, add it to unauthorized_node_list file.

@nesitor nesitor self-assigned this Jan 11, 2024
Copy link
Member

@MHHukiewitz MHHukiewitz left a comment

Choose a reason for hiding this comment

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

LGTM

deployment/deploy_vrf_vms.py Outdated Show resolved Hide resolved
Copy link
Member

@MHHukiewitz MHHukiewitz left a comment

Choose a reason for hiding this comment

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

LGTM

@MHHukiewitz
Copy link
Member

@hoh approve plz

Copy link
Member

@hoh hoh left a comment

Choose a reason for hiding this comment

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

I have a few questions regarding the logic here:

  1. Why use an unauthorized_list of nodes that did not respond instead of an authorized_list of nodes that did respond ?
  2. Can you explain the motivation to hardcode this list in the function squashfs ? I assume that it is for transparency reasons, but that should be explained somewhere else we'll never remember in a few months...
  3. I don't see any docstring on the new functions to explain what we use them for and what they do on a high level. Can you add this ?

Apart from these 3 questions, I made some suggestions to improve the quality of the code with suggestions that you should just be able to apply on the code.

deployment/deploy_vrf_vms.py Show resolved Hide resolved
deployment/prepare_vrf_vms.py Outdated Show resolved Hide resolved
deployment/prepare_vrf_vms.py Outdated Show resolved Hide resolved
deployment/prepare_vrf_vms.py Outdated Show resolved Hide resolved
deployment/prepare_vrf_vms.py Outdated Show resolved Hide resolved
src/aleph_vrf/coordinator/executor_selection.py Outdated Show resolved Hide resolved
src/aleph_vrf/coordinator/vrf.py Outdated Show resolved Hide resolved
@nesitor
Copy link
Member Author

nesitor commented Jan 23, 2024

I have a few questions regarding the logic here:

  1. Why use an unauthorized_list of nodes that did not respond instead of an authorized_list of nodes that did respond ?
  2. Can you explain the motivation to hardcode this list in the function squashfs ? I assume that it is for transparency reasons, but that should be explained somewhere else we'll never remember in a few months...
  3. I don't see any docstring on the new functions to explain what we use them for and what they do on a high level. Can you add this ?

Apart from these 3 questions, I made some suggestions to improve the quality of the code with suggestions that you should just be able to apply on the code.

1.- Because if we use an authorized_list of nodes, if the users add new CRNs to the network, we will just limit to the nodes added, instead to also use the new ones.

2.- You need to hardcode the list to use it inside of the function. Yes, you can load the list as a volume also, but this should be done as another future, and also ensure to publish the list independently, that will complicate the deployment.

3.- Indeed I don't added so much important methods, and all are used inside of the logic. The important external methods are already documented.

Andres D. Molins and others added 4 commits January 24, 2024 12:30
…xecutor functions.

Feature: Added a preload script of the executor function to candidate CRNs, and if the CRN doesn't respond, add it to unauthorized_node_list file.
@nesitor nesitor force-pushed the andres-feature-preload_executor_vms branch from 23b120e to 05e6938 Compare January 24, 2024 11:32
@MHHukiewitz
Copy link
Member

Could this help with some of the VRF problems we encountered recently @nesitor ?

@nesitor nesitor closed this Jun 19, 2024
@nesitor nesitor deleted the andres-feature-preload_executor_vms branch June 19, 2024 15:27
@nesitor nesitor restored the andres-feature-preload_executor_vms branch June 19, 2024 15:28
@nesitor nesitor reopened this Jun 19, 2024
@nesitor nesitor merged commit 3a8b592 into main Jun 20, 2024
3 checks passed
@nesitor nesitor deleted the andres-feature-preload_executor_vms branch June 20, 2024 13:16
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.

3 participants