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

Feature: Additional Allocations #2

Closed
wants to merge 22 commits into from
Closed

Conversation

snags141
Copy link

@snags141 snags141 commented Jan 4, 2022

Changed:

  • Modified the server creation logic (wisp_CreateAccount) to check if the additional allocations field has been set. If it has, fetches all allocations from each applicable node.
  • Updated Readme instructions, in particular the FAQ section regarding additional allocations and the API permissions image to include read access to allocations.

Added:

  • Comments to code to help improve readability for someone picking it up for the first time
  • Log messages that are optionally turned on in your WHMCS settings. This should help troubleshoot issues when starting to use this module.
  • additional_ports parameter (optional) -> Allows specifying additional allocations based on an offset of the first available allocation. Allocation assignment logic explained below
  • additional_port_fail_mode (optional) -> Specifies what action the module should take if it fails to find nodes with allocations available (e.g. allocations are either full or so full and fragmented such that it can't facilitate the required offsets). Continue = Create the server anyways, but only with one allocation, whatever is available. Stop = Raise an exception and stop the server from being created.

Allocation Assignment Logic

(Assuming the optional additional allocations parameter has been set, otherwise continues as normal)

  1. Pull a list of nodes based on the location ID
  2. For each node, pull all allocations
  3. Filter available allocations into an array of ports by each IP address
  4. Check for a port that matches base port and base port+ [each additional allocation offset]
  5. If found available allocations that meet requirements, return and create the server as normal (If any environment parameters are mapped to an allocation like RCON_PORT we assign those before creating the server)
  6. If not, continue based on fail mode

Other Notes

The only minor thing to be aware of is the logic I've implemented pays no attention to the optional field "port_range".
If "additional_ports" is set, it'll just find any ports available that meet your requirements, ignoring any specified port range in the "port_range" parameter.
It's possible to change this in future, but I didn't see this as being an issue and using both parameters together seems to be a rare use-case.

More than happy to hear any feedback or suggestions to improve this before merge 👍
Cheers all!

@snags141
Copy link
Author

snags141 commented Jan 4, 2022

One thing for me to change before merge, based on feedback from kubi: change activity log calls to module log

@snags141 snags141 marked this pull request as draft January 5, 2022 03:29
@snags141
Copy link
Author

snags141 commented Jan 5, 2022

Temporarily converted to draft while I add support to use the additional allocations in the startup string parameter.

@snags141
Copy link
Author

snags141 commented Jan 5, 2022

New commits added:
✅ Action Kubi feedback: Convert logActivity functions to logModuleCall.
✅ Add support to assign "auto allocation" ports to egg variables such as RCON_PORT.
This Required changing the additional_ports field in whmcs to expect a json string. Readme has been updated explaining how to use.

@snags141 snags141 marked this pull request as ready for review January 5, 2022 15:24
Copy link
Contributor

@TrixterTheTux TrixterTheTux left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! From a quick glance, I'd recommend the following changes. I'll try to play around with this locally during the next week.

README.md Outdated Show resolved Hide resolved
wisp/wisp.php Outdated Show resolved Hide resolved
wisp/wisp.php Outdated Show resolved Hide resolved
wisp/wisp.php Outdated Show resolved Hide resolved
wisp/wisp.php Show resolved Hide resolved
wisp/wisp.php Show resolved Hide resolved
wisp/wisp.php Outdated Show resolved Hide resolved
@snags141
Copy link
Author

snags141 commented Jan 5, 2022

Thanks for the review Trixter - I've gone through and fixed the issues you mentioned, happy to make any further changes as necessary. Look forward to hearing back whenever you or the team get a chance to play around with it 👍
Cheers!

@IIPoliII
Copy link

IIPoliII commented Apr 6, 2022

Will this be merged?

@HerrSammyDE
Copy link

Update?

Copy link
Contributor

@krenny krenny left a comment

Choose a reason for hiding this comment

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

All the bugs I found at snags141#6

Fetches and returns all nodes with a specific location_id
*/
$filteredNodes = Array();
$nodes = wisp_API($params, 'nodes/');
Copy link
Contributor

Choose a reason for hiding this comment

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

The nodes/ endpoint should be changed to nodes since wispv2 returns an error when using nodes/

Comment on lines +77 to +84
| ------------ | ------------ | ------------ | ------------ |
| Rust | Game port and RCON port | `{1:"RCON_PORT"}` | Game Port: 1000, RCON_PORT: 1001|
| Arma 3 | Game port, Game port +1 for Steam Query, Game port + 2 for Steam Port, and Game port +4 for BattleEye | `{1:"NONE", 2:"NONE", 4:"NONE"}` | Game Port: 1000, Additional Ports: 1001, 1002, 1004 |
| Unturned | Game port, Game port +1 and Game port +2 | `{1:"NONE", 2:"NONE"}` | Game Port: 1000, Additional Ports: 1001, 1002 |
| Project Zomboid | Game Port, Steam port and an additional port for every player. Let's say we want 10 additional ports for 10 players. | `{1:"STEAM_PORT", 2:"NONE", 3:"NONE", 4:"NONE", 5:"NONE", 6:"NONE", 7:"NONE", 8:"NONE", 9:"NONE", 10:"NONE", 11:"NONE"}` | Game Port: 1000, Steam Port: 1001, Additional Ports: 1002, 1003, 1004, 1005, 1006, 1007, 1008, 1009, 1010, 1011|

**What does "NONE" mean?**
"NONE" means you want to assign the additional port to the server, but it doesn't need to be assigned to a server parameter. If instead you want to add a +1 port and assign it to the parameter "RCON_PORT" then you'd use `{1:"RCON_PORT"}` for example.
Copy link
Contributor

@krenny krenny Sep 20, 2022

Choose a reason for hiding this comment

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

That json isn't valid.

keys should be strings
for example:
{"1":"RCON_PORT"}


(If they're available)

Note: I this option is set, it will override anything specified under "port_range" - Use one or the other, not both.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo


(If they're available)

Note: I this option is set, it will override anything specified under "port_range" - Use one or the other, not both.
Copy link
Contributor

Choose a reason for hiding this comment

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

It will not only override port_range but also every under deploy.
That should be dedicated IP, location and port_range

$main_allocation_id = $portDetails['id'];
$main_allocation_port = $port;
$found_all = true;
foreach($port_offsets_array as $key => $port_offset) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Foreach key and value are swapped
There the key (which you use as port offset) is used as parameter name and the value (which you use as parameter name) is used as port offset.

Comment on lines +829 to +831
// Failed to find available set of ports based on requirements
logModuleCall("WISP-WHMCS", "Failed to find available ports!", "", "");
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

that shouldn't be in the for loop

@mason-rogers
Copy link
Contributor

Thank you so much for laying the foundation for this - I've made a new PR within wisp-gg/whmcs which carries on your work, since I can't edit your repo/pr obviously.

Closing in favour of #8

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.

6 participants