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

Determine the tasks and division of roles required to migration Zabbix roles #16

Closed
sky-joker opened this issue Apr 26, 2020 · 27 comments
Closed
Labels
enhancement New feature or request

Comments

@sky-joker
Copy link
Contributor

PURPOSE

In this here, we would like to identify the tasks and division of roles required for the below role migration.

@sky-joker sky-joker changed the title Determine the tasks and division of roles required to migration a Zabbix roles Determine the tasks and division of roles required to migration Zabbix roles Apr 26, 2020
@D3DeFi D3DeFi added the enhancement New feature or request label Apr 27, 2020
@D3DeFi
Copy link
Contributor

D3DeFi commented Apr 28, 2020

Ok, so we now compose some task list here and then move each item as a separate issue/note under this project?

Lets start with original skeleton. I am adding a few new tasks, but please feel free to shuffle, change, delete as see fit:

@D3DeFi
Copy link
Contributor

D3DeFi commented Apr 28, 2020

I volunteer to open an issue for each task we agree to go with after this discussion.

@sky-joker
Copy link
Contributor Author

sky-joker commented Apr 28, 2020

Ok, so we now compose some task list here and then move each item as a separate issue/note under this project?

yes :)
I've added a task because I think I'd like to decide on the release cycle and merge policy.

@ericsysmin
Copy link
Contributor

@sky-joker @D3DeFi once the PR for the roles are added, I can do the PR on top of that fork that would include the molecule testing, similar to https://github.com/ericsysmin/ansible-collection-system. Looks like I figured out how to do testing for multiple modules etc with molecule

@D3DeFi
Copy link
Contributor

D3DeFi commented Apr 30, 2020

Ok, I've tried to populate the project with the steps we discussed in this thread. I've also updated my previous comment with tasks list to include references to PR numbers as well.

Issues/notes should be addressed from top to bottom as present in the To Do column. Does it make sense to you guys?

Edit: I wasn't able to assign neither @dj-wasabi to #20 nor @ericsysmin to #22

@sky-joker
Copy link
Contributor Author

Thank you so much @D3DeFi for adding the tasks to the project!

@ericsysmin
Copy link
Contributor

@dj-wasabi, I will work on the CI Tests for the Agent role.

I did take a look at the existing molecule test, and was curious where this file was coming from

      zabbix_agent_tlspskfile: /data/certs/zabbix.psk

@ericsysmin
Copy link
Contributor

ericsysmin commented May 6, 2020

I believe I found a problem caused by the use of - in the collection role name...this is a breaking change that people will need to change in their code in the future.

Update: it's confirmed. Using the - in the role name causes the role to not be discovered. You will not be able to use any roles with -, and need to move to using _.

@D3DeFi
Copy link
Contributor

D3DeFi commented May 8, 2020

I believe I found a problem caused by the use of - in the collection role name...this is a breaking change that people will need to change in their code in the future.

Update: it's confirmed. Using the - in the role name causes the role to not be discovered. You will not be able to use any roles with -, and need to move to using _.

Oh, that actually hurts.

I've had a brief thought about renaming modules into short names in the past. Since the collection now identifies scope by itself, we no longer need the zabbix_ prefix. So instead of zabbix_host we would have just host. I didn't pursue the idea, because in the case of zabbix modules, it is just a cosmetic change. Roles on the other hand are a deal breaker.

If we actually decide to do this for both modules and roles, we can release something like 1.0.0 as a major change release.

Depends on what do you guys think about this. Either changing roles from zabbix-* to zabbix_* or removing zabbix[-_] prefix altogether?

@dj-wasabi
Copy link

I would say removing it. The collection is already specific to Zabbix thus all the module/roles/plugins etc is Zabbix specific, having a zabbix[-_] prefix does not add any extra value to it.

My 2 cents

@dj-wasabi
Copy link

@dj-wasabi, I will work on the CI Tests for the Agent role.

I did take a look at the existing molecule test, and was curious where this file was coming from

      zabbix_agent_tlspskfile: /data/certs/zabbix.psk

Out of the blue, no specific logic behind it. 😄

@dj-wasabi
Copy link

I created all PR's to move the roles to this collection.

@D3DeFi
Copy link
Contributor

D3DeFi commented May 23, 2020

Thanks for the PRs @dj-wasabi !

@ericsysmin have you progressed with CI adjustments?

I believe we should start with removing "zabbix-" prefixes and #21 . @sky-joker do we remove "zabbix_" prefixes from modules as well? If yes, I think that at least symlinks zabbix_host -> host should be created for backwards compatibility.

@sky-joker
Copy link
Contributor Author

sky-joker commented May 26, 2020

I think it's a good idea to to remove zabbix_ prefix from role name.
I have an image of ultimately the following playbook.

---
- name: test
  hosts: localhost
  gather_facts: no
  collections:
    - community.zabbix
  tasks:
    - import_role:
        name: host

But I have to worry if the other role has the same role name.
If the role name is duplicate, the following playbook can work around it tentatively, but...

---
- name: test
  hosts: localhost
  gather_facts: no
  tasks:
    - import_role:
        name: community.zabbix.host

@dj-wasabi
Copy link

I personally like the 2nd example as it is exactly clear what role will be executed, which if you have multiple collections configured with the 1st example isn't the case.

If we document the examples in the roles documentation with the 2nd example, then we can sort of guarantee that it works?

@D3DeFi
Copy link
Contributor

D3DeFi commented May 27, 2020

I agree with the 2nd example as well. We definitely should promote more explicit approach in our documentation to not further confuse users.

What I originally meant with my question was what to do with plugins/modules since we already know that we do need to rename roles as confirmed by @ericsysmin in the comment above.

As per the Using collections documentation example we have two options here:

  1. Leave modules as they are right now. This will enable users to carelessly use short names without much confusion:
- hosts: all
  collections:
   - community.zabbix

  tasks:
    - zabbix_host:
        server_url: https://zabbix.example.com
        ...

But will create duplicity for word 'zabbix' if used as FQCN:

- hosts: all
  tasks:
    - community.zabbix.zabbix_host:
        server_url: https://zabbix.example.com
        ...
  1. Remove zabbix_ prefix from modules as well. This will confuse users when using short names as shown in the first point:
- hosts: all
  collections:
   - community.zabbix

  tasks:
    - host:
        server_url: https://zabbix.example.com
        ...

But will be prettier with FQCN:

- hosts: all
  tasks:
    - community.zabbix.host:
        server_url: https://zabbix.example.com
        ...

Middle ground is to symlink plugins/modules/zabbix_host -> plugins/modules/host, but this seems more like a temporary solution.

From the blog post a few months back, they suggest dropping the prefixes as it is known from a collection name to which namespace they belong.

At the same time I believe that most of the collections will keep the prefixes (e.g. grafana collection repo) for backwards compatibility, but it is essential for me to at least discuss this possibility with modules since we need to handle this with roles no matter what.

Btw, there is still possibility to rename roles from zabbix-agent to zabbix_agent.

@D3DeFi
Copy link
Contributor

D3DeFi commented May 27, 2020

I went through the other collections in https://github.com/ansible-collection and none of them decided to remove prefixes they were using so far, so we can probably ignore my blabbering in the previous comment. I am starting to think that simply renaming roles to zabbix_ would be a more consistent approach for now.

@sky-joker
Copy link
Contributor Author

sky-joker commented May 27, 2020

I am starting to think that simply renaming roles to zabbix_ would be a more consistent approach for now.

I think that is good.
I think there would be less confusion if the documentation told you how to use the roles and modules.

For example, if use collections keyword to execute the module.

- hosts: all
  collections:
    - community.zabbix
  tasks:
    - zabbix_host:
      (snip)

For example, if use FQCN to execute the module.

- hosts: all
  tasks:
    - community.zabbix.zabbix_host:
      (snip)

For example, if use collections keyword to execute the role.

- hosts: all
  collections:
    - community.zabbix
  tasks:
    - import_role:
      name: zabbix_host:

For example, if use FQCN to execute the role.

