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

Fix missing or double output in autoinst-log.txt with partial revert #1161

Merged
merged 1 commit into from Jun 4, 2019

Conversation

@okurz
Copy link
Member

commented Jun 4, 2019

This reverts commit 85a4003 and
partially reverts commit fe70160 as
problems in the output handling in conjunction with openQA has been
encountered. The general code interface is preserved and should be
overhauled when the implementation is completed.

Related ticket: https://progress.opensuse.org/issues/52235

Was: Reverts #1159 to prevent the problem as described in https://progress.opensuse.org/issues/52235#note-5 until a proper solution has been found.

@okurz

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 4, 2019

Codecov Report

Merging #1161 into master will increase coverage by 1.17%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1161      +/-   ##
==========================================
+ Coverage   38.27%   39.44%   +1.17%     
==========================================
  Files          40       40              
  Lines        4902     4949      +47     
  Branches      834      854      +20     
==========================================
+ Hits         1876     1952      +76     
+ Misses       2694     2663      -31     
- Partials      332      334       +2
Impacted Files Coverage Δ
basetest.pm 11.21% <0%> (+0.84%) ⬆️
bmwqemu.pm 53.84% <83.33%> (-1.78%) ⬇️
commands.pm 11.95% <0%> (-1.73%) ⬇️
needle.pm 50.85% <0%> (+0.57%) ⬆️
backend/qemu.pm 32.59% <0%> (+1.29%) ⬆️
consoles/VNC.pm 44.27% <0%> (+1.72%) ⬆️
consoles/console.pm 52.17% <0%> (+2.17%) ⬆️
consoles/vnc_base.pm 42.7% <0%> (+3.12%) ⬆️
backend/baseclass.pm 58.86% <0%> (+8.13%) ⬆️
consoles/network_console.pm 91.66% <0%> (+8.33%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5350709...a2fb56d. Read the comment docs.

@foursixnine

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@asdil12 could you work on improving the fullstack tests too while fixing this part of the problem?

@asdil12
asdil12 approved these changes Jun 4, 2019
@coolo

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Actually I would prefer reverting the complete feature - until it's done correctly

@foursixnine

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Actually I would prefer reverting the complete feature - until it's done correctly

+1

@asdil12

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Actually I would prefer reverting the complete feature - until it's done correctly

-1

Too much work and it would break tests if we don't also revert it in the test distri.
Also it is not the feature that is broken but isotovideo being broken and just noone noticing before.

@coolo

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

No. your feature broke it by depending on a debug log

@coolo

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

and we can leave the API in place - as NOP

@Martchus

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Seems like we need to revert #1159 and the changes introducing the bug that PR is attempting to fix. I guess that's also what @coolo means.

And next time test it please within the worker.

@coolo

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

No, 1159 was already an attempt to cover up. Even earlier

@pevik

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Thanks oliver for this PR. This revert is really needed as #1159 breaks (at least some) s390x builds:
http://quasar.suse.cz/tests/2853#step/bootloader_zkvm/28

@asdil12

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Reverting doesn't seem to be needed anymore with os-autoinst/openQA#2092

@asdil12

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Thanks oliver for this PR. This revert is really needed as #1159 breaks (at least some) s390x builds:
http://quasar.suse.cz/tests/2853#step/bootloader_zkvm/28

@pevik
How do you relate this fail to that PR?
Is there something in that error message that I overlooked?

@pevik

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@asdil12 OK, with manual change from https://github.com/os-autoinst/openQA/pull/2092/files it's working again also for s390x builds. Thanks for pointing it out.

@pevik pevik referenced this pull request Jun 4, 2019
@okurz okurz force-pushed the revert-1159-logstderr branch from 1d667cf to c0de425 Jun 4, 2019
@okurz okurz changed the title Revert "Always log to autoinst-log.txt" to prevent duplication of every single output line Fix missing or double output in autoinst-log.txt with partial revert Jun 4, 2019
@okurz

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

I don't know what I am doing but I tried to "disarm #1149" as suggested in os-autoinst/openQA#2093 (comment)

@coolo
coolo approved these changes Jun 4, 2019
Copy link
Contributor

left a comment

this is a little larger revert than what I considered necessary, but git preserves the code

This reverts commit 85a4003 and
partially reverts commit fe70160 as
problems in the output handling in conjunction with openQA has been
encountered. The general code interface is preserved and should be
overhauled when the implementation is completed.

Related ticket: https://progress.opensuse.org/issues/52235
@okurz okurz force-pushed the revert-1159-logstderr branch from c0de425 to a2fb56d Jun 4, 2019
@okurz

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

yeah I tried to keep the "Try::Tiny" changes in place but this would fail the existing implementation and tests of the serial log error detection.

@asdil12 I hope you are also ok with it

@okurz

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

ok, all included tests are passed. I could reproduce the duplication problem using a local openQA instance, e.g. by running sudo -u _openqa-worker ~/local/os-autoinst/openQA/script/worker --instance 1 --verbose --no-cleanup and monitoring /var/lib/openqa/pool/1/autoinst-log.txt and verified that this PR is fine regarding output by running it as worker with sudo -u _openqa-worker ~/local/os-autoinst/openQA/script/worker --instance 1 --isotovideo ~/local/os-autoinst/os-autoinst/isotovideo --verbose --no-cleanup confirming that the logfile looks fine, calling isotovideo -d on a local vars.json file asserting that the output is there as well as isotovideo without -d asserting that no debug output is there.

@okurz okurz merged commit ba43d1c into master Jun 4, 2019
4 checks passed
4 checks passed
codecov/patch 50% of diff hit (target 38.27%)
Details
codecov/project 39.44% (target 39%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@okurz okurz deleted the revert-1159-logstderr branch Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.