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

Check ports allocation for NS8 Modules #742

Merged
merged 10 commits into from
Nov 6, 2024
Merged

Conversation

tommaso-ascani
Copy link
Contributor

@tommaso-ascani tommaso-ascani commented Oct 30, 2024

This pull request adds a check to the port allocation library to ensure modules don’t request more ports than allowed. When a module is installed, its maximum required TCP and UDP ports are stored in Redis. Then, when allocating new ports, the system checks that the total number of allocated ports plus the requested ports doesn’t exceed this limit. This prevents over-allocation and keeps port usage within safe limits.

Refs NethServer/dev#7092

Copy link
Member

@DavidePrincipi DavidePrincipi left a comment

Choose a reason for hiding this comment

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

Some thoughts to discuss together.

  • The PR makes the library a Redis client.
  • The check is required only when a module (not the cluster) agent invokes node/allocate-ports.
  • New and existing modules instances, only those with the new portsadm role, are interested by this limit.

@tommaso-ascani tommaso-ascani force-pushed the check-ports-demand-1 branch 3 times, most recently from f23137f to 8a24475 Compare November 4, 2024 14:51
@tommaso-ascani tommaso-ascani marked this pull request as ready for review November 4, 2024 16:55
Copy link
Member

@DavidePrincipi DavidePrincipi left a comment

Choose a reason for hiding this comment

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

More validation error details

github-actions bot pushed a commit that referenced this pull request Nov 5, 2024
Copy link
Member

@DavidePrincipi DavidePrincipi left a comment

Choose a reason for hiding this comment

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

There is still a reference to a removed function

github-actions bot pushed a commit that referenced this pull request Nov 6, 2024
github-actions bot pushed a commit that referenced this pull request Nov 6, 2024
Copy link
Member

@DavidePrincipi DavidePrincipi left a comment

Choose a reason for hiding this comment

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

There is an issue with the max_ports_exceeded computation.

github-actions bot pushed a commit that referenced this pull request Nov 6, 2024
The message is sent to the system journal and also in the API error
stream, which is visible in the Python exception raised by
agent.allocate_ports(), too.
github-actions bot pushed a commit that referenced this pull request Nov 6, 2024
Return an empty list if the module has no ports allocation.
Suppress module-not-found warnings when a module is removed.
github-actions bot pushed a commit that referenced this pull request Nov 6, 2024
Copy link
Member

@DavidePrincipi DavidePrincipi left a comment

Choose a reason for hiding this comment

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

LGTM

@DavidePrincipi DavidePrincipi merged commit a724dba into main Nov 6, 2024
3 checks passed
@DavidePrincipi DavidePrincipi deleted the check-ports-demand-1 branch November 6, 2024 16:26
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.

2 participants