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

Expanding HLT Configuration Proxy Support and Adding Tunnel Support #40436

Closed

Conversation

Sam-Harper
Copy link
Contributor

@Sam-Harper Sam-Harper commented Jan 6, 2023

PR description:

Confdb access is restricted to within the CERN network and thus for users outside CERN they must connect via tunnels.

Currently hltGetConfiguration and hltConfigFromDB allow the use of a socks proxy to allow users outside CERN to download menus. However the IPs need to be harcoded due to DNS issues (an issue with Java which as far as we are aware is unsolvable).

Only the offline DB had its IPs hardcoded, we now add IPs for the online (p5 only) and online mirror (accessable outside p5) so users can also use a proxy for these dbs.

Finally to have a work around in the future, support for direct tunnels where a user specifically forwards a given port on their machine to a db has been added so we can always fall back on this.

Aim to back port to 12_4, 12_5, 12_6 and 10_6 (I believe these are all the releases users are using for analysis).

edit: probably not 10_6, it hasn't received the other updates necessary unless there is a strong push for it.

PR validation:

Configs from online/ online-mirror and offline were downloaded at RAL using both direct tunnels and a proxy server.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40436/33577

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

A new Pull Request was created by @Sam-Harper (Sam Harper) for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@Sam-Harper
Copy link
Contributor Author

please test

Copy link
Contributor

@missirol missirol 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 this update! One change is maybe needed, the other comments are optional (cosmetics).



tunnel = False
tunnel_port = "10121"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self (not for this PR): we should find a way to set the default values only once (e.g. in options.py), without having to hard-code more than once.

@Sam-Harper
Copy link
Contributor Author

Thanks Marino for the review!

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40436/33585

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

Pull request #40436 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

@missirol
Copy link
Contributor

missirol commented Jan 6, 2023

Thanks, Sam. I hate to be picky, but would you mind squashing things in 1 single commit?

@Sam-Harper Sam-Harper force-pushed the ConfdbProxyTunnels_1220pre2 branch from 092e78e to 9ffc399 Compare January 6, 2023 16:47
@Sam-Harper
Copy link
Contributor Author

no problem its better that way, done

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40436/33587

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

Pull request #40436 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

@Sam-Harper
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f4955b/29817/summary.html
COMMIT: 9ffc399
CMSSW: CMSSW_13_0_X_2023-01-06-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40436/29817/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555748
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555723
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@missirol
Copy link
Contributor

missirol commented Jan 7, 2023

I tested the db proxy/tunnel options inside the CERN network, and found no problems [*] [**]. On Monday, I can test this outside the CERN network (the T3 I have access to is under maintenance till then), and can then sign this PR.

I noticed a couple of things that could be added to this PR already, if possible. They are in the following diff.

diff --git a/HLTrigger/Configuration/python/Tools/confdbOfflineConverter.py b/HLTrigger/Configuration/python/Tools/confdbOfflineConverter.py
index 2b28e7e2624..a89a5aa0b88 100644
--- a/HLTrigger/Configuration/python/Tools/confdbOfflineConverter.py
+++ b/HLTrigger/Configuration/python/Tools/confdbOfflineConverter.py
@@ -242,6 +242,14 @@ def help():
         --modules <p1[,p2]>         (include modules, referenced or not!)
         --blocks <m1::p1[,p2][,m2]> (generate parameter blocks)
 