- hosts: all
  tasks:
    - import_role:
      name: community.zabbix.zabbix_host:

I think that's better rename role to zabbix_.

@ericsysmin
Copy link
Contributor

I ran into issues on testing, we currently molecule does not support the proper installation of roles and collections mixed together yet as part of dependencies. I am working on a solution.

@ericsysmin
Copy link
Contributor

ericsysmin commented Jun 5, 2020

@D3DeFi @dj-wasabi there's the only roles that should exist would be

  • community.zabbix.agent
  • community.zabbix.javagateway
  • community.zabbix.proxy
  • community.zabbix.server
  • community.zabbix.web

I do believe however that modifying the default/vars will create a lot of issues as people may be using higher level vars to fill in those values for example a hosts file providing vars for the host instead of within the playbook which calls that role.

https://github.com/ericsysmin/community.zabbix/tree/devel-roles-migration

@ericsysmin
Copy link
Contributor

ericsysmin commented Jun 5, 2020

@dj-wasabi how were you testing all of this before? it looks like you were deploying the entire Zabbix infrastructure, do we still want to test out an entire infrastructure @D3DeFi I was hoping to test each piece independently not all tied together.

@dj-wasabi
Copy link

@ericsysmin

With the Zabbix Agent I had 3 scenario's for Molecule:

  • Just the agent, with latest version;
  • Server + Web and agents, to make sure it can create the configuration fine in the Zabbix Server + web so monitoring works outofthebox;
  • Just the agent, with previous version. Goal was to use the LTS Zabbix versions, so that these would always work. But had no time for it yet to implement it..

Server + Proxy had 1 scenario, both mysql and postgressql where deployed for CentOS, Ubuntu and Debian;

Web had 1 scenario, both mysql and posgresql where deployed, but had to deploy the servers as well.

I don't think there was much with the javagateway?

@D3DeFi
Copy link
Contributor

D3DeFi commented Jun 6, 2020

I do believe however that modifying the default/vars will create a lot of issues as people may be using higher level vars to fill in those values for example a hosts file providing vars for the host instead of within the playbook which calls that role.

@ericsysmin I strongly advice against renaming anything from vars/ or defaults/ as this will create a lot of chaos (You've already pointed that out too).

I was wondering before if simply renaming roles from zabbix- to zabbix_ wouldn't be a better approach and this further affirms the thought for me. Variables could match name of the role with this approach:

roles:
  - role: community.zabbix.zabbix_agent
    zabbix_agent_server: example

I also believe that most of the users will opt for using shorter names with collections: keyword (to simply have less work when porting to 2.10) and would want to leave module and role names untouched:

  collections:
    - community.zabbix

@dj-wasabi have you thought about this after my last set of comment in this thread? Or you would still rather go with removing zabbix- prefixes altogether?

@D3DeFi
Copy link
Contributor

D3DeFi commented Jun 6, 2020

@dj-wasabi how were you testing all of this before? it looks like you were deploying the entire Zabbix infrastructure, do we still want to test out an entire infrastructure @D3DeFi I was hoping to test each piece independently not all tied together.

Judging from other comments too, I think that it is better for now to at least test each role separately than not at all :) We can always think of how to merge it together later.

@dj-wasabi
Copy link

@D3DeFi We could always remove in a later stage the zabbix-|_ prefix. So maybe for now we should keep the zabbix_ ones and rename the roles with a dash to an underscore. This would mean we see zabbix twice, but I think for now this is easier to go live with.

@D3DeFi
Copy link
Contributor

D3DeFi commented Jun 10, 2020

I believe we are ready to merge devel-roles-migration to master once #74 and #76 get through the review. As discussed, CI for roles will come later as it will postpone this integration too much for now.

We probably should address #77 before the merge tho. I have enough of documentation for today, so volunteers are welcome :)

@D3DeFi
Copy link
Contributor

D3DeFi commented Aug 8, 2020

I am closing this issue as it is no longer used for any communication regarding migration of zabbix roles (which was successfully done, good job everyone!) and any follow up tasks have their separate issues open

@D3DeFi D3DeFi closed this as completed Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants