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

Stop updating volume snapshot status on every reconcile #8431

Merged
merged 1 commit into from Sep 10, 2022

Conversation

rmohr
Copy link
Member

@rmohr rmohr commented Sep 8, 2022

What this PR does / why we need it:

Ensure that the snapshot controller only updates the VM status when something changed.

Without this comparision, the snapshot controller is performing a REST operation after every trigger. This leads to the following behaviour:

  • Every VM modification leads to a rest call by the snapshot controller.
  • On ther periodic 5-minute reconcile by this controller a REST call is performed too.

This can easily be observed by watching this metric:

rest_client_request_latency_seconds_count{url="https://10.96.0.1:443/apis/kubevirt.io/v1alpha3/namespaces/%7Bnamespace%7D/virtualmachines/%7Bname%7D/status",verb="PUT"} 26

Even if the cluster is idle, the number of REST calls goes up by the number of existing VMs every 5 minutes.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Fix shadow status updates and periodic status updates on VMs, performed by the snapshot controller

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XS labels Sep 8, 2022
@rmohr
Copy link
Member Author

rmohr commented Sep 8, 2022

/topic scale

@rmohr
Copy link
Member Author

rmohr commented Sep 8, 2022

/cc @rthallisey
/cc @marceloamaral

FYI

Ensure that the snapshot controller only updates the VM status when
something changed.

Without this comparision, the snapshot controller is performing a REST
operation after every trigger. This leads to the following behaviour:

 * Every VM modification leads to a rest call by the snapshot
   controller.
 * On ther periodic 5-minute reconcile by this controller a REST call is
   performed too.

Signed-off-by: Roman Mohr <rmohr@google.com>
@rmohr
Copy link
Member Author

rmohr commented Sep 8, 2022

/cc @awels
/cc @mhenriks

FYI

@rthallisey
Copy link
Contributor

/test pull-kubevirt-e2e-k8s-1.21-sig-performance

@kubevirt-bot
Copy link
Contributor

@rthallisey: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-kubevirt-apidocs
  • /test pull-kubevirt-build
  • /test pull-kubevirt-build-arm64
  • /test pull-kubevirt-check-unassigned-tests
  • /test pull-kubevirt-client-python
  • /test pull-kubevirt-e2e-k8s-1.22-operator
  • /test pull-kubevirt-e2e-k8s-1.22-sig-compute
  • /test pull-kubevirt-e2e-k8s-1.22-sig-monitoring
  • /test pull-kubevirt-e2e-k8s-1.22-sig-network
  • /test pull-kubevirt-e2e-k8s-1.22-sig-storage
  • /test pull-kubevirt-e2e-k8s-1.23-operator
  • /test pull-kubevirt-e2e-k8s-1.23-operator-nonroot
  • /test pull-kubevirt-e2e-k8s-1.23-sig-compute
  • /test pull-kubevirt-e2e-k8s-1.23-sig-compute-cgroupsv2
  • /test pull-kubevirt-e2e-k8s-1.23-sig-compute-migrations
  • /test pull-kubevirt-e2e-k8s-1.23-sig-compute-nonroot
  • /test pull-kubevirt-e2e-k8s-1.23-sig-network
  • /test pull-kubevirt-e2e-k8s-1.23-sig-network-nonroot
  • /test pull-kubevirt-e2e-k8s-1.23-sig-storage
  • /test pull-kubevirt-e2e-k8s-1.23-sig-storage-cgroupsv2
  • /test pull-kubevirt-e2e-k8s-1.23-sig-storage-nonroot
  • /test pull-kubevirt-e2e-k8s-1.24-ipv6-sig-network
  • /test pull-kubevirt-e2e-k8s-1.24-operator
  • /test pull-kubevirt-e2e-k8s-1.24-sig-compute
  • /test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
  • /test pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations-nonroot
  • /test pull-kubevirt-e2e-k8s-1.24-sig-network
  • /test pull-kubevirt-e2e-k8s-1.24-sig-storage
  • /test pull-kubevirt-e2e-k8s-1.24-sig-storage-dv-gc
  • /test pull-kubevirt-e2e-kind-1.22-sriov-nonroot
  • /test pull-kubevirt-e2e-kind-1.23-vgpu
  • /test pull-kubevirt-e2e-kind-1.23-vgpu-nonroot
  • /test pull-kubevirt-e2e-windows2016
  • /test pull-kubevirt-generate
  • /test pull-kubevirt-manifests
  • /test pull-kubevirt-prom-rules-verify
  • /test pull-kubevirt-unit-test
  • /test pull-kubevirt-verify-go-mod
  • /test pull-kubevirtci-bump-kubevirt

The following commands are available to trigger optional jobs:

  • /test build-kubevirt-builder
  • /test pull-kubevirt-check-tests-for-flakes
  • /test pull-kubevirt-code-lint
  • /test pull-kubevirt-e2e-arm64
  • /test pull-kubevirt-e2e-k8s-1.22-sig-compute-realtime
  • /test pull-kubevirt-e2e-k8s-1.22-sig-performance
  • /test pull-kubevirt-e2e-k8s-1.23-single-node
  • /test pull-kubevirt-e2e-k8s-1.23-swap-enabled
  • /test pull-kubevirt-e2e-kind-1.22-sriov
  • /test pull-kubevirt-fossa
  • /test pull-kubevirt-gosec
  • /test pull-kubevirt-goveralls
  • /test pull-kubevirt-verify-rpms

Use /test all to run the following jobs that were automatically triggered:

  • pull-kubevirt-apidocs
  • pull-kubevirt-build
  • pull-kubevirt-build-arm64
  • pull-kubevirt-check-tests-for-flakes
  • pull-kubevirt-check-unassigned-tests
  • pull-kubevirt-client-python
  • pull-kubevirt-code-lint
  • pull-kubevirt-e2e-k8s-1.22-operator
  • pull-kubevirt-e2e-k8s-1.22-sig-compute
  • pull-kubevirt-e2e-k8s-1.22-sig-network
  • pull-kubevirt-e2e-k8s-1.22-sig-storage
  • pull-kubevirt-e2e-k8s-1.23-operator
  • pull-kubevirt-e2e-k8s-1.23-operator-nonroot
  • pull-kubevirt-e2e-k8s-1.23-sig-compute
  • pull-kubevirt-e2e-k8s-1.23-sig-compute-migrations
  • pull-kubevirt-e2e-k8s-1.23-sig-compute-nonroot
  • pull-kubevirt-e2e-k8s-1.23-sig-network
  • pull-kubevirt-e2e-k8s-1.23-sig-network-nonroot
  • pull-kubevirt-e2e-k8s-1.23-sig-storage
  • pull-kubevirt-e2e-k8s-1.23-sig-storage-nonroot
  • pull-kubevirt-e2e-k8s-1.24-ipv6-sig-network
  • pull-kubevirt-e2e-k8s-1.24-operator
  • pull-kubevirt-e2e-k8s-1.24-sig-compute
  • pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations
  • pull-kubevirt-e2e-k8s-1.24-sig-compute-migrations-nonroot
  • pull-kubevirt-e2e-k8s-1.24-sig-network
  • pull-kubevirt-e2e-k8s-1.24-sig-storage
  • pull-kubevirt-e2e-k8s-1.24-sig-storage-dv-gc
  • pull-kubevirt-e2e-kind-1.22-sriov-nonroot
  • pull-kubevirt-e2e-kind-1.23-vgpu
  • pull-kubevirt-e2e-kind-1.23-vgpu-nonroot
  • pull-kubevirt-e2e-windows2016
  • pull-kubevirt-fossa
  • pull-kubevirt-generate
  • pull-kubevirt-goveralls
  • pull-kubevirt-manifests
  • pull-kubevirt-prom-rules-verify
  • pull-kubevirt-unit-test

In response to this:

/test pull-kubevirt-e2e-k8s-1.21-sig-performance

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rthallisey
Copy link
Contributor

/test pull-kubevirt-e2e-k8s-1.22-sig-performance

1 similar comment
@rmohr
Copy link
Member Author

rmohr commented Sep 8, 2022

/test pull-kubevirt-e2e-k8s-1.22-sig-performance

@rmohr
Copy link
Member Author

rmohr commented Sep 8, 2022

/retest

@rthallisey
Copy link
Contributor

Before

     "CREATE-pods-count": {
      "value": 117.11444444444446
    }, 
     "UPDATE-virtualmachineinstances-count": {
      "value": 645.9650959549048,
      "thresholdResult": {
        "thresholdValue": 1171.1444444444446,
        "thresholdMetric": "CREATE-pods-count",
        "thresholdRatio": 5.5156739975087445,
        "thresholdExceeded": false
      } 

After

     "CREATE-pods-count": {
      "value": 107.7673620367902
    }, 
     "UPDATE-virtualmachineinstances-count": {
      "value": 595.4768773693352,
      "thresholdResult": {
        "thresholdValue": 1077.673620367902,
        "thresholdMetric": "CREATE-pods-count",
        "thresholdRatio": 5.525577188815738,
        "thresholdExceeded": false
      } 

For context, the performance job is only creating 100 VMIs which may not be long enough to notice the difference. So I guess it makes sense the results look about the same (similar ratio to CREATE-pods-count). And I'm assuming UPDATEing VMs will also update VMIs because I don't see any UPDATE-virtualmachines calls.

@mhenriks
Copy link
Member

mhenriks commented Sep 9, 2022

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2022
@mhenriks
Copy link
Member

mhenriks commented Sep 9, 2022

/retest-required

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2022
@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Sep 9, 2022

@rmohr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-goveralls b527b90 link false /test pull-kubevirt-goveralls

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rmohr
Copy link
Member Author

rmohr commented Sep 9, 2022

For context, the performance job is only creating 100 VMIs which may not be long enough to notice the difference. So I guess it makes sense the results look about the same (similar ratio to CREATE-pods-count). And I'm assuming UPDATEing VMs will also update VMIs because I don't see any UPDATE-virtualmachines calls.

@rthallisey I am not sure if you were looking at the right metrics. The status updates are happening on the VirtualMachine (not the VMI). Therefore I think that on VMI-only performance tests you will not see any difference.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants