Add '/queue simple' path #260

samburney · 2024-02-09T23:39:26Z

SUMMARY

Support API path '/queue simple'

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

api_info
api_modify

github-actions · 2024-02-09T23:40:57Z

Docs Build 📝

This PR is closed and any previously published docsite has been unpublished.

codecov · 2024-02-09T23:45:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.99%. Comparing base (542a362) to head (c282423).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #260      +/-   ##
==========================================
+ Coverage   82.97%   82.99%   +0.02%     
==========================================
  Files          32       32              
  Lines        4046     4051       +5     
  Branches      871      873       +2     
==========================================
+ Hits         3357     3362       +5     
  Misses        506      506              
  Partials      183      183

Flag	Coverage Δ
integration	`66.86% <ø> (ø)`
sanity	`22.08% <33.33%> (-0.03%)`	⬇️
units	`82.93% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

derdeagle · 2024-02-10T13:15:59Z

plugins/module_utils/_api_data.py

+                'burst-threshold': KeyInfo(default=0),
+                'burst-time': KeyInfo(default='0s'),
+                'comment': KeyInfo(can_disable=True, remove_value=''),
+                'disabled': KeyInfo(default=False),


I am not sure if setting the default for disabled to False is practical here. Even when setting it to no directly on a device it is neither shown in the output of /queue simple export nor in the output of /queue simple print detail.
Setting the default to False here looks like it would lead to changes on every execution. Can you confirm please?

I can confirm that setting to the disabled default to 'False' does not result in a change every time.

You're correct in that it does not appear in /export or api_info, I can remove this default if you like.

ok: [60brh-dc2-rtr1] => { "show_interface_queues": { "changed": false, "failed": false, "result": [ { "!dst": null, "!time": null, ".id": "*1", "bucket-size": "0.1/0.1", "burst-limit": "0/0", "burst-threshold": "0/0", "burst-time": "0s/0s", "comment": "Core: 172msa-dc1-core1:bonding1-vlan318 {HU-xxx-xxx} [1G]", "limit-at": "0/0", "max-limit": "0/1000000000", "name": "queue-bonding1-vlan318", "priority": "8/8", "queue": "default-small/default-small", "target": "bonding1-vlan318" } ] } }

Thank you for testing. If it doesn't lead to constant changes I am fine with it 👍

derdeagle · 2024-02-10T13:20:18Z

plugins/module_utils/_api_data.py

+                'name': KeyInfo(),
+                'packet-marks': KeyInfo(default=''),
+                'parent': KeyInfo(default='none'),
+                'priority': KeyInfo(default=8),


Are you sure about those values? Unfortunately I cannot test it on my own right now via Ansible but manually set I get the error message expected value of download-priority (line 1 column 39). The value 8/8 is the default for me.

The following is the output of /queue simple print detail for me on RouterOS 7.13 where I only specified the target manually which you already correctly set as required.

name="queue1" target=1.1.1.12/32 parent=none packet-marks="" priority=8/8 queue=default-small/default-small limit-at=0/0 max-limit=0/0 burst-limit=0/0 burst-threshold=0/0 burst-time=0s/0s bucket-size=0.1/0.1.

I took most of the default keys from the existing implementation of 'queue tree'. These values work as expected in an environment running 7.13.4.

The returned data does include the '/'s but they do not appear to be required as a part of the data set.

--- before +++ after @@ -1,3 +1,21 @@ { - "data": [] + "data": [ + { + ".id": "*1", + "bucket-size": "0.1/0.1", + "burst-limit": "0/0", + "burst-threshold": "0/0", + "burst-time": "0s/0s", + "comment": "Core: 172msa-dc1-core1:bonding1-vlan318 {HU-xxx-xxx} [1G]", + "disabled": false, + "limit-at": "0/0", + "max-limit": "0/1000000000", + "name": "queue-bonding1-vlan318", + "packet-marks": "", + "parent": "none", + "priority": "8/8", + "queue": "default-small/default-small", + "target": "bonding1-vlan318" + } + ] }

I'm happy to update and test the defaults to be include splitting the upload/download values if you like but it does work as-is.

Mhh interesting fact that the API accepts it but CLI doesn't. I then guess there are also no changes when rolled out again with only one value defined.
It seems it is working and it is not documented by MikroTik so 👍

I just spun up a test and it shows changes for me, please see below.

"data": [ { ".id": "*7", - "bucket-size": "0.1/0.1", - "burst-limit": "0/0", - "burst-threshold": "0/0", - "burst-time": "0s/0s", + "bucket-size": "0.1", + "burst-limit": 0, + "burst-threshold": 0, + "burst-time": "0s", "disabled": true, - "limit-at": "0/0", - "max-limit": "7000000/2000000", + "limit-at": 0, + "max-limit": "7M/2M", "name": "Limit wifi speed", "packet-marks": "", "parent": "none", - "priority": "8/8", + "priority": 8, "queue": "pcq-upload-default/pcq-download-default", "target": "10.10.2.0/24"

The only data I have provided is the following.

- disabled: True max-limit: 7M/2M name: Limit wifi speed queue: pcq-upload-default/pcq-download-default target: 10.10.2.0/24

Could you please recheck?

Can you please try max-limit: 7000000/2000000? The API will accept k/M/G/T etc... but converts it to bits on output. This causes it to always trigger a change.

I created a filter that does this conversion, allowing me to still use the abbreviated format.

Tbh. I could live with that but what I wanted to point out is the change from e.g. "priority": "8/8" to "priority": 8 without manually defining it. So the defaults you set do lead to constant changes (at least for me). The diff you posted only shows a new entry. Would you mind applying this change, then do a --check run again and post the output, please?

I've tested with your exact data with the results I've come to expect.

Playbook

- name: Test queue simple API hosts: mikrotik gather_facts: false module_defaults: community.routeros.api_modify: hostname: "{{ ansible_host }}" username: "{{ ansible_ssh_user }}" password: "{{ ansible_ssh_password }}" tasks: - name: Remove simple queues community.routeros.api_modify: path: queue simple handle_absent_entries: remove data: [] - name: Add simple queue (Create) community.routeros.api_modify: path: queue simple data: - disabled: True max-limit: 7M/2M name: Limit wifi speed queue: pcq-upload-default/pcq-download-default target: 10.10.2.0/24 - name: Add simple queue (Test for change with 7M/2M) community.routeros.api_modify: path: queue simple data: - disabled: True max-limit: 7M/2M name: Limit wifi speed queue: pcq-upload-default/pcq-download-default target: 10.10.2.0/24 - name: Add simple queue (Test for change with 7000000/2000000) community.routeros.api_modify: path: queue simple data: - disabled: True max-limit: 7000000/2000000 name: Limit wifi speed queue: pcq-upload-default/pcq-download-default target: 10.10.2.0/24

With --check

$ ansible-playbook playbooks/queue_simple_test.yml -l gns3-lab-core1 --diff --check PLAY [Test queue simple API] ********************************************************************************************************************************* TASK [Remove simple queues] ********************************************************************************************************************************** --- before +++ after @@ -1,20 +1,3 @@ { - "data": [ - { - ".id": "*9", - "bucket-size": "0.1/0.1", - "burst-limit": "0/0", - "burst-threshold": "0/0", - "burst-time": "0s/0s", - "disabled": true, - "limit-at": "0/0", - "max-limit": "7000000/2000000", - "name": "Limit wifi speed", - "packet-marks": "", - "parent": "none", - "priority": "8/8", - "queue": "pcq-upload-default/pcq-download-default", - "target": "10.10.2.0/24" - } - ] + "data": [] } changed: [gns3-lab-core1] TASK [Add simple queue (Create)] ***************************************************************************************************************************** --- before +++ after @@ -8,7 +8,7 @@ "burst-time": "0s/0s", "disabled": true, "limit-at": "0/0", - "max-limit": "7000000/2000000", + "max-limit": "7M/2M", "name": "Limit wifi speed", "packet-marks": "", "parent": "none", changed: [gns3-lab-core1] TASK [Add simple queue (Test for change with 7M/2M)] ********************************************************************************************************* --- before +++ after @@ -8,7 +8,7 @@ "burst-time": "0s/0s", "disabled": true, "limit-at": "0/0", - "max-limit": "7000000/2000000", + "max-limit": "7M/2M", "name": "Limit wifi speed", "packet-marks": "", "parent": "none", changed: [gns3-lab-core1] TASK [Add simple queue (Test for change with 7000000/2000000)] *********************************************************************************************** ok: [gns3-lab-core1] PLAY RECAP *************************************************************************************************************************************************** gns3-lab-core1 : ok=4 changed=3 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0

Without --check

$ ansible-playbook playbooks/queue_simple_test.yml -l gns3-lab-core1 --diff PLAY [Test queue simple API] ********************************************************************************************************************************* TASK [Remove simple queues] ********************************************************************************************************************************** --- before +++ after @@ -1,20 +1,3 @@ { - "data": [ - { - ".id": "*9", - "bucket-size": "0.1/0.1", - "burst-limit": "0/0", - "burst-threshold": "0/0", - "burst-time": "0s/0s", - "disabled": true, - "limit-at": "0/0", - "max-limit": "7000000/2000000", - "name": "Limit wifi speed", - "packet-marks": "", - "parent": "none", - "priority": "8/8", - "queue": "pcq-upload-default/pcq-download-default", - "target": "10.10.2.0/24" - } - ] + "data": [] } changed: [gns3-lab-core1] TASK [Add simple queue (Create)] ***************************************************************************************************************************** --- before +++ after @@ -1,3 +1,20 @@ { - "data": [] + "data": [ + { + ".id": "*A", + "bucket-size": "0.1/0.1", + "burst-limit": "0/0", + "burst-threshold": "0/0", + "burst-time": "0s/0s", + "disabled": true, + "limit-at": "0/0", + "max-limit": "7000000/2000000", + "name": "Limit wifi speed", + "packet-marks": "", + "parent": "none", + "priority": "8/8", + "queue": "pcq-upload-default/pcq-download-default", + "target": "10.10.2.0/24" + } + ] } changed: [gns3-lab-core1] TASK [Add simple queue (Test for change with 7M/2M)] ********************************************************************************************************* changed: [gns3-lab-core1] TASK [Add simple queue (Test for change with 7000000/2000000)] *********************************************************************************************** ok: [gns3-lab-core1] PLAY RECAP *************************************************************************************************************************************************** gns3-lab-core1 : ok=4 changed=3 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0

Sorry it took me so long to respond. I tried it with version 7.13 and 7.13.4 but I still do get other results than you it seems. With your version I always get changes regardless of using --check or not. When using --diff --check I do get the differences shown but with only --diff I just see the following

PLAY [mikrotik_devices] *********************************************************************************************** TASK [Manage /queue/simple] ******************************************************************************************* changed: [dev1.example.com] changed: [dev2.example.com] changed: [dev3.example.com] changed: [dev4.example.com] changed: [dev5.example.com] changed: [dev6.example.com] changed: [dev7.example.com] changed: [dev8.example.com] PLAY RECAP ************************************************************************************************************ dev3.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev4.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev5.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev1.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev2.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev7.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev6.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev8.example.com : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0

So the API changes something but RouterOS transformed it so that when checking back for the result it is the same as before. That is not desirable IMHO.
When defining the defaults as MikroTik uses them (in _api_data.py) I get the following (IMHO desirable) result:

PLAY [mikrotik_devices] *********************************************************************************************** TASK [Manage /queue/simple] ******************************************************************************************* ok: [dev1.example.com] ok: [dev2.example.com] ok: [dev3.example.com] ok: [dev4.example.com] ok: [dev5.example.com] ok: [dev6.example.com] ok: [dev7.example.com] ok: [dev8.example.com] PLAY RECAP ************************************************************************************************************ dev4.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev3.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev5.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev1.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev2.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev7.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev6.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0 dev8.example.com : ok=1 changed=0 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0

* Update api_info.py * Update api_modify.py * Update _api_data.py * Update _api_data.py * Update _api_data.py * Update api_info.py * Update api_modify.py * Update api_info.py * Update api_modify.py * Create 259-add-routeros7-support-for-ip-vrf.yml * Update changelogs/fragments/259-add-routeros7-support-for-ip-vrf.yml Co-authored-by: Felix Fontein <[email protected]> --------- Co-authored-by: Felix Fontein <[email protected]>

* Fix date removal in nonverbose config As in newer versions of RouterOS the date format is 2024-10-02 and no longer 2024/10/02, the regex did no longer match all cases. This is fixed. * Add changelog fragment

Signed-off-by: Michael Hanselmann <[email protected]>

…ible-collections#264) The `running` field can't be configured. By having a default value it's written and shows up in diffs.

…nt_value, default, or required (ansible-collections#265) * Prevent read_only usage with can_disable, remove_value, absent_value, default, or required. * Add test.

Bumps [fsfe/reuse-action](https://github.com/fsfe/reuse-action) from 2 to 3. - [Release notes](https://github.com/fsfe/reuse-action/releases) - [Commits](fsfe/reuse-action@v2...v3) --- updated-dependencies: - dependency-name: fsfe/reuse-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ansible-collections#268) * Ignore pylint warnings for construct that does not work with Python 2. * Revert "Ignore pylint warnings for construct that does not work with Python 2." This reverts commit 407b2ef. * Different approach: use ignore.txt since otherwise ansible-core 2.14 tests fail.

* Add interface wifi paths * Update changelog * Remove most defaults

- Updated all default values to match those from the API - Re-ordered fields to match API output - Target is not required; added default value

samburney · 2024-03-30T10:35:29Z

@derdeagle, I agree with your opinion on this, the default should match the api_info output.

I've updated the defaults as suggested and made a couple of other changes as well.

Given the time delay between my first commit and now I also did a git rebase, which has introduced a number of commits into this pull request. I'm not familiar enough with github (Or git in general) to understand whether this is normal, or how to fix it if it isn't; so my apologies for the mess.

I have a number of similar changes to submit, but I'll wait until this one is successful first.

Thanks.

felixfontein · 2024-03-30T10:41:41Z

Given the time delay between my first commit and now I also did a git rebase, which has introduced a number of commits into this pull request. I'm not familiar enough with github (Or git in general) to understand whether this is normal, or how to fix it if it isn't; so my apologies for the mess.

This is definitely not normal, but you're not the first who managed this (I've seen this both on GitHub in various repos and in our company's GitLab instance). I've never figured out how anyone managed to do this though :) You probably have to try to rebase them away.

If you want I can also try that and then force-push to your branch. (You'll then have to pay a bit of care when pulling so you don't end up with an even more messed up branch. Potentially it's easier to delete the local branch and check out the branch from your remote afterwards if the result looks suspicious.)

samburney · 2024-03-30T11:06:49Z

@felixfontein I ended up with this result by typing 'git rebase main' in my local branch, and then doing a push/pull. Happy with any assistance in cleaning it up!

samburney · 2024-03-30T11:16:12Z

plugins/module_utils/_api_data.py

The API does require the 'target' value, however it has some caveats.

A value of 0.0.0.0/0 will be returned as '' from the API, resulting in a change every time:

ok: [testrtr1] => { "queue_simple_api_info": { "changed": false, "failed": false, "result": [ { "!comment": null, "!dst": null, "!time": null, ".id": "*2D", "name": "Test queue with just defaults", "target": "" <----- This is with "target: 0.0.0.0/0" } ] } }

A value of '' will be accepted, but the API will return this as null (As below), again always resulting in a change:

ok: [testrtr1] => { "queue_simple_api_info": { "changed": false, "failed": false, "result": [ { "!comment": null, "!dst": null, "!target": null, <----- This is with "target: ''" "!time": null, ".id": "*29", "name": "Test queue with just defaults" } ] } }

Other values, such as IP addresses and interface names are accepted and behave as expected.

Does the module contain any logic to account for these kinds of edge cases?

samburney · 2024-03-30T11:31:15Z

This is definitely not normal, but you're not the first who managed this (I've seen this both on GitHub in various repos and in our company's GitLab instance). I've never figured out how anyone managed to do this though :) You probably have to try to rebase them away.

I have created sane branch - I created a new branch, rebased and then cherry-picked my subsequent commit above: https://github.com/samburney/community.routeros/tree/add_simple_queues_2

Is the fix to do that on my original branch and force-push? I'm hesitant to do that without a bit of guidance to be honest!

Thanks.

felixfontein · 2024-03-30T11:55:38Z

@felixfontein I ended up with this result by typing 'git rebase main' in my local branch, and then doing a push/pull. Happy with any assistance in cleaning it up!

Did you first update your local main branch before doing that? And after doing the rebase, did you do a force push afterwards?

…f 0.0.0.0/0 and '' (Empty string).

samburney · 2024-03-30T12:06:19Z

@felixfontein I ended up with this result by typing 'git rebase main' in my local branch, and then doing a push/pull. Happy with any assistance in cleaning it up!

Did you first update your local main branch before doing that? And after doing the rebase, did you do a force push afterwards?

Welp, I made it worse :|

I'm just going to close this pull request and create a new one.

samburney · 2024-03-30T12:12:46Z

Recreated as #269.

Add '/queue simple' path

bc6f386

Add changelogs/fragments/260-add-queue_simple-path.yml

4e5c7ee

samburney marked this pull request as ready for review February 9, 2024 23:44

derdeagle reviewed Feb 10, 2024

View reviewed changes

felixfontein and others added 14 commits March 30, 2024 20:52

Add MarkDown changelog. (ansible-collections#261)

be10d44

Fix date removal in nonverbose config (ansible-collections#262)

fc569a5

* Fix date removal in nonverbose config As in newer versions of RouterOS the date format is 2024-10-02 and no longer 2024/10/02, the regex did no longer match all cases. This is fixed. * Add changelog fragment

Prepare 2.13.0 release.

fdb4ebd

Release 2.13.0.

ec2c757

Add missing fields in "system package update" (ansible-collections#263)

f88cdf9

Signed-off-by: Michael Hanselmann <[email protected]>

Remove default value for "running" field in "interface wireless" (ans…

9bada52

…ible-collections#264) The `running` field can't be configured. By having a default value it's written and shows up in diffs.

KeyInfo: prevent read_only usage with can_disable, remove_value, abse…

da231f4

…nt_value, default, or required (ansible-collections#265) * Prevent read_only usage with can_disable, remove_value, absent_value, default, or required. * Add test.

Prepare 2.14.0 release.

f3763d5

Add interface wifi paths (ansible-collections#266)

ba984d0

* Add interface wifi paths * Update changelog * Remove most defaults

Release 2.14.0.

a175a15

Updated queue-simple path:

668d874

- Updated all default values to match those from the API - Re-ordered fields to match API output - Target is not required; added default value

samburney commented Mar 30, 2024

View reviewed changes

'target' *is* required, however it interracts strangely with values o…

c282423

…f 0.0.0.0/0 and '' (Empty string).

samburney force-pushed the add_simple_queues branch from 6b1a5f5 to c282423 Compare March 30, 2024 12:04

samburney marked this pull request as draft March 30, 2024 12:07

samburney closed this Mar 30, 2024

samburney deleted the add_simple_queues branch March 30, 2024 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add '/queue simple' path #260

Add '/queue simple' path #260

samburney commented Feb 9, 2024

github-actions bot commented Feb 9, 2024 •

edited

Loading

codecov bot commented Feb 9, 2024 •

edited

Loading

derdeagle Feb 10, 2024

samburney Feb 11, 2024 •

edited

Loading

derdeagle Feb 12, 2024

derdeagle Feb 10, 2024

samburney Feb 11, 2024

derdeagle Feb 12, 2024

derdeagle Feb 12, 2024

samburney Feb 12, 2024

derdeagle Feb 12, 2024

samburney Feb 12, 2024

derdeagle Feb 17, 2024

samburney commented Mar 30, 2024

felixfontein commented Mar 30, 2024

samburney commented Mar 30, 2024

samburney Mar 30, 2024

samburney commented Mar 30, 2024

felixfontein commented Mar 30, 2024

samburney commented Mar 30, 2024

samburney commented Mar 30, 2024

Add '/queue simple' path #260

Add '/queue simple' path #260

Conversation

samburney commented Feb 9, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

github-actions bot commented Feb 9, 2024 • edited Loading

Docs Build 📝

codecov bot commented Feb 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

samburney Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Playbook

With --check

Without --check

Choose a reason for hiding this comment

samburney commented Mar 30, 2024

felixfontein commented Mar 30, 2024

samburney commented Mar 30, 2024

Choose a reason for hiding this comment

samburney commented Mar 30, 2024

felixfontein commented Mar 30, 2024

samburney commented Mar 30, 2024

samburney commented Mar 30, 2024

github-actions bot commented Feb 9, 2024 •

edited

Loading

codecov bot commented Feb 9, 2024 •

edited

Loading

samburney Feb 11, 2024 •

edited

Loading