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

API Chunking Support #312

Merged
merged 8 commits into from
Sep 20, 2019
Merged

API Chunking Support #312

merged 8 commits into from
Sep 20, 2019

Conversation

agrare
Copy link
Member

@agrare agrare commented Dec 4, 2018

Pass a limit to each get_ call to prevent massive numbers of objects being returned and have them returned in multiple pages instead.

This reduces memory usage of the collector and also reduces load on the kubernetes API server.

https://bugzilla.redhat.com/show_bug.cgi?id=1722808

@miq-bot miq-bot added the wip label Dec 4, 2018
@agrare agrare force-pushed the use_api_chunking branch 4 times, most recently from 09b1bac to 10ba18c Compare December 6, 2018 18:23
@miq-bot
Copy link
Member

miq-bot commented Jun 17, 2019

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

@miq-bot miq-bot closed this Jun 17, 2019
@cben
Copy link
Contributor

cben commented Sep 18, 2019

@agrare How much work do you think it would take to revive this? cc @oourfali
Your Kubeclient changes have been merged & released, and the code here looks good 👍
It should also automatically work with older api that ignores limit.
My guess is the bigger effort would be testing for regressions / unforeseen stability problems down the line.

Motivation:

In https://bugzilla.redhat.com/1722808, users were seeing timeouts on openshift side for manageiq's large refresh requests. I suspect in that particular setup it could be relieved on openshift side (https://bugzilla.redhat.com/1729461), and I'm not aware of any concrete bug in manageiq contributing to that, aside from the fact manageiq makes huge requests.
But chunked requests are known to be better for all layers in k8s (apiserver, etcd) so would improve scaling if we can spare the effort.

@agrare
Copy link
Member Author

agrare commented Sep 18, 2019

Hey @cben the biggest issue for me was not being able to re-record the VCRs for specs
If the original systems are still around that would be helpful otherwise we'll basically need to rewrite them.

@agrare agrare reopened this Sep 18, 2019
@cben
Copy link
Contributor

cben commented Sep 19, 2019

VCR recording is quite automated with https://github.com/ManageIQ/guides/blob/master/providers/openshift.md#automated-script-to-record-new-vcr

You need a clean empty openshift running, easiest is with minishift as documented in that doc.
Then you run that script from the providers-openshift repo.

We never bothered with separate recording of a kubernetes cluster; instead we just copy the updated VCRs over here to providers-kubernetes repo.

cp -v ../manageiq-providers-openshift/spec/vcr_cassettes/manageiq/providers/openshift/container_manager/refresher*deletions*  spec/vcr_cassettes/manageiq/providers/kubernetes/container_manager/

The assertions are split between the tests in the 2 repos, here we test the upstream k8s objects.

  • I don't know how the 2 refresher_targeted_refresh_*.yml files got recorded, I think Ladislav added them.

  • The refresher.yml file apparently dates back to single manageiq repo and was manually edited since then :-(
    IIRC the plan was to eventually entirely replace it with the scripted refresher*deletions* files, not sure if there are any valuable tests left that use it.
    Maybe manually edit it to match the limit param, or config vcr to ignore limit param when matching; the responses would remain as a test for "server doesn't understand chunking"?

@agrare agrare changed the title [WIP] Initial API chunking support Initial API chunking support Sep 19, 2019
@miq-bot miq-bot removed the wip label Sep 19, 2019
@agrare
Copy link
Member Author

agrare commented Sep 19, 2019

Thanks @cben ! I got the kubernetes and openshift specs passing. I had the original refresher.yml specs match only on the method and path ignoring the query so it is the "no-limit-supported" test also

@agrare agrare changed the title Initial API chunking support API Chunking Support Sep 19, 2019
@miq-bot
Copy link
Member

miq-bot commented Sep 20, 2019

Checked commits agrare/manageiq-providers-kubernetes@97ddecf~...2b2a7e4 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

This is awesome 👏

@cben cben merged commit bea27dd into ManageIQ:master Sep 20, 2019
@agrare agrare deleted the use_api_chunking branch September 20, 2019 13:34
@agrare agrare added this to the Sprint 121 Ending Sep 30, 2019 milestone Sep 20, 2019
simaishi pushed a commit that referenced this pull request Sep 20, 2019
@simaishi
Copy link

Hammer backport details:

$ git log -1
commit b3577b80897095a285e26adee9aa8c18dd48fa9f
Author: Beni Cherniavsky-Paskin <[email protected]>
Date:   Fri Sep 20 16:32:29 2019 +0300

    Merge pull request #312 from agrare/use_api_chunking
    
    API Chunking Support
    
    (cherry picked from commit bea27dd9fc49d1e488b8d17ec9c92bdc62046d22)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1754071

simaishi pushed a commit that referenced this pull request Nov 1, 2019
@simaishi
Copy link

simaishi commented Nov 1, 2019

Ivanchuk backport details:

$ git log -1
commit 5066f371cfd15ddbe3032fb82604e59629a11ea3
Author: Beni Cherniavsky-Paskin <[email protected]>
Date:   Fri Sep 20 16:32:29 2019 +0300

    Merge pull request #312 from agrare/use_api_chunking
    
    API Chunking Support
    
    (cherry picked from commit bea27dd9fc49d1e488b8d17ec9c92bdc62046d22)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1767834

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants