Respect port provided by the client && Switch from urllib.urlparse to simple substring replacement. by todor-ivanov · Pull Request #10364 · dmwm/WMCore

todor-ivanov · 2021-03-22T17:18:51Z

Status

ready

Description

While we developing the port forwarding logic, we did not take into account the cases when the client actually provides a port as part of the url. Along the way few measurements happened regarding the efficiency of the two approaches:

Using the urllib.urlparse to parse the whole url in components and just change what is needed
Using the simplest possible sub string substitution.

The current change switches back to the sub string replacement mechanism, which implicitly satisfies the major requirement for the issue - Not to change ports provided with the url by the client.

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

No

External dependencies / deployment changes

No

todor-ivanov · 2021-03-22T17:22:16Z

Hi @amaltaro, This is a change enclosed completely in the PortForward file in Utils. I tried to not touch the already tested code. But in case you think (based on the measurements we did and pasted in the issue) we'd better completely scratch the decorator, I am ready to do so.

cmsdmwmbot · 2021-03-22T17:38:29Z

Jenkins results:

Unit tests: failed
- 10 new failures
Pylint check: failed
- 1 warnings and errors that must be fixed
- 2 warnings
- 6 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
- 1 comments to review
Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11525/artifact/artifacts/PullRequestReport.html

mapellidario

I do like the idea of getting rid of urlllib.parse, kudos!

However I think I missed that bytes is not imported when reviewing #10330 and added some comments.

Thanks Todor, sorry for disturbing with an unrequested review!

cmsdmwmbot · 2021-03-22T22:02:28Z

Jenkins results:

Unit tests: failed
- 2 new failures
Pylint check: succeeded
- 2 warnings
- 4 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11529/artifact/artifacts/PullRequestReport.html

amaltaro

Todor, once you fix those unit tests, make another review request please.

todor-ivanov · 2021-03-23T07:29:13Z

They are fixed @amaltaro , It is only the jenkins run that we are waiting for.

cmsdmwmbot · 2021-03-23T07:40:35Z

Jenkins results:

Unit tests: failed
- 1 new failures
Pylint check: failed
- 2 warnings and errors that must be fixed
- 14 warnings
- 24 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
- 12 comments to review
Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11530/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2021-03-23T08:02:34Z

Jenkins results:

Unit tests: failed
- 1 new failures
Pylint check: failed
- 14 warnings
- 24 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
- 12 comments to review
Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11531/artifact/artifacts/PullRequestReport.html

todor-ivanov · 2021-03-23T08:19:17Z

@amaltaro I cannot figure out why this one fails:

WMCore_t.DataStructs_t.LumiList_t.LumiListTest:testURL changed from success to error

I simply fail to see how my changes affect it.

amaltaro · 2021-03-23T09:17:44Z

Todor, I would suggest you to look at what gets reported in jenkins (it's also a good exercise!), here is some help to navigate there:
https://github.com/dmwm/WMCore/wiki/Understanding-Jenkins#unit-tests

and also try to reproduce this error with your docker unit tests.

I haven't looked at it myself, so it could be related, or not ;)

todor-ivanov · 2021-03-23T10:13:32Z

@amaltaro I already did these both - examining jenkins in deep and running the failing test with nose in my private instance, and still cannot relate my changes to the failing test in a module few levels up.

amaltaro · 2021-03-23T10:51:11Z

Yes, it looks like that file is no longer available; or the web server is having issues to serve that file. I made a new GH issue such that we can fix that unit test in the future: #10367

todor-ivanov · 2021-03-23T10:57:17Z

@amaltaro it is indeed an error due to a hard coded url [1] in the test and this one if you try to browse it yourself it fails with the HTTTP erro 500 - internal server error. And if I need to be 100% strict on the fact that my change has no relation to this error here is what is passed to urllib directly (url not touched by my change at all) data obtained with pdb [2]

[1]
https://cms-service-dqm.web.cern.ch/cms-service-dqm/CAF/certification/Collisions12/8TeV/Reprocessing/Cert_190456-195530_8TeV_08Jun2012ReReco_Collisions12_JSON.txt

[2]

> ~/unittest/unittestdeploy/wmagent/1.4.1.pre5-comp10/sw/slc7_amd64_gcc630/external/py2-future/0.18.2/lib/python2.7/site-packages/future/backports/urllib/request.py(157)urlopen()
-> else: capath = None
(Pdb) l
152  	_opener = None
153  	def urlopen(url, data=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT, **_3to2kwargs):
154  	    if 'cadefault' in _3to2kwargs: cadefault = _3to2kwargs['cadefault']; del _3to2kwargs['cadefault']
155  	    else: cadefault = False
156  	    if 'capath' in _3to2kwargs: capath = _3to2kwargs['capath']; del _3to2kwargs['capath']
157    	    else: capath = None
158  	    if 'cafile' in _3to2kwargs: cafile = _3to2kwargs['cafile']; del _3to2kwargs['cafile']
159  	    else: cafile = None
160  	    global _opener
161  	    if cafile or capath or cadefault:
162  	        if not _have_ssl:
(Pdb) 
163  	            raise ValueError('SSL support not available')
164  	        context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
165  	        context.options |= ssl.OP_NO_SSLv2
166  	        context.verify_mode = ssl.CERT_REQUIRED
167  	        if cafile or capath:
168  	            context.load_verify_locations(cafile, capath)
169  	        else:
170  	            context.set_default_verify_paths()
171  	        https_handler = HTTPSHandler(context=context, check_hostname=True)
172  	        opener = build_opener(https_handler)
173  	    elif _opener is None:
174  	        _opener = opener = build_opener()
175  	    else:
176  	        opener = _opener
177  ->	    return opener.open(url, data, timeout)


(Pdb) url
'https://cms-service-dqm.web.cern.ch/cms-service-dqm/CAF/certification/Collisions12/8TeV/Reprocessing/Cert_190456-195530_8TeV_08Jun2012ReReco_Collisions12_JSON.txt'
(Pdb)

todor-ivanov · 2021-03-23T10:59:23Z

Sorry @amaltaro - I missed your previous comment. Thanks for creating the issue!

cmsdmwmbot · 2021-03-23T11:34:52Z

Jenkins results:

Unit tests: failed
- 1 new failures
Pylint check: failed
- 14 warnings
- 26 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
- 14 comments to review
Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11537/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2021-03-23T11:36:59Z

Jenkins results:

Unit tests: failed
- 1 new failures
Pylint check: failed
- 14 warnings
- 26 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
- 14 comments to review
Python3 compatibility checks: succeeded

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11538/artifact/artifacts/PullRequestReport.html

amaltaro · 2021-03-23T12:52:40Z

This is looking much better now, timing is very close between the different approaches as well:

(PY2) alannewmacpro:ProductionTools amaltaro$ python timePortForward.py 1000000
Going to make 1000000 url conversions
testDecorator received payload: 1000000
Finished single function call in 1.86168313026 secs

testFuncDecorator received payload: 1000000
Finished single function call in 2.5859568119 secs

testSimpleFunc received payload: 1000000
Finished single function call in 1.98184108734 secs

… simple substring replacement. Remove unneeded imports Add string prefix && Remove logger from PortForward class. Import bytes

Unittests Unittests

cmsdmwmbot · 2021-03-23T13:46:59Z

Jenkins results:

Unit tests: failed
- 1 new failures
- 3 tests deleted
Pylint check: failed
- 14 warnings
- 26 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
- 14 comments to review
Python3 compatibility checks: succeeded
- there are suggested fixes for newer python3 idioms

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11540/artifact/artifacts/PullRequestReport.html

cmsdmwmbot · 2021-03-23T13:48:49Z

Jenkins results:

Unit tests: failed
- 1 new failures
- 3 tests deleted
Pylint check: failed
- 14 warnings
- 26 comments to review
Pylint py3k check: succeeded
- 0 errors and warnings that should be fixed
- 0 warnings
- 0 comments to review
Pycodestyle check: succeeded
- 14 comments to review
Python3 compatibility checks: succeeded
- there are suggested fixes for newer python3 idioms

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/11541/artifact/artifacts/PullRequestReport.html

mapellidario

Thanks Todor for the changes! :) Looks good to me now!

