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

Sync needles when caching is enabled #1381

Merged
merged 2 commits into from Sep 20, 2017

Conversation

foursixnine
Copy link
Member

@foursixnine foursixnine commented Jul 4, 2017

This is a quick try to solve poo#19408

A verification run: http://deimos.suse.de/tests/6104#step/bootloader/6 that's showing just the needle that was created in interactive mode.

@foursixnine
Copy link
Member Author

foursixnine commented Jul 4, 2017

I basically duplicated the code from isotovideo engine. May be it calls for refactoring, but for now it seems to do the job. /cc @okurz @Zaoliang

@sysrich
Copy link
Member

sysrich commented Jul 4, 2017

As discussed with @foursixnine -1 from me

@okurz
Copy link
Member

okurz commented Jul 5, 2017

Great. You found a way to make it work and you know how to test. Now to reduce the duplication I suggest to implement that call as a isotovideo RPC command which can then be called from all locations where necessary.

@codecov
Copy link

codecov bot commented Jul 5, 2017

Codecov Report

Merging #1381 into master will increase coverage by 13.19%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1381       +/-   ##
===========================================
+ Coverage   75.27%   88.47%   +13.19%     
===========================================
  Files         105      105               
  Lines        7936     7945        +9     
===========================================
+ Hits         5974     7029     +1055     
+ Misses       1962      916     -1046
Impacted Files Coverage Δ
lib/OpenQA/Worker/Common.pm 80.93% <100%> (+35.82%) ⬆️
lib/OpenQA/Worker.pm 96.61% <100%> (+72.04%) ⬆️
lib/OpenQA/Worker/Commands.pm 76.66% <33.33%> (+24.28%) ⬆️
lib/OpenQA/WebAPI/Plugin/Helpers.pm 97.14% <0%> (+0.57%) ⬆️
lib/OpenQA/WebAPI.pm 92.18% <0%> (+2.28%) ⬆️
lib/OpenQA/Schema/Result/ScreenshotLinks.pm 39.62% <0%> (+5.66%) ⬆️
lib/OpenQA/Schema/Result/Needles.pm 79.04% <0%> (+5.71%) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm 79.31% <0%> (+6.03%) ⬆️
lib/OpenQA/Utils.pm 90.65% <0%> (+7.16%) ⬆️
lib/OpenQA/Schema/Result/JobModules.pm 91.16% <0%> (+7.9%) ⬆️
... and 14 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 4a0fcfc...79add65. Read the comment docs.

@coolo
Copy link
Contributor

coolo commented Jul 6, 2017

Actually, interactive mode with caching is IMO just nice to have. So refactor with ease

@foursixnine foursixnine force-pushed the fix/interactive_mode branch 5 times, most recently from 4c7cb1d to d09e1fe Compare August 11, 2017 08:17
@foursixnine
Copy link
Member Author

Time to work on a full-stack-ui test suite?

@coolo
Copy link
Contributor

coolo commented Aug 11, 2017

the whole idea of the full stack test was born to be able to test the interactive mode at all. But I never came around it, because https://progress.opensuse.org/issues/14818 needs to be fixed first

@coolo
Copy link
Contributor

coolo commented Aug 11, 2017

but if you could put it in record: did you test this manually with and without caching enabled?

@foursixnine
Copy link
Member Author

foursixnine commented Aug 11, 2017

Yep, tested on both scenarios, no problems so far signs with blood
When caching is enabled, this will be the output:

[DEBUG] Calling flock /var/lib/openqa/cache/deimos.suse.de/needleslock rsync -avHP rsync://deimos.suse.de/openqa-tests/ --delete /var/lib/openqa/cache/deimos.suse.de/tests/
receiving incremental file list                                         
opensuse/products/sle/needles/                                                                                                         
opensuse/products/sle/needles/failed-to-boot-20170810.json                                                                             
            217 100%  211.91kB/s    0:00:00 (xfr#1, ir-chk=8438/16702)                                        
opensuse/products/sle/needles/failed-to-boot-20170810.png                                                                              
          1,193 100%    1.14MB/s    0:00:00 (xfr#2, ir-chk=8437/16702)                                                                                                                                                                                                         
                                                                                                                                                                                                                                                                               
sent 431 bytes  received 450,909 bytes  902,680.00 bytes/sec                                                                            
total size is 2,066,964,415  speedup is 4,579.62                                                                                                                                                               
[DEBUG] RSYNC: Synchronization of tests directory took 0.00 seconds                                                                                        
[DEBUG] needles will be reloaded

When it is disabled, it still is:
[DEBUG] needles will be reloaded

@okurz
Copy link
Member

okurz commented Aug 15, 2017

LGTM

@foursixnine foursixnine force-pushed the fix/interactive_mode branch 2 times, most recently from e42eb80 to 4fa7758 Compare September 13, 2017 19:34
This is a quick try to solve poo#19408
Move the code a bit to allow the usage of cache_tests method on other
places to avoid code duplication.
@foursixnine
Copy link
Member Author

Life is sexy again now, let's live with that small coverage drop for the time being, as we would need to improve the fullstack test to also cover the needle editor with and without caching.

@foursixnine foursixnine merged commit 696bb63 into os-autoinst:master Sep 20, 2017
coolo pushed a commit that referenced this pull request Sep 20, 2017
commit 696bb63
Merge: 4a0fcfc 79add65
Author:     Santiago Zarate <santiago@zarate.net.ve>
AuthorDate: Wed Sep 20 15:07:42 2017 +0200
Commit:     GitHub <noreply@github.com>
CommitDate: Wed Sep 20 15:07:42 2017 +0200

    Merge pull request #1381 from foursixnine/fix/interactive_mode

    Sync needles when caching is enabled
coolo pushed a commit that referenced this pull request Sep 20, 2017
commit 696bb63
Merge: 4a0fcfc 79add65
Author:     Santiago Zarate <santiago@zarate.net.ve>
AuthorDate: Wed Sep 20 15:07:42 2017 +0200
Commit:     GitHub <noreply@github.com>
CommitDate: Wed Sep 20 15:07:42 2017 +0200

    Merge pull request #1381 from foursixnine/fix/interactive_mode

    Sync needles when caching is enabled
coolo pushed a commit that referenced this pull request Sep 20, 2017
commit 696bb63
Merge: 4a0fcfc 79add65
Author:     Santiago Zarate <santiago@zarate.net.ve>
AuthorDate: Wed Sep 20 15:07:42 2017 +0200
Commit:     GitHub <noreply@github.com>
CommitDate: Wed Sep 20 15:07:42 2017 +0200

    Merge pull request #1381 from foursixnine/fix/interactive_mode

    Sync needles when caching is enabled
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

4 participants