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

feat: dashmate for seeds #535

Merged
merged 13 commits into from
Aug 28, 2023
Merged

feat: dashmate for seeds #535

merged 13 commits into from
Aug 28, 2023

Conversation

pshenmic
Copy link
Collaborator

@pshenmic pshenmic commented Aug 12, 2023

Issue being fixed or feature implemented

#514

What was done?

  • Removed mn_evo_services role
  • Stop unused containers at the end of dashmate role for seed nodes

How Has This Been Tested?

On my own devnet + smoke tests

Breaking Changes

No

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@pshenmic pshenmic marked this pull request as draft August 13, 2023 16:04
@pshenmic pshenmic requested review from shumkov and strophy and removed request for shumkov August 15, 2023 16:58
@pshenmic pshenmic marked this pull request as ready for review August 15, 2023 16:59
@strophy
Copy link
Collaborator

strophy commented Aug 17, 2023

I'll try and deploy this on testnet seed-3 once the 0.24.22 dashmate release is complete.

strophy
strophy previously approved these changes Aug 18, 2023
Copy link
Collaborator

@strophy strophy left a comment

Choose a reason for hiding this comment

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

Code reviewed and approved in concept, waiting for dashmate release for testing on testnet, so please don't merge yet.

@strophy
Copy link
Collaborator

strophy commented Aug 22, 2023

There appears to be an error in the logic on initial startup, where immediately after starting all services, the dashmate role then attempts to restart them, which fails. Ansible output:

TASK [dashmate : Print status] *******************************************************************************************************************************
ok: [seed-1] => 
  msg: |2-
  
     Core Started: False
     Platform Started: False
     Core Images Updated: False
     Platform Images Updated: True
     Dashmate Package Updated: True
     Dashmate Config Changed: True

TASK [dashmate : Start dashmate services] ********************************************************************************************************************
changed: [seed-1]

TASK [dashmate : Start dashmate platform services] ***********************************************************************************************************
skipping: [seed-1]

TASK [dashmate : Restart dashmate services] ******************************************************************************************************************
fatal: [seed-1]: FAILED! => 
  msg: |-
    The conditional check 'dashmate_start_platform.skipped and dashmate_start.skipped' failed. The error was: error while evaluating conditional (dashmate_start_platform.skipped and dashmate_start.skipped): 'dict object' has no attribute 'skipped'. 'dict object' has no attribute 'skipped'
  
    The error appears to be in '/home/strophy/dash-network-deploy/ansible/roles/dashmate/tasks/main.yml': line 239, column 3, but may
    be elsewhere in the file depending on the exact syntax problem.
  
    The offending line appears to be:
  
  
    - name: Restart dashmate services
      ^ here

@strophy
Copy link
Collaborator

strophy commented Aug 22, 2023

This is also not quite idempotent yet, running the dashmate role twice on a seed node will always result in is_dashmate_config_changed being true, and all processes are restarted even though nothing is changed. We need to look at the ansible.builtin.template command on line 113 and figure out why this always reports status changed.

@strophy
Copy link
Collaborator

strophy commented Aug 22, 2023

Other than the above logic issue, this seems to be working fine. It's a bit inefficient to run through the whole ZeroSSL workflow, assign a cert, and then shut down the DAPI container while leaving all the provisioned config and containers in place, but I think changing this would require significant changes to dashmate right? I think we can leave it for now, it's really great to be able to remove the entire mn_evo_services role.

I think we can also remove "convenient tasks" from lines 301-365 in the dashmate role, these tasks seem to be handled properly in dashmate now, right?

@strophy
Copy link
Collaborator

strophy commented Aug 22, 2023

Please add mode: "validator" to EN dashmate invocation in deploy.yml after line 238 to avoid this error:

TASK [dashmate : Write dashmate config file] *******************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: ansible.errors.AnsibleUndefinedVariable: 'mode' is undefined. 'mode' is undefined
fatal: [hp-masternode-1]: FAILED! => changed=false 
  msg: 'AnsibleUndefinedVariable: ''mode'' is undefined. ''mode'' is undefined'

@strophy
Copy link
Collaborator

strophy commented Aug 22, 2023

While trying to update evonodes to the v0.24.23 in order to fix issues related to restarts and log files on testnet, I found that dashmate fails to stop all the containers on evonodes:

dashmate@hp-masternode-1:~$ dashmate stop
✔ Check node is running
✔ Stopping testnet node
dashmate@hp-masternode-1:~$ docker ps
CONTAINER ID   IMAGE                       COMMAND                  CREATED       STATUS      PORTS                                                                                                        NAMES
2db3a4fd2b3b   dashpay/tenderdash:0.11.3   "docker-entrypoint.s…"   2 weeks ago   Up 4 days   26656-26657/tcp, 127.0.0.1:6060->6060/tcp, 127.0.0.1:36657->36657/tcp, 26660/tcp, 0.0.0.0:36656->36656/tcp   dashmate_testnet-drive_tenderdash-1
59a46d4fee19   dashpay/drive:0.24.19       "docker-entrypoint.s…"   2 weeks ago   Up 4 days   26658/tcp                                                                                                    dashmate_testnet-drive_abci-1
c5bb65518a3c   dashpay/dashd:19.3.0        "/docker-entrypoint.…"   2 weeks ago   Up 4 days   9998-9999/tcp, 127.0.0.1:19998->19998/tcp, 0.0.0.0:19999->19999/tcp                                          dashmate_testnet-core-1
dashmate@hp-masternode-1:~$ dashmate --version
dashmate/0.24.23 linux-x64 node-v16.20.1

@pshenmic
Copy link
Collaborator Author

pshenmic commented Aug 22, 2023

dashmate_start_platform.skipped and dashmate_start.skipped

Weirdly, I remember fixing this already in this branch 🤔 . Fixed once again

This is also not quite idempotent yet, running the dashmate role twice on a seed node will always result in is_dashmate_config_changed being true

Yes, the idempotency is not yet stable, because we often change a lot of values during dashmate setup manually, that causes config re-render.
To be honest, I feel like restart logic that we came up with is too complex to be stable

Other than the above logic issue, this seems to be working fine. It's a bit inefficient to run through the whole ZeroSSL workflow, assign a cert, and then shut down the DAPI container while leaving all the provisioned config and containers in place, but I think changing this would require significant changes to dashmate right? I think we can leave it for now, it's really great to be able to remove the entire mn_evo_services role.

We probably would want to skip that part and use self signed for example, but on the other side. Though we will still need dapis on the seeds later.

'AnsibleUndefinedVariable: ''mode'' is undefined

Fixed

dashmate fails to stop all the containers on evonodes:

I am going to look into that now

@strophy
Copy link
Collaborator

strophy commented Aug 22, 2023

Though we will still need dapis on the seeds later.

In that case we can just leave the current setup, and change behaviour around the DAPI container shutdown when we enable it later.

ansible/roles/dashmate/tasks/main.yml Outdated Show resolved Hide resolved
@shumkov shumkov changed the title feat: replace mn_evo_services with dashmate for seeds feat: dashmate for seeds Aug 23, 2023
ansible/roles/dashmate/tasks/main.yml Outdated Show resolved Hide resolved
ktechmidas
ktechmidas previously approved these changes Aug 25, 2023
Copy link
Contributor

@ktechmidas ktechmidas left a comment

Choose a reason for hiding this comment

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

utACK

test/smoke/tenderdash.js Outdated Show resolved Hide resolved
@strophy strophy merged commit 314df4f into master Aug 28, 2023
1 check passed
@strophy strophy deleted the feat/dashmate-seeds branch August 28, 2023 04:15
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.

4 participants