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

Add filter when checking out OpenSearch-Dashboards to speed up CI checks #1613

Merged
merged 30 commits into from
Oct 20, 2023

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Oct 16, 2023

Description

Adds filter to the actions/checkout@v2 step of install-dashboards to minimize the size of the repo that is cloned. The cypress folder is over 600MB in size and not needed for the security integ tests.

Category

Test fix

Issues Resolved

#1611

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #1613 (f91773f) into main (d3b3f84) will increase coverage by 1.75%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1613      +/-   ##
==========================================
+ Coverage   66.18%   67.93%   +1.75%     
==========================================
  Files          93       93              
  Lines        2339     2339              
  Branches      317      317              
==========================================
+ Hits         1548     1589      +41     
  Misses        722      722              
+ Partials       69       28      -41     

see 17 files with indirect coverage changes

Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented Oct 16, 2023

The integ test failures are definitely due to high watermark.

Filesystem      Size  Used Avail Use% Mounted on
/dev/root        84G   74G  9.5G  89% /

Where is most of the space being consumed?

cwperks and others added 23 commits October 17, 2023 09:21
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented Oct 19, 2023

4 / 10 of the windows runs failed on the first run, but succeeded on the 2nd. Its fairly stable with this fix, but still susceptible to failure. I'm filing an issue now around stability and speed for the CI on windows.

@cwperks
Copy link
Member Author

cwperks commented Oct 19, 2023

@peternied issue on this repo: #1620

I am also going to file an issue in OSD core to see if its possible to only install prod dependencies.

Something like NODE_ENVIRONMENT=production yarn osd bootstrap

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

4 / 10 of the windows runs failed on the first run, but succeeded on the 2nd. Its fairly stable with this fix, but still susceptible to failure. I'm filing an issue now around stability and speed for the CI on windows.

Wait, what was the failure rate before? I know you've been investing time in this - but I'd rather we make the minimal change with low downside rather that potential create more issues.

Let me know if you've got before and after numbers - I am also happy to fold these changes into the overall CI improvements work and let you move onto other topics.

@cwperks
Copy link
Member Author

cwperks commented Oct 19, 2023

@peternied The failure rate before this change was 100% on Windows.

Edit: I just kicked off 10 iterations for each platform before this change here: https://github.com/cwperks/security-dashboards-plugin/actions/runs/6577075426

@cwperks
Copy link
Member Author

cwperks commented Oct 19, 2023

@peternied Please see the link above. Only 2 / 10 passed before the change to free up space on the Windows runner.

@peternied
Copy link
Member

On its own the change to filter check out of the Dashboards repository - happy to merge.

Manually deleting/uninstall - I'm not comfortable without a 100% pass rate. It's hard to measure/maintain as a testament to the work you've done - I don't want us chasing future maintenance issues in the space if it isn't the full picture.

If we don't have the Windows platform operating reliably, it sounds like we should just disable it and then separately see how to get it reliable.

@cwperks
Copy link
Member Author

cwperks commented Oct 19, 2023

If we don't have the Windows platform operating reliably, it sounds like we should just disable it and then separately see how to get it reliable.

I would be supportive of that. This change would make the pass rate go from <20% to >70% on Windows so it would provide benefit in the interim while #1620 is worked on.

btw if any uninstall fails, the workflow continues as normal and would effectively work as it does today where it would run with all of the pre-installed software.

Signed-off-by: Darshit Chanpura <[email protected]>
(cherry picked from commit da01ca9e93a18028044ea178743a73af01f37c99)
@DarshitChanpura
Copy link
Member

@peternied @cwperks I've update the workflows to disable disk threshold check and have also reverted changes related to deleting pre-installed software on Windows runners

@cwperks
Copy link
Member Author

cwperks commented Oct 19, 2023

Looks good to me!

@peternied
Copy link
Member

@DarshitChanpura Thanks for your help on this. I've created the code instability tracing issue [1] as well as a number of observations about test pass rate. Lets try to keep working against a known state - while you are focused on this maybe you could update and track where we are at until we can get back to reliable.

@DarshitChanpura DarshitChanpura merged commit d97192d into opensearch-project:main Oct 20, 2023
9 checks passed
@DarshitChanpura DarshitChanpura added the backport 2.x backport to 2.x branch label Oct 20, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 20, 2023
…cks (#1613)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Co-authored-by: Darshit Chanpura <[email protected]>
(cherry picked from commit d97192d)
DarshitChanpura pushed a commit that referenced this pull request Oct 20, 2023
…cks (#1613) (#1625)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Co-authored-by: Darshit Chanpura <[email protected]>
(cherry picked from commit d97192d)

Co-authored-by: Craig Perkins <[email protected]>
leanneeliatra pushed a commit to leanneeliatra/security-dashboards-plugin-fork that referenced this pull request Nov 17, 2023
…cks (opensearch-project#1613)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Co-authored-by: Darshit Chanpura <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants