Add unit tests to check for API backward compatibility #11856

Sreeja1725 · 2024-05-06T18:11:31Z

What this PR does

Before this PR: There are no unit tests which check for API backward compatibility

After this PR:

API reviewers can say if changes in current version will break older clients upon upgrade
During upgrades, vendors can check the API changes going into the upgrade using simple differ and get a better synopsis of what is failing during upgrade.

Fixes #11629

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

A unit test that decodes from yaml to go structs and encodes from go structs to yaml objects. Both encoding and decoding passes for each API change. This unit test will be very helpful in automating tests for api changes in kubevirt.

blocked on updating the makefile.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
[Y] Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

      NONE

kubevirt-bot · 2024-05-06T18:11:40Z

Hi @Sreeja1725. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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 · 2024-05-06T18:11:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign acardace for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

alaypatel07 · 2024-05-07T11:52:02Z

/assign @alaypatel07

lyarwood · 2024-05-07T14:16:26Z

staging/src/kubevirt.io/api/roundtrip/README.md

@@ -0,0 +1,21 @@
+### Description:
+
+1. This creates JSON and YAML files for all the API exposed by kubevirt in group-version "kubevirt.io/v1", 


I'm likely missing something here but why do we need this when we have https://github.com/kubevirt/api ?

@lyarwood the contents of https://github.com/kubevirt/api are automatically mirror from the staging directory in this repository. Example this commit kubevirt/api@105e64b was picked from here 7cbd05c

Yes my point was that we already have a versioned account of the API so do we need to generate anything in our tree? Shouldn't these tests also run as part of that project?

@lyarwood we need it in-tree because the idea is that these tests will be plugged into CI so APK reviewers can investigate the failure of the test for backwards compatibility breakage. If we do it in kubevirt/api repo, the test will be run postmerge so it will defeat the - purpose of helping API reviewers. Besides contents of the kubevirt/api repo are also in-tree in the staging dir

@alaypatel07 right apologies for the confusion about also running tests in that repo, that would be pointless if we ran them against the original PR in kubevirt/kubevirt already. However IMHO my point still remains that we don't need to generate yet more yaml/json in this repo to test the backward compatibility of a PR when we already have versioned copies of the API in https://github.com/kubevirt/api .

@lyarwood sorry I was missing your point. The versioned copy of API unfortunately does not provide us with a guarantee that changes to APIs (any of the V1 apis for example) will be backward compatible. For instance:
the https://github.com/kubevirt/api/blob/main/core/v1/types.go#L86 this field in a v1 api could be changed to an int type in this release as follows and there are currently no automated tests to prevent that while it is clearly a backward incompatible change

PriorityClassName int `json:"priorityClassName,omitempty"`

Here is a real example that could be caught by this automation: https://github.com/kubevirt/kubevirt/pull/6709/files#diff-34a591e3aa47260cfad791064c20f34f98fdca4a63532fbf7184e68682f4fcb8R254, apologies for picking on this PR. Notice the field has +optional tag but missing omitempty in its json tags

I'm not suggesting that we don't need to write tests to pickup on these incompatible changes just the need to generate duplicate data in this repo when it already exists elsewhere. Could we not use the copies of the API in https://github.com/kubevirt/api to detect such a change?

I've spoken to @alaypatel07 on Slack about this and I unfortunately missed that this is common practice in core k8s. The alternative of hosting these generated manifests elsewhere also isn't simple so for now I'm going to remove my concern here. Thanks again for taking the time to talk about this @alaypatel07.

alaypatel07 · 2024-05-08T13:52:29Z

Makefile

@@ -5,7 +5,7 @@ ifeq (${TIMESTAMP}, 1)
  SHELL = ./hack/timestamps.sh
 endif

-all: format bazel-build manifests
+all: format bazel-build manifests update-generate-api-testdata


this should be dropped from here

and moved to generate?

alaypatel07 · 2024-05-08T13:53:16Z

Makefile

@@ -232,6 +232,11 @@ lint-metrics:
 	./hack/prom-metric-linter/metric_name_linter.sh --operator-name="kubevirt" --sub-operator-name="kubevirt" --metrics-file=metrics.json
 	rm metrics.json

+update-generate-api-testdata:


can we also add test-api-compat that will invoke these tests without UPDATE_COMPATIBILITY_FIXTURE_DATA=true for all three previous versions

EdDev · 2024-05-15T13:18:55Z

/cc

alaypatel07

Added some comments.

@Sreeja1725 can you also take a look at the Previous Version test and check if downgrading is tested?

alaypatel07 · 2024-05-16T02:53:45Z

staging/src/kubevirt.io/api/testdata/README.md

+
+This directory tree contains serialized API objects in json and yaml formats.
+
+This creates JSON and YAML files for all the API exposed by kubevirt in group-version "kubevirt.io/v1", versioned by the release. The current version is in `HEAD` directory, previous versions are in `release-0.yy` releasedirectory. APIs includes, more APIs can be added in the future:


change this to

This creates JSON and YAML files for three APIs exposed by kubevirt in group-version "kubevirt.io/v1", versioned by the release. The main branch is will be stored in `HEAD` directory, previous versions are in `release-0.yy` release directory. While more APIs can be added in the future, the current list includes:

alaypatel07 · 2024-05-16T02:55:58Z

staging/src/kubevirt.io/api/testdata/README.md

+
+To run serialization tests for a particular group/version/kind, like `apps/v1` `Deployment`:
+```sh
+go test staging/src/kubevirt.io/api -run /apps.v1.Deployment/


nit: change this to a kubevirt api?

alaypatel07 · 2024-05-16T02:57:01Z

staging/src/kubevirt.io/api/testdata/README.md

+2. api-field: `status.runtimeUser` field was added[ref-3](https://github.com/kubevirt/kubevirt/pull/6709)
+
+
+## REVIEWRS GUIDE


typo: REVIEWERS

alaypatel07 · 2024-05-16T02:57:56Z

staging/src/kubevirt.io/api/testdata/README.md

+
+## REVIEWRS GUIDE
+
+Upon upgrade to API, the json and YAML files will be updated.


nit: change this to

With any change in go structs of the APIs, the corresponding JSON and YAML files will be updated

Sreeja1725 · 2024-05-17T14:34:11Z

Added some comments.

@Sreeja1725 can you also take a look at the Previous Version test and check if downgrading is tested?

The current tests will alway test upgrade since the schema builder will be at a higher version than the json data (this is the upgrade scenario). For downgrade scenario we need the schema builder to be at a lower version than the json data which is currently not tested.

Sreeja1725 · 2024-05-28T17:14:12Z

/test pull-kubevirt-unit-test
/test pull-kubevirt-build

kubevirt-bot · 2024-05-28T17:14:27Z

@Sreeja1725: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-kubevirt-unit-test
/test pull-kubevirt-build

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-sigs/prow repository.

kubevirt-bot · 2024-05-28T17:17:17Z

@Sreeja1725: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

alaypatel07 · 2024-05-28T17:27:23Z

/test pull-kubevirt-unit-test
/test pull-kubevirt-build

Sreeja1725 · 2024-05-28T17:43:56Z

/retest

kubevirt-bot · 2024-05-28T17:44:11Z

@Sreeja1725: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

alaypatel07 · 2024-05-28T17:46:25Z

/test pull-kubevirt-unit-test
/test pull-kubevirt-build

alaypatel07 · 2024-05-29T11:46:59Z

/test pull-kubevirt-build

alaypatel07 · 2024-05-29T12:10:18Z

/test pull-kubevirt-unit-test

alaypatel07 · 2024-05-29T13:03:32Z

LGTM. Thanks for getting this ready, great work @Sreeja1725!

@xpivarc @EdDev this PR seems to be ready for a review. PTAL

/ok-to-test

alaypatel07 · 2024-05-29T13:08:01Z

staging/src/kubevirt.io/api/roundtrip/compatibility.go

@@ -0,0 +1,426 @@
+package roundtrip


nit: this and the other new go files need a license header. See this for reference:

https://github.com/kubevirt/kubevirt/blob/main/pkg/monitoring/rules/alerts/virt-controller.go#L1-L15

Done, updated

alaypatel07 · 2024-05-29T13:11:41Z

staging/src/kubevirt.io/api/roundtrip/compatibility.go

+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/runtime/serializer/json"
+	"k8s.io/apimachinery/pkg/util/sets"
+	v1 "kubevirt.io/api/core/v1"


nit: change the name to kubevirtv1

kubevirtv1 "kubevirt.io/api/core/v1"

alaypatel07 · 2024-05-29T13:17:04Z

staging/src/kubevirt.io/api/roundtrip/compatibility.go

+	"k8s.io/apimachinery/pkg/util/sets"
+	v1 "kubevirt.io/api/core/v1"
+)
+


can we please attribute the original tests here in the comments?

// This tests are largely take from k8s apitesting, in the future when it is possible it could be imported directly: // https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/apitesting/roundtrip/roundtrip.go

Signed-off-by: svarnam <svarnam@nvidia.com>

alaypatel07 · 2024-05-29T16:01:25Z

staging/src/kubevirt.io/api/roundtrip/compatibility.go

@@ -0,0 +1,445 @@
+/*
+Copyright 2023 The KubeVirt Authors.


s/2023/2024/g

Signed-off-by: svarnam <svarnam@nvidia.com>

alaypatel07 · 2024-05-29T16:50:45Z

/test all

/lgtm

kubevirt-bot · 2024-05-29T18:14:33Z

@Sreeja1725: 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-e2e-k8s-1.30-sig-operator	`ef4c711`	link	true	`/test pull-kubevirt-e2e-k8s-1.30-sig-operator`

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-sigs/prow repository. I understand the commands that are listed here.

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels May 6, 2024

kubevirt-bot requested review from fossedihelm and lyarwood May 6, 2024 18:11

kubevirt-bot added the size/XXL label May 6, 2024

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch from 7db22b1 to 83827c1 Compare May 7, 2024 11:34

kubevirt-bot assigned alaypatel07 May 7, 2024

lyarwood reviewed May 7, 2024

View reviewed changes

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch from 83827c1 to 34d0795 Compare May 8, 2024 12:19

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels May 8, 2024

alaypatel07 reviewed May 8, 2024

View reviewed changes

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch 2 times, most recently from 06b792e to 8061198 Compare May 15, 2024 06:17

kubevirt-bot requested a review from EdDev May 15, 2024 13:18

alaypatel07 suggested changes May 16, 2024

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2024

kubevirt-bot added sig/compute sig/scale needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 28, 2024

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch 3 times, most recently from 3c1f9c9 to 1cc40e4 Compare May 28, 2024 17:11

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2024

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch from 1cc40e4 to 7d5aab7 Compare May 28, 2024 17:42

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch from 7d5aab7 to f49f7f6 Compare May 29, 2024 06:28

alaypatel07 reviewed May 29, 2024

View reviewed changes

Generate API testdata

cd4b000

Signed-off-by: svarnam <svarnam@nvidia.com>

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch from f49f7f6 to 202c0db Compare May 29, 2024 13:46

alaypatel07 reviewed May 29, 2024

View reviewed changes

Add unit tests to check for API backward compatibility

ef4c711

Signed-off-by: svarnam <svarnam@nvidia.com>

Sreeja1725 force-pushed the sreeja-kubevirt-api-testing branch from 202c0db to ef4c711 Compare May 29, 2024 16:10

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2024

		@@ -0,0 +1,21 @@
		### Description:

		1. This creates JSON and YAML files for all the API exposed by kubevirt in group-version "kubevirt.io/v1",


		This directory tree contains serialized API objects in json and yaml formats.

		This creates JSON and YAML files for all the API exposed by kubevirt in group-version "kubevirt.io/v1", versioned by the release. The current version is in `HEAD` directory, previous versions are in `release-0.yy` releasedirectory. APIs includes, more APIs can be added in the future:

		2. api-field: `status.runtimeUser` field was added[ref-3](https://github.com/kubevirt/kubevirt/pull/6709)


		## REVIEWRS GUIDE


		## REVIEWRS GUIDE

		Upon upgrade to API, the json and YAML files will be updated.

Add unit tests to check for API backward compatibility #11856

Are you sure you want to change the base?

Add unit tests to check for API backward compatibility #11856

Conversation

Sreeja1725 commented May 6, 2024

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

kubevirt-bot commented May 6, 2024

kubevirt-bot commented May 6, 2024

alaypatel07 commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alaypatel07 May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented May 15, 2024

alaypatel07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sreeja1725 commented May 17, 2024

Sreeja1725 commented May 28, 2024

kubevirt-bot commented May 28, 2024

kubevirt-bot commented May 28, 2024

alaypatel07 commented May 28, 2024

Sreeja1725 commented May 28, 2024

kubevirt-bot commented May 28, 2024

alaypatel07 commented May 28, 2024

alaypatel07 commented May 29, 2024

alaypatel07 commented May 29, 2024

alaypatel07 commented May 29, 2024

alaypatel07 May 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alaypatel07 commented May 29, 2024

kubevirt-bot commented May 29, 2024

alaypatel07 May 8, 2024 •

edited

alaypatel07 May 29, 2024 •

edited