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 sporadic error in t/10-virtio_terminal.t #1870

Merged
merged 1 commit into from Nov 30, 2021

Conversation

cfconrad
Copy link
Contributor

@cfconrad cfconrad commented Nov 30, 2021

I wasn't able to reproce the problem, but this is what I guess.
We call $console->peak() before we wrote the data into the other side of
the pipe.
Now we use USR2 signal to sync between "sender" and "receiver".
Also calling flush() explicit, so we ensure data is not buffered by
perl.

Ticked: https://progress.opensuse.org/issues/94991
Issue: #1686

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #1870 (9055ea2) into master (ae6eed2) will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1870      +/-   ##
==========================================
+ Coverage   67.46%   67.56%   +0.10%     
==========================================
  Files          62       62              
  Lines        6885     6885              
==========================================
+ Hits         4645     4652       +7     
+ Misses       2240     2233       -7     
Impacted Files Coverage Δ
consoles/sshSerial.pm 100.00% <0.00%> (+24.13%) ⬆️

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 ae6eed2...9055ea2. Read the comment docs.

I wasn't able to reproce the problem, but this is what I guess.
We call `$console->peak()` before we wrote the data into the other side of
the pipe.
Now we use USR2 signal to sync between "sender" and "receiver".
Also calling `flush()` explicit, so we ensure data is not buffered by
perl.

Ticked: https://progress.opensuse.org/issues/94991
Issue: os-autoinst#1686
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

I suppose this could generally work but see the inline comments.

t/10-virtio_terminal.t Show resolved Hide resolved
t/10-virtio_terminal.t Show resolved Hide resolved
@okurz
Copy link
Member

okurz commented Nov 30, 2021

As the problem showed up on OBS and as you already changed the spec file the test is also executed on OBS. That's good. But as that was a sporadic problem I suggest to run much more often. E.g. you could try to run a for-loop within OBS checks temporarily as part of this PR. Or you create a custom OBS branch project checking out from git.

@cfconrad
Copy link
Contributor Author

Thats what I currently do in here: https://build.opensuse.org/package/show/home:cfconrad:branches:devel:openQA/os-autoinst
And I use this script:

LOOP=0

osc_r() {
    osc r | grep openSUSE_Factory
}

is_building() {
    test $(osc_r | awk '{print $4}' | grep -P 'building|blocked|scheduled|finished' | wc -l) -gt 0
}

is_failed() {
    test $(osc_r | awk '{print $4}' | grep -v succeeded | wc -l) -ne 0
}

osc_rebuild() {
    for i in openSUSE_Factory_PowerPC openSUSE_Factory_ARM openSUSE_Factory; do
        osc rebuild -r $i
    done
    sleep 10;
}

msg() {
    echo "[$LOOP]" $@
}

while true; do
    LOOP=$(( $LOOP + 1 ));
    while is_building; do 
        msg "Wait for builds are ready"
        osc_r
        sleep 1;
    done

    if is_failed; then
        msg "Something failed!!!"
        osc_r
        exit 1 
    fi

    osc_rebuild
done

I'm on loop 16. Any hints to improve this?

@okurz
Copy link
Member

okurz commented Nov 30, 2021

I'm on loop 16. Any hints to improve this?

Great! Your approach is better than what I had in mind because you do rebuild the complete OBS job so you might hit different OBS workers with differing performance. Well, to improve you could first try to gather the fail-ratio before your fix so that you know what to test for :) But I guess it's ok

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I have reviewed the CI runs and found no unhandled output and comparing to the last successful run of CI tests in master in https://github.com/os-autoinst/os-autoinst/runs/4340445436?check_suite_focus=true I see a runtime of 8s and we still have 8s in your PR

@mergify mergify bot merged commit 0a3f5b9 into os-autoinst:master Nov 30, 2021
@cfconrad cfconrad deleted the obs_package branch November 30, 2021 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants