client: fix for jetty session timeout

[marcaurele]

Description

Related to #2329 (comment) the session timeout is not taken into account. This change creates a session handler explicitly and the timeout value should be correctly taken into account.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

How Has This Been Tested?

This has not been tested yet.

[andrijapanicsb]

thx! let me test that

[andrijapanicsb]

@blueorangutan package

[blueorangutan]

@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-334

[andrijapanicsb]

@blueorangutan test matrix

[blueorangutan]

@andrijapanicsb a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

[nvazquez]

Hi @marcaurele thanks for the changes. I tested this but unfortunately I couldn't see any difference after changing the value on server.properties and restarting the management server

[blueorangutan]

Trillian test result (tid-438)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34872 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3658-t438-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[blueorangutan]

Trillian test result (tid-439)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35868 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3658-t439-vmware-65u2.zip
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[andrijapanicsb]

Unfortunately, not working @marcaurele 😢

[marcaurele]

I did some testing on a simple server and I had to set the timeout after the server was started. I pushed an update, let me know if it's working now @andrijapanicsb

[andrijapanicsb]

thx @marcaurele.

@blueorangutan package

[blueorangutan]

@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-335

[nvazquez]

Thanks @marcaurele, this is working now but I think it can be improved a bit:

Instead of creating a pair to pass the sessionHandler, it can be defined as a variable and set the timeout after the server starts, reducing the lines of code:

private SessionHandler sessionHandler;

...

private HandlerCollection createHandlers() {
   ...
   sessionHandler = webApp.getSessionHandler();
   ...
}

public void start() {
   ...
   server.start();
   // Must set the session timeout after the server has started
   sessionHandler.setMaxInactiveInterval(sessionTimeout * 60);
   ...
}

[andrijapanicsb]

@marcaurele @nvazquez - I've tested both your solutions - it works fine! (tried both setting session to 2minutes and 45 minutes) - so all good!

[nvazquez]

@marcaurele please check PR #3663

[marcaurele]

@nvazquez IMO it does not look well coded to keep a reference of the SessionHandler as a class member to only skip having to return this pair of objects.
Then, I don't understand why you open a new PR to perform those changes? Is that so urgent? Aren't your comment enough?

[nvazquez]

@marcaurele no urgency, was trying to propose an improvement and tried with the comments but thought those were not being added until I click Submit on the review. So I created the PR for clarity but didn't realise the comments were already added, please discard them. Thanks for the fix

[andrijapanicsb]

Guys, please let me know once you have agreed on the way forward, so I can merge this one.

[nvazquez]

I have closed the other PR, I'm ok with this one

[marcaurele]

@nvazquez I wish we could return multiple argument without creating a custom class in Java. Thanks for taking my point in consideration.

[marcaurele]

Rebase done to only leave the good commit. Good for review & merge on my side.

[nvazquez]

@blueorangutan package

[blueorangutan]

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-339

[nvazquez]

@blueorangutan test matrix

[blueorangutan]

@nvazquez a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

[blueorangutan]

Trillian test result (tid-452)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42011 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3658-t452-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[andrijapanicsb]

the latest code is NOT ok.
I've set timeout to 2 minutes and I don't get logged out on 2m30sec (nor on 3m30sec nor on 4m15sec - tested just in case...)

[yadvr]

@marcaurele can you try https://stackoverflow.com/a/56091476

[marcaurele]

@andrijapanicsb weird as I don't see any difference so far from the previous 2 commits. Can you double check you're using the right branch and cleared up the compiled classes of the client module, just in case.

@rhtyd that's what it's doing, but I found out that the session must be set after doing the server.start(). I modified the jetty example embbeded project to try out.

[andrijapanicsb]

my bad, give me 2 minutes...

[andrijapanicsb]

all good @marcaurele.

LGTM from my side (manual testing).

[nvazquez]

LGTM

[andrijapanicsb]

merging based on 2 LGTM, manual testing and automated test results.

thx @marcaurele !

[marcaurele]

For the record, this is the small embedded app to validate: https://github.com/marcaurele/embedded-jetty-uber-jar/tree/session-test