amaltaro

Thanks Todor, it looks good to me.

todor-ivanov requested a review from amaltaro March 22, 2021 17:18

mapellidario requested changes Mar 22, 2021

View reviewed changes

Comment thread src/python/Utils/PortForward.py

Comment thread src/python/Utils/PortForward.py Outdated

Comment thread src/python/Utils/PortForward.py Outdated

Comment thread src/python/Utils/PortForward.py

todor-ivanov force-pushed the feature_RespectPort_fix-10359 branch from 3954130 to 0adf701 Compare March 22, 2021 21:36

amaltaro reviewed Mar 23, 2021

View reviewed changes

todor-ivanov requested a review from amaltaro March 23, 2021 07:27

todor-ivanov force-pushed the feature_RespectPort_fix-10359 branch from 796a13e to a7b03ed Compare March 23, 2021 07:48

todor-ivanov force-pushed the feature_RespectPort_fix-10359 branch from a451fa2 to f9ac993 Compare March 23, 2021 11:20

todor-ivanov added 2 commits March 23, 2021 14:34

Respect port provided by the client && Switch from urllib.urlparse to…

0f780fb

… simple substring replacement. Remove unneeded imports Add string prefix && Remove logger from PortForward class. Import bytes

Unittests

8f0d395

Unittests Unittests

todor-ivanov force-pushed the feature_RespectPort_fix-10359 branch from 33dc220 to 8f0d395 Compare March 23, 2021 13:34

mapellidario self-requested a review March 23, 2021 14:02

mapellidario approved these changes Mar 23, 2021

View reviewed changes

amaltaro approved these changes Mar 23, 2021

View reviewed changes

amaltaro merged commit e7e5d91 into dmwm:master Mar 23, 2021

Conversation

todor-ivanov commented Mar 22, 2021

Status

Description

Is it backward compatible (if not, which system it affects?)

Related PRs

External dependencies / deployment changes

Uh oh!

todor-ivanov commented Mar 22, 2021

Uh oh!

cmsdmwmbot commented Mar 22, 2021

Uh oh!

mapellidario left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmsdmwmbot commented Mar 22, 2021

Uh oh!

amaltaro left a comment

Choose a reason for hiding this comment

Uh oh!

todor-ivanov commented Mar 23, 2021

Uh oh!

cmsdmwmbot commented Mar 23, 2021

Uh oh!

cmsdmwmbot commented Mar 23, 2021

Uh oh!

todor-ivanov commented Mar 23, 2021

Uh oh!

amaltaro commented Mar 23, 2021

Uh oh!

todor-ivanov commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaltaro commented Mar 23, 2021

Uh oh!

todor-ivanov commented Mar 23, 2021

Uh oh!

todor-ivanov commented Mar 23, 2021

Uh oh!

cmsdmwmbot commented Mar 23, 2021

Uh oh!

cmsdmwmbot commented Mar 23, 2021

Uh oh!

amaltaro commented Mar 23, 2021

Uh oh!

cmsdmwmbot commented Mar 23, 2021

Uh oh!

cmsdmwmbot commented Mar 23, 2021

Uh oh!

mapellidario left a comment

Choose a reason for hiding this comment

Uh oh!

amaltaro left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

todor-ivanov commented Mar 23, 2021 •

edited

Loading