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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove restore cache keys fallbacks and solve CI segfault problem #6557

Merged
merged 6 commits into from Sep 25, 2020

Conversation

microstudi
Copy link
Contributor

@microstudi microstudi commented Sep 24, 2020

馃帺 What? Why?

This is (yet) another attempt to discard a possible issue due cached gems.
It seems that is possible that we are retrieving backups from an older version of ruby as we use many fallbacks for the action/cache in Github workflows. This PR is to see if this may be the cause.

馃搶 Related Issues

  • Related to #?
  • Fixes #?

馃搵 Subtasks

  • Add CHANGELOG upgrade notes, if required
  • If there's a new public field, add it to GraphQL API
  • Add documentation regarding the feature
  • Add/modify seeds
  • Add tests
  • Another subtask

馃摲 Screenshots (optional)

Description

@microstudi
Copy link
Contributor Author

One test still failing due a segfault 馃槩, the other seems to be a flaky.

Usually segfaults come from a C extension gems. According to heroku this command should return all the C extensions used in the gemfile (assuming you have your gems in a subfolder of the current directory):

bundle show --paths | ruby -e "STDIN.each_line {|dep| puts dep.split('/').last if File.directory?(File.join(dep.chomp, 'ext')) }"

which gives the list:

bcrypt-3.1.13
bindex-0.8.1
bootsnap-1.4.6
byebug-11.1.3
charlock_holmes-0.7.7
concurrent-ruby-1.1.7
ffi-1.13.1
html_tokenizer-0.0.7
jaro_winkler-1.5.4
msgpack-1.3.3
nio4r-2.5.3
nokogiri-1.10.10
pg-1.1.4
puma-4.3.5
rb-fsevent-0.10.4
redcarpet-3.5.0
sassc-2.3.0
seven_zip_ruby-1.3.0
thread_safe-0.3.6
unf_ext-0.0.7.7
websocket-driver-0.7.3

PR from Sep 10:

From these, only the next were upgraded in #6513

nio4r (2.5.2) => nio4r (2.5.3)
nokogiri (1.10.9) => nokogiri (1.10.10)
concurrent-ruby (1.1.6) => concurrent-ruby (1.1.7)
seven_zip_ruby (1.2.5) => seven_zip_ruby (1.3.0)
websocket-driver (0.7.2) => websocket-driver (0.7.3)

In #6509 only this was upgraded:

seven_zip_ruby (~> 1.2, >= 1.2.2) => seven_zip_ruby (~> 1.3)

@microstudi
Copy link
Contributor Author

Looking at several logs with segfaults,
I haven't found these gems to be implicated:

seven_zip_ruby
websocket-driver

and:

nio4r
nokogiri
concurrent-ruby

Where present in all the traces.
However, specifically in the Process memory map dumped trace, only these gems are present:

nokogiri
nio4r

@microstudi
Copy link
Contributor Author

I haven't found information about nokogiri segfault that could be the same as we are having here.
However I did find something about the gem nio4r in socketry/nio4r#251

It seems that version 2.5.3 is giving intermittent segfaults as pointed here socketry/nio4r#251 (comment)

@microstudi
Copy link
Contributor Author

Bingo!

@microstudi microstudi changed the title remove restore cache keys alternatives Remove restore cache keys fallbacks and solve CI segfault problem Sep 25, 2020
@mrcasals
Copy link
Contributor

mrcasals commented Sep 25, 2020

Excellent job @microstudi! Super nice debugging! 馃憦 馃憦 Thanks for putting the explanations and the train of your thoughts, I found it super easy to understand what's going on!

Should we add the cache keys again changing their names so we don't get old cache versions? Maybe we could prepend the Ruby version:

          key: ${{ runner.OS }}-ruby266deps-${{ hashFiles('Gemfile.lock') }}
          restore-keys: |
            ${{ runner.OS }}-ruby266deps-${{ env.cache-name }}-
            ${{ runner.OS }}-ruby266deps-
            ${{ runner.OS }}-

@microstudi
Copy link
Contributor Author

@mrcasals do we need to specify fallback keys? I mean, the one created is going to be used anyway. In fact, for what I've read here: https://github.com/ruby/setup-ruby#caching-bundle-install-automatically it seems that we could get rid of all cache statement as is included in an option of the setup-ruby action.

We could configure setup-ruby as:

    - uses: ruby/setup-ruby@v1
      with:
        bundler-cache: true

@mrcasals
Copy link
Contributor

Oh, I didn't know about that option! Then yeeah, I think it's worth using it. We should update the action version too to make sure the option is available! 馃槃

@microstudi
Copy link
Contributor Author

All right, I'll make another commit see if the builtin cache works, this way we pass the test suite again, let's make sure there's no segfaults.

@tramuntanal
Copy link
Contributor

Excel路lent analysis @microstudi 馃憦 馃憦
We were myops only focusing on upgrading Ruby

@tramuntanal
Copy link
Contributor

Let's see if with the next commit the corresponding buid succeeds at first attempt 馃
I'm sure it will

@microstudi
Copy link
Contributor Author

@mrcasals It seems that the built in bundler works smoothly.

Here is the first run:
image
image
An here the second:
image

I'll change all the workflows then.

Copy link
Contributor

@Leusev Leusev left a comment

Choose a reason for hiding this comment

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

Wow!! Excellent catch here and great job @microstudi 馃槃
Thanks for all doc & explanation links too! It was easier to follow your thoughts
It seems all test have been passed, so looks good for me 馃憤
Thanks a lot!

@Leusev Leusev added in-review type: bug Issues that describe a bug labels Sep 25, 2020
Copy link
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Great!

@tramuntanal tramuntanal merged commit 0093a0f into develop Sep 25, 2020
@tramuntanal tramuntanal deleted the fix/github-cache branch September 25, 2020 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review module: core type: bug Issues that describe a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants