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 the limits for checking the VNC console. #906

Merged
merged 1 commit into from Jan 16, 2018

Conversation

Projects
None yet
5 participants
@dasantiago
Contributor

dasantiago commented Jan 12, 2018

The limits, although were big enough for most of the tests, were not big enough
for xen tests or when the machines were under heavy load.

Limits increased 3 times.

It's now possible to control this values by using the vars _CHKSEL_RATE_WAIT_TIME and _CHKSEL_RATE_HITS

The checking function was also changed to make sure that:

  • will wait the CHKSEL_RATE_WAIT_TIME. Before if it hit the _CHKSEL_RATE_HITS, it would consider dead

  • All the FD in the bucket need to achieve the _CHKSEL_RATE_HITS, not just one. This in conjunction with the previous point will allow the system to recover if another FD suddenly starts responding.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 12, 2018

Coverage Status

Coverage increased (+0.01%) to 54.667% when pulling 9cb8680 on dasantiago:capture_loop into 5f24243 on os-autoinst:master.

coveralls commented Jan 12, 2018

Coverage Status

Coverage increased (+0.01%) to 54.667% when pulling 9cb8680 on dasantiago:capture_loop into 5f24243 on os-autoinst:master.

@mnowaksuse

This comment has been minimized.

Show comment
Hide comment
@mnowaksuse

mnowaksuse Jan 12, 2018

Contributor

Great idea to have it configurable. I'd like the variable name to be trimmed of the OS_AUTOINST_ prefix, it's quite unusual to have it and it's quite long anyways. Also document the variables somewhere, e.g. in docs/backend_vars.asciidoc.

Contributor

mnowaksuse commented Jan 12, 2018

Great idea to have it configurable. I'd like the variable name to be trimmed of the OS_AUTOINST_ prefix, it's quite unusual to have it and it's quite long anyways. Also document the variables somewhere, e.g. in docs/backend_vars.asciidoc.

@dasantiago dasantiago changed the title from Increase the limits for checking the VNC console. to WIP: Increase the limits for checking the VNC console. Jan 12, 2018

@coolo

This comment has been minimized.

Show comment
Hide comment
@coolo

coolo Jan 14, 2018

Contributor

But as I mentioned in other PRs before, I want (new) variables that affect isotovideo's internals to be prefixed. For EXIT_AFTER_SCHEDULE we agreed on a _ - so I'd be fine with _BUCKET_TIME_SIZE

The question I have though is: how many more buckets will we have over time. So perhaps instead of _BUCKET, make the variable short and hard to spell, but document it. As in _SKBTS - ok, a bit longer :)

Contributor

coolo commented Jan 14, 2018

But as I mentioned in other PRs before, I want (new) variables that affect isotovideo's internals to be prefixed. For EXIT_AFTER_SCHEDULE we agreed on a _ - so I'd be fine with _BUCKET_TIME_SIZE

The question I have though is: how many more buckets will we have over time. So perhaps instead of _BUCKET, make the variable short and hard to spell, but document it. As in _SKBTS - ok, a bit longer :)

Increase the limits for checking the VNC console.
The limits, although were big enough for most of the tests, were not big enough
for xen tests or when the machines were under heavy load.

Limits increased 3 times.

It's now possible to control this values by using the vars _CHKSEL_RATE_WAIT_TIME
and _CHKSEL_RATE_HITS

@dasantiago dasantiago changed the title from WIP: Increase the limits for checking the VNC console. to Increase the limits for checking the VNC console. Jan 15, 2018

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 15, 2018

Coverage Status

Coverage increased (+0.03%) to 54.686% when pulling e6b8a34 on dasantiago:capture_loop into 7f2bd40 on os-autoinst:master.

coveralls commented Jan 15, 2018

Coverage Status

Coverage increased (+0.03%) to 54.686% when pulling e6b8a34 on dasantiago:capture_loop into 7f2bd40 on os-autoinst:master.

@coolo

This comment has been minimized.

Show comment
Hide comment
@coolo

coolo Jan 16, 2018

Contributor

I have second thoughts, if making it configurable is smart at all. 30 seconds are a lot - still not enough for xen?

Contributor

coolo commented Jan 16, 2018

I have second thoughts, if making it configurable is smart at all. 30 seconds are a lot - still not enough for xen?

@coolo

This comment has been minimized.

Show comment
Hide comment
@coolo

coolo Jan 16, 2018

Contributor

But let's merge it with the _ prefix it should be clear it's for rare cases

Contributor

coolo commented Jan 16, 2018

But let's merge it with the _ prefix it should be clear it's for rare cases

@coolo coolo merged commit 6d3a09a into os-autoinst:master Jan 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 54.686%
Details
@dasantiago

This comment has been minimized.

Show comment
Hide comment
@dasantiago

dasantiago Jan 17, 2018

Contributor

Just to inform that the tests are now passing:
https://openqa.suse.de/tests/1394398

Contributor

dasantiago commented Jan 17, 2018

Just to inform that the tests are now passing:
https://openqa.suse.de/tests/1394398

@@ -234,7 +237,8 @@ sub run_capture_loop {
if (fileno $fh && fileno $fh != -1) {
# Very high limits! On a working socket, the maximum hits per 10 seconds will be around 60.
# The maximum hits per 10 seconds saw on a half open socket was >100k
if (update_time_bucket($buckets, 10, fileno $fh) > 5_000) {
# if (update_time_bucket($buckets, $bucket_time_size, fileno $fh) > $bucket_hit_size) {

This comment has been minimized.

@okurz

okurz Jan 21, 2018

Member

what's with this line now?

@okurz

okurz Jan 21, 2018

Member

what's with this line now?

This comment has been minimized.

@dasantiago

dasantiago Jan 22, 2018

Contributor

it's commented, it shouldn't be there.

@dasantiago

dasantiago Jan 22, 2018

Contributor

it's commented, it shouldn't be there.

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