+        Options to connect to target db via SOCKS proxy, or direct tunnel:
+          [the options --dbproxy and --dbtunnel are mutually exclusive]
+        --dbproxy                   (use a SOCKS proxy to connect outside CERN network [default: False])
+        --dbproxyhost <hostname>    (host of the SOCKS proxy [default: "localhost"])
+        --dbproxyport <port>        (port of the SOCKS proxy [default: 8080])
+        --dbtunnel                  (use direct tunnel to connect outside CERN network [default: False])
+        --dbtunnelport <port>       (port when using a direct tunnel on localhost [default: 10121])
+
         --verbose                   (print additional details)
 """)
 
@@ -294,8 +302,8 @@ def main():
         version = 'v3-test'
         db      = 'dev'
         args.remove('--v3-test')
-    
-    proxy=False
+
+    proxy = False
     proxy_host = "localhost"
     proxy_port = "8080"
     if '--dbproxy' in args:
@@ -347,11 +355,11 @@ def main():
             sys.exit(1)
 
     converter = OfflineConverter(version = version, database = db, verbose = verbose,
-                                 proxy = proxy, proxyHost = proxy_host, proxyPort=proxy_port,
+                                 proxy = proxy, proxyHost = proxy_host, proxyPort = proxy_port,
                                  tunnel = tunnel, tunnelPort = tunnel_port)
     out, err = converter.query( * args )
     if 'ERROR' in err:
-        sys.stderr.write( "%s: error while retriving the HLT menu\n\n%s\n\n" % (sys.argv[0], err) )
+        sys.stderr.write( "%s: error while retrieving the HLT menu\n\n%s\n\n" % (sys.argv[0], err) )
         sys.exit(1)
     else:
         sys.stdout.write( out )
diff --git a/HLTrigger/Configuration/scripts/hltGetConfiguration b/HLTrigger/Configuration/scripts/hltGetConfiguration
index 587cfe25cbd..1a0a6aa338b 100755
--- a/HLTrigger/Configuration/scripts/hltGetConfiguration
+++ b/HLTrigger/Configuration/scripts/hltGetConfiguration
@@ -255,14 +255,14 @@ parser.add_argument('--help',
 # parse command line arguments and options
 config = parser.parse_args(namespace = options.HLTProcessOptions())
 
-# do not include db-proxy options in 1st-line comment
+# do not include db-proxy/tunnel options in 1st-line comment
 cmdArgs, skipNext = [], False
 for cmdArg in sys.argv:
   if skipNext:
     skipNext = False
     continue
-  if cmdArg.startswith('--dbproxy'):
-    if cmdArg.startswith('--dbproxyh') or cmdArg.startswith('--dbproxyp'):
+  if cmdArg.startswith('--dbproxy') or cmdArg.startswith('--dbtunnel'):
+    if cmdArg.startswith('--dbproxyh') or cmdArg.startswith('--dbproxyp') or cmdArg.startswith('--dbtunnelp'):
       skipNext = '=' not in cmdArg
     continue
   cmdArgs += [cmdArg]

[*] In my case, the direct tunnel works as long as I use port=10121 (as opposed to other port numbers). I was wondering if that's expected. Are the other ports 'blocked'?

[**] There is an edge case in hltConfigFromDB that could be fixed (having to do with using =), as this won't work

hltConfigFromDB --adg --configName /cdaq/physics/Run2022HI/v1.1.0/HLT/V11 --dbtunnel --dbtunnelport=10121

while the following does (courtesy of argparse)

hltGetConfiguration adg:/cdaq/physics/Run2022HI/v1.1.0/HLT/V11 --dbtunnel --dbtunnelport=10121

On the other hand, this 'limitation' already exists for dbproxy*, so I think it'd be okay to leave it for a future PR.

@Sam-Harper
Copy link
Contributor Author

[**] There is an edge case in hltConfigFromDB that could be fixed (having to do with using =), as this won't work

hltConfigFromDB --adg --configName /cdaq/physics/Run2022HI/v1.1.0/HLT/V11 --dbtunnel --dbtunnelport=10121
while the following does (courtesy of argparse)

hltGetConfiguration adg:/cdaq/physics/Run2022HI/v1.1.0/HLT/V11 --dbtunnel --dbtunnelport=10121

Thats sadly just a feature of hltConfigFromDB, it works as --option value not --option=value. Changing would apply to all arguements, eg --configName /cdaq/physics/Run2022HI/v1.1.0/HLT/V11 to --configName=/cdaq/physics/Run2022HI/v1.1.0/HLT/V11 . So as you say, another time.

[*] In my case, the direct tunnel works as long as I use port=10121 (as opposed to other port numbers). I was wondering if that's expected. Are the other ports 'blocked'?

hmm i tested with 10122 using -L 10122:<dbserver>:10121 and it worked?

I noticed a couple of things that could be added to this PR already, if possible. They are in the following diff.
Sure will impliment it monday

@missirol
Copy link
Contributor

missirol commented Jan 7, 2023

hmm i tested with 10122 using -L 10122:<dbserver>:10121 and it worked?

Ah, yes, that works: I was mistakenly changing both, e.g. 10122:<dbserver>:10122. Thanks!

@missirol
Copy link
Contributor

missirol commented Jan 9, 2023

Tested outside the CERN network, and works as expected.

@Sam-Harper
Copy link
Contributor Author

it is difficult for me to apply the last patch in the way I have set this up so I'll open a new PR shortly

@Sam-Harper
Copy link
Contributor Author

new PR: #40454

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40436/33610

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2023

Pull request #40436 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants