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

Per-Script Worker Queue Setting #16552

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

jchambers2012
Copy link

Fixes: #16516

This PR add a new Meta field to the scripts called rq_queue_name. This new field will allow time critical scripts to route to the high queue and run before scripts that are designed to be background jobs and run at a lower priority. If the script programmer tries to route the script to a non-existing queue, the current logic will route the script to the queue set to for scripts.

The PR was tested via the GUI and via the API. This was assessed by creating 4 Custom scripts that each route to diffrent queues. Three scripts were set to route to the standard high, default, low queues and a fourth was set to route to a queue called None that did not exist in configuration. The three scripts that had queue ran in the correct order with high script running first, followed by default then low. The fourth script that routed to None was redirected to the default queue.

GUI Tests:

image

image

API Tests:

image

Script Used for testing

from extras.scripts import *
import time


class testLowQue(Script):
    class Meta:
        name = "Log Test  - Que Low"
        scheduling_enabled = False
        description = ""
        rq_queue_name = "low"

    def run(self, data, commit):
        for i in range(6):
            self.log_info(f"Log Test - Sleeping for 10 Sec - Que Low: {i}")
            time.sleep(10)
        return str("")


class testDefaultQue(Script):
    class Meta:
        name = "Log Test  - Que Default"
        scheduling_enabled = False
        description = ""
        rq_queue_name = "default"

    def run(self, data, commit):
        for i in range(6):
            self.log_info(f"Log Test - Sleeping for 10 Sec - Que Default: {i}")
            time.sleep(10)
        return str("")


class testHighQue(Script):
    class Meta:
        name = "Log Test  - Que high"
        scheduling_enabled = False
        description = ""
        rq_queue_name = "high"

    def run(self, data, commit):
        for i in range(6):
            self.log_info(f"Log Test - Sleeping for 10 Sec - Que High: {i}")
            time.sleep(10)
        return str("")


class testNoneQue(Script):
    class Meta:
        name = "Log Test  - Que None"
        scheduling_enabled = False
        description = ""
        rq_queue_name = "None"

    def run(self, data, commit):
        for i in range(6):
            self.log_info(f"Log Test - Sleeping for 10 Sec - Que None: {i}")
            time.sleep(10)
        return str("")

netbox/core/models/jobs.py Outdated Show resolved Hide resolved
@jchambers2012
Copy link
Author

Updated the code: 5676c4a

Copy link
Contributor

github-actions bot commented Jul 8, 2024

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 8, 2024
Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

Also, I'm not sure if this is missing something, what mechanism would be used to service the extra queue? Would you need to make a plugin with the queues defined or should there be a change to settings/config.py to allow an additional queue to be defined? Currently NetBox only defines / services the high, default,low queue.

netbox/core/models/jobs.py Outdated Show resolved Hide resolved
docs/customization/custom-scripts.md Outdated Show resolved Hide resolved
@jchambers2012
Copy link
Author

Also, I'm not sure if this is missing something, what mechanism would be used to service the extra queue? Would you need to make a plugin with the queues defined or should there be a change to settings/config.py to allow an additional queue to be defined? Currently NetBox only defines / services the high, default,low queue.

Ya this was mainly to support custom queue without the need to do a later PR to support those cases. If we dont think they are heavily used by the user base then it could be removed and limited to High, Default, Low queue.

https://netboxlabs.com/docs/netbox/en/stable/plugins/development/background-tasks/

Plugins can also add custom queues for their own needs by setting the queues attribute under the PluginConfig class. An example is included below:

@jeremystretch jeremystretch removed the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 12, 2024
@@ -108,6 +108,10 @@ commit_default = False

By default, a script can be scheduled for execution at a later time. Setting `scheduling_enabled` to False disables this ability: Only immediate execution will be possible. (This also disables the ability to set a recurring execution interval.)

### `rq_queue_name`
Copy link
Contributor

Choose a reason for hiding this comment

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

In #16927 I'm introducing a general purpose job API used by NetBox and made available for plugins. A minor background for this is abstraction to decouple plugins from using Redis directly, making future changes possible. Therefore I'd like to suggest not using parameter names related to Redis queues, i.e. just using something like queue instead of rq_queue_name.

Copy link
Author

Choose a reason for hiding this comment

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

I'm good with whatever Netbox wants to use for name, be it queue, worker_queue or job_queue. But I won't make any changes right now till the Netbox maintainers pick the language that is preferred for this.

@jchambers2012
Copy link
Author

@arthanson please see #16552 (comment) and let me know if there is a preference here on what the variable should be called?

@jeremystretch
Copy link
Member

I like the general idea, but I'm not sure explicitly specifying a queue name inside each script is the optimal approach. For one, it could become highly repetitive when dealing with a large number of scripts. Second, renaming the queues would require modifying each individual script.

IMO the use case would be better served by the ability to configure a policy mapping scripts (by module, name, or some other attribute) to the desired queues. This would allow for more flexibility to adjust the mapping over time without needing to modify individual scripts. A configuration parameter would probably suffice for this purpose.

Separately, we should probably also include the ability for the user to manually select the queue when executing a script (via the web UI or REST API), perhaps limiting the available options to a subset of approved queues.

@jchambers2012
Copy link
Author

@jeremystretch

Parden my ignorance if configuration is dynamically loaded each time there is a change while the application is running.

Just to defend the pro-per-script setting. At our shop, we need to do change control for making application-level changes and restarting of the platform as this would make it unavailable (even briefly) for the users and CI/DI pipelines. The outage could be longer if someone makes a typo in the config (IE not a restart but a start-debug-retry). We use Data files only for all our NetBox script so they go through their own CI/DI automated and manual testing and validation process. So, to make this a “global“ configuration change for mappings, at least for our shop, would make it more paperwork to move the setting their vs just syncing in the scripts.

I would also be an Anti-User Selected Queue setting as everyone will just think their job should be the highest priority, though it is not a bad idea to have the option available and exposed if needed. Would you think this be a new per-script setting to enable queue selection like scheduling (scheduling_enabled) has a flag today? Something like user_queue_selection_enables = True? This setting might also need to be a per-script option as the script writer may not want their script to have queue changes much like some job should not be scheduled and may/may not have the ability to make changes to the platform configuration to quickly flip this flag on/off.

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Aug 31, 2024
@jchambers2012
Copy link
Author

Works been a bit crazy and i will try and get to rewriting with 4.1.x code as soon as I have the time to experiment with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending closure Requires immediate attention to avoid being closed for inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per Script Worker Queue Setting
4 participants