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 increased IC pod startup time when hundreds VirtualServerRoutes are used #1926

Merged
merged 2 commits into from
Sep 7, 2021

Conversation

pleshakov
Copy link
Contributor

@pleshakov pleshakov commented Sep 2, 2021

Proposed changes

Previously, the IC would update the status of a VSR (via an API call to the k8s API) even if the status of the VSR wasn't changed. The unnecessary API calls lead to the increased IC pod startup time for cases when a VS had multiple (hundreds) VSRs.

The PR fixes the problem - the IC will skip the update if the status of a VSR isn't changed.

The PR includes a test that creates a VS with multiple VSRs and then checks the startup time of the pod.

The output is for 500 VSRs:

Before: gave up after 15 mins

Now: 201 seconds

All pods are ContainersReady
All pods came up in 201 seconds
Scale a deployment 'nginx-ingress': complete

@github-actions github-actions bot added the bug An issue reporting a potential bug label Sep 2, 2021
VSR_COUNT=500

@pytest.fixture(scope="class")
def vs_vsr_setup(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function doesn't remove any created resources because k8s will remove them when the test_namespace is deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

test_namespace fixture is class scoped not method so if there are other tests in same class then those resources will remain until all the tests are executed.

and get_last_reload_status(metrics_url, "nginx") == "1"
)

assert num is None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this assert from the other tests - not fully sure though about it

Copy link
Contributor

Choose a reason for hiding this comment

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

this is to make sure that it scaled down to 0 before starting to scale up but not necessary anymore since we've better checks now.

Copy link
Contributor

@vepatel vepatel left a comment

Choose a reason for hiding this comment

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

The two new functions in custom_resource_utils.py are not needed as there are pre-existing functions to perform those tasks.

@vepatel vepatel self-requested a review September 7, 2021 15:47
Previously, the IC would update the status of a VSR (via an API call
to the k8s API) even if the status of the VSR wasn't changed.
The unnecessary API calls lead to the increased IC pod startup time
for cases when a VS had multiple (hundreds) VSRs.

The commit fixes the problem - the IC will skip the update if the
status of a VSR isn't changed.
@pleshakov pleshakov merged commit 7b96bec into master Sep 7, 2021
@pleshakov pleshakov deleted the fix/vsr-status branch September 7, 2021 20:43
@pleshakov pleshakov changed the title Fix increased IC pod startup time when hundreds VSRs are used Fix increased IC pod startup time when hundreds VirtualServerRoutes are used Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants