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

instancetypes: Improve instancetype snapshots #8282

Merged
merged 12 commits into from Sep 30, 2022

Conversation

akrejcir
Copy link
Contributor

@akrejcir akrejcir commented Aug 11, 2022

What this PR does / why we need it:
The ControllerRevisions used for instancetype and preference snapshots will contain the whole object.
It is useful, because the object contains version information, and we can use methods in the runtime package to decode it.

Release note:

Improves instancetype and preference controller revisions. This is a backwards incompatible change and introduces a new v1alpha2 api for instancetype and preferences.

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 11, 2022
@akrejcir
Copy link
Contributor Author

/cc @lyarwood

@lyarwood
Copy link
Member

/area instancetype

Copy link
Member

@lyarwood lyarwood left a comment

Choose a reason for hiding this comment

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

Looking good overall, really like the new ControllerRevision code so thanks for that!

I've listed a few nits and a conflict with a PR I posted earlier today. If you could also add some more context to some of your refactor commits I'd appreciate it. I get that it's boring but it's useful for reviewers and the original author (./me waves) to understand why you wanted to change certain things.

Cheers!

tests/instancetype_test.go Show resolved Hide resolved
pkg/instancetype/instancetype.go Outdated Show resolved Hide resolved
pkg/instancetype/instancetype.go Show resolved Hide resolved
@lyarwood
Copy link
Member

Odd this is also causing the following failure for me locally:

# ./cluster-up/kubectl.sh apply -f ./examples/csmall.yaml -f ./examples/vm-cirros-csmall.yaml
[..]
# ./cluster-up/virtctl.sh start vm-cirros-csmall
[..]
# ./cluster-up/kubectl.sh get vms/vm-cirros-csmall -o json | jq .status.conditions[1]
selecting docker as container runtime
{
  "lastProbeTime": null,
  "lastTransitionTime": "2022-08-11T14:50:02Z",
  "message": "Failure while starting VMI: admission webhook \"virtualmachine-validator.kubevirt.io\" denied the request: Failure to find instancetype: failed to decode object in ControllerRevision: Object 'Kind' is missing in '{\"metadata\":{\"name\":\"csmall\",\"namespace\":\"default\",\"uid\":\"f2218614-7e54-4ebe-8c7f-fec60b0d7d86\",\"resourceVersion\":\"87367\",\"generation\":1,\"creationTimestamp\":\"2022-08-11T14:49:54Z\"},\"spec\":{\"cpu\":{\"guest\":1},\"memory\":{\"guest\":\"128Mi\"}}}'",
  "reason": "FailedCreate",
  "status": "True",
  "type": "Failure"
}

@akrejcir
Copy link
Contributor Author

Odd this is also causing the following failure for me locally:

Maybe the code is not working yet. I'm still compiling it and uploading to quay to test it.
I will mark this as draft in the meantime.

@akrejcir akrejcir marked this pull request as draft August 11, 2022 14:59
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2022
@lyarwood
Copy link
Member

lyarwood commented Aug 11, 2022

Odd this is also causing the following failure for me locally:

Maybe the code is not working yet. I'm still compiling it and uploading to quay to test it. I will mark this as draft in the meantime.

Yeah it reproduces in the unit tests as well FWIW. I think we need something like this as TypeMeta isn't present :

kubernetes/kubernetes#3030 (comment)

@lyarwood
Copy link
Member

Odd this is also causing the following failure for me locally:

Maybe the code is not working yet. I'm still compiling it and uploading to quay to test it. I will mark this as draft in the meantime.

Yeah it reproduces in the unit tests as well FWIW. I think we need something like this as TypeMeta isn't present :

kubernetes/kubernetes#3030 (comment)

The following is working for me now:

https://gist.github.com/lyarwood/1645a40c115eb62e6da37ca1e289be9e

@akrejcir
Copy link
Contributor Author

/retest pull-kubevirt-e2e-k8s-1.23-sig-compute

@akrejcir
Copy link
Contributor Author

/retest pull-kubevirt-unit-test

@kubevirt-bot
Copy link
Contributor

@akrejcir: The /retest command does not accept any targets.
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-compute-migrations
  • /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-migrations-nonroot
  • /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-network
  • /test pull-kubevirt-e2e-k8s-1.24-sig-storage
  • /test pull-kubevirt-e2e-kind-1.22-sriov
  • /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-k8s-1.24-sig-storage-dv-gc
  • /test pull-kubevirt-e2e-kind-1.22-sriov-nonroot
  • /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-compute-migrations
  • pull-kubevirt-e2e-k8s-1.22-sig-network
  • pull-kubevirt-e2e-k8s-1.22-sig-performance
  • 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-migrations-nonroot
  • 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-network
  • pull-kubevirt-e2e-k8s-1.24-sig-storage
  • pull-kubevirt-e2e-kind-1.22-sriov
  • 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
  • pull-kubevirt-verify-go-mod

In response to this:

/retest pull-kubevirt-e2e-k8s-1.23-sig-compute

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.

@kubevirt-bot
Copy link
Contributor

@akrejcir: The /retest command does not accept any targets.
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-compute-migrations
  • /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-migrations-nonroot
  • /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-network
  • /test pull-kubevirt-e2e-k8s-1.24-sig-storage
  • /test pull-kubevirt-e2e-kind-1.22-sriov
  • /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-k8s-1.24-sig-storage-dv-gc
  • /test pull-kubevirt-e2e-kind-1.22-sriov-nonroot
  • /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-compute-migrations
  • pull-kubevirt-e2e-k8s-1.22-sig-network
  • pull-kubevirt-e2e-k8s-1.22-sig-performance
  • 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-migrations-nonroot
  • 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-network
  • pull-kubevirt-e2e-k8s-1.24-sig-storage
  • pull-kubevirt-e2e-kind-1.22-sriov
  • 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
  • pull-kubevirt-verify-go-mod

In response to this:

/retest pull-kubevirt-unit-test

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.

@akrejcir
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.23-sig-compute
/test pull-kubevirt-unit-test

@akrejcir
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.23-sig-compute

@akrejcir
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.23-sig-compute

@akrejcir
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.23-sig-compute

@akrejcir akrejcir marked this pull request as ready for review August 16, 2022 15:55
One letter variables are confusing.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Move variable definitions to the scope of their usage.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Changed preference definition in unittests,
so that the tests do not require an instancetype.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
One context for instncetype tests and the other
for preference tests.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Changed calls to NewRandomVMIWithEphemeralDisk() to functions
from libvmi.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Small code changes:
- renamed variables
- unified two Eventually() blocks to one, which checks both conditions

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Created a new function to store ControllerRevisions.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
The next commits will make changes to the API,
so the version needs to be increased.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
Use the new API version instead of v1alpha1.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
The ControllerRevision will store one of these objects:
- VirtualMachineInstancetype
- VirtualMachineClusterInstancetype
- VirtualMachinePreference
- VirtualMachineClusterPreference

Advantage of this approach is that the objects already contain
version information, and they will be properly displayed when
using "kubectl get".

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
- Create revision name inside CreateControllerRevision() function.
- Lazily initialize logger for errors, because it is needed only
  when an error happens.
- Other smaller code cleanup.

Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2022
@lyarwood
Copy link
Member

/lgtm

Fun times, c9e474a landed overnight and added more tests using the v1alpha1 version causing various failures in the gate. I've rebased the series on main again and moved all remaining tests to v1alpha2 in 1149942. This should be good now so I've added lgtm back in.

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2022
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2022
b91332f and
c9e474a recently added more tests using
v1alpha1, this change moves these to the v1alpha2.

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
Copy link
Member

/lgtm

Apologies for the noise I missed the BUILD.bazel changes, should be good now.

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

/retest

2 similar comments
@lyarwood
Copy link
Member

/retest

@lyarwood
Copy link
Member

/retest

@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.

@lyarwood
Copy link
Member

/retest

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Sep 30, 2022

@akrejcir: The following tests 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-fossa 567670a link false /test pull-kubevirt-fossa
pull-kubevirt-e2e-kind-1.22-sriov-nonroot 2f4fda4 link true /test pull-kubevirt-e2e-kind-1.22-sriov-nonroot

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.

@kubevirt-bot kubevirt-bot merged commit 661b2dc into kubevirt:main Sep 30, 2022
@akrejcir akrejcir deleted the instancetype-snapshots branch October 2, 2022 17:50
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. area/instancetype 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/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants