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

Increase flexibility of network port mapping in clustermq #329

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

michaelmayer2
Copy link
Contributor

At the moment, the ports used by clustermq are hard coded. By default, ports between 6000 and 9999 are used by clustermq and ports between 50000 and 55000 are used by the ssh connector for forwarding ssh sessions.

This PR will introduce two new options to clustermq, i.e.

  • clustermq.port.range
  • clustermq.ssh.hpc_fwd_port.range

both of which will make working with clustermq in environments with networking firewalls much easier.

Copy link
Owner

@mschubert mschubert 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! Please see my comments

@michaelmayer2 michaelmayer2 requested a review from mschubert March 28, 2024 10:42
R/qsys_ssh.r Outdated
Comment on lines 58 to 66
args$ssh.hpc_fwd_port
hpc_fwd_port = getOption(
"clustermq.ssh.hpc_fwd_port",
sample((50000:55000),1)
)
args$ssh.hpc_fwd_port=ifelse(length(hpc_fwd_port)==1,
hpc_fwd_port,
sample(hpc_fwd_port,1)
)
Copy link
Owner

Choose a reason for hiding this comment

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

Why not just:

args$ssh.hpc_fwd_port = sample(getOption("clustermq.ssh.hpc_fwd_port", 50000:55000), 1)

Copy link
Contributor Author

@michaelmayer2 michaelmayer2 Mar 28, 2024

Choose a reason for hiding this comment

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

That would be great but if we really want to allow both single ports and port ranges, then your approach would lead to

#default
> sample(getOption("clustermq.ssh.hpc_fwd_port", 50000:55000), 1)
[1] 53710
#single custom port
> options(clustermq.ssh.hpc_fwd_port=1024)
> sample(getOption("clustermq.ssh.hpc_fwd_port", 50000:55000), 1)
[1] 437
#custom port range
> options(clustermq.ssh.hpc_fwd_port=1024:2048)
> sample(getOption("clustermq.ssh.hpc_fwd_port", 50000:55000), 1)
[1] 1731

the problem is that the single port would then be used as single argument to the sample command and then the actual port would be a sample of 1:clustermq.ssh.hpc_fwd_port

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, you're right. I forgot that sample with a single integer samples any integer up to and including this point. In that case I'd prefer:

args$ssh.hpc_fwd_port = getOption("clustermq.ssh.hpc_fwd_port", 50000:55000)
if (length(args$ssh.hpc_fwd_port) > 1)
    args$ssh.hpc_fwd_port = sample(args$ssh.hpc_fwd_port, 1)

Copy link
Contributor Author

@michaelmayer2 michaelmayer2 Apr 3, 2024

Choose a reason for hiding this comment

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

That's much cleaner than my algorithm - thanks for this ! I have added this as a commit now in 7995621.

@michaelmayer2 michaelmayer2 requested a review from mschubert April 2, 2024 12:40
R/qsys_ssh.r Outdated
args$ssh.hpc_fwd_port=getOption("clustermq.ssh.hpc_fwd_port", 50000:55000)
if (length(args$ssh.hpc_fwd_port) > 1)
args$ssh.hpc_fwd_port = sample(args$ssh.hpc_fwd_port, 1)
utils::modifyList(private$defaults, args)
Copy link
Owner

Choose a reason for hiding this comment

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

please still fix tab indentation on L61 -> should be spaces

@mschubert mschubert merged commit e1fc7e6 into mschubert:master Apr 4, 2024
@mschubert
Copy link
Owner

Thank you! 🎉

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