Skip to content

Conversation

@m-ildefons
Copy link
Member

Add test which checks if the CRD versions defined in the cluster match the versions in the subcharts in the git repo.
This test ensures that the subcharts in the release version don't unexpectedly change their API versions within a minor Harvester release.

To implement this test, additional infrastructure for the Kubernetes API has been added.

fixes: #1314

@m-ildefons m-ildefons requested a review from albinsun June 10, 2024 10:37
@m-ildefons m-ildefons self-assigned this Jun 10, 2024
@m-ildefons m-ildefons force-pushed the wip/test-crd-versions branch 3 times, most recently from 943710d to 26cc904 Compare June 10, 2024 10:59
Add test which checks if the CRD versions defined in the cluster match
the versions in the subcharts in the git repo.
This test ensures that the subcharts in the release version don't
unexpectedly change their API versions within a minor Harvester release.

To implement this test, additional infrastructure for the Kubernetes API
has been added.

fixes: harvester#1314

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
Copy link
Member

@lanfon72 lanfon72 left a comment

Choose a reason for hiding this comment

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

IMO, the library apiclient is focusing for Harvester and related endpoint, we don't and will not have plan to implement kube APIs, as it already have many other libs does.

and as Albin's snippets, it shows we can simply get those CRD info from bash, so I would prefer use that version rather than create a new API set.

It would be more readable and fit our manual test steps.

and I don't think we need to verify those CRDs every time, usually we will only verify them after the cluster upgraded into new version, so we will not place then in apis.

assert code == 200
assert data.get("value") is not None

yield semver.VersionInfo.parse(data.get("value").lstrip("v"))
Copy link
Member

Choose a reason for hiding this comment

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

we already have apiclient.cluster_version to expose the cluster's version.

Copy link
Member Author

Choose a reason for hiding this comment

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

The apiclient.cluster_version returns a pkg_resources.Version object, but my tests are using the semver library to parse versioning information.
The semver library has several advantages. One being that it isn't in the process of being deprecated. Another advantage is that it has good documentation and deals with a lot of different operations on version information better. Third, the pkg_resources library is supposed to deal with python-packages and not generic version information. This is a difference in so far as python packages have specific requirements on the format of the version string that we don't adhere to with Harvester

@albinsun
Copy link
Contributor

albinsun commented Jun 17, 2024

Test target and classification are important, we should not mix different purpose test code.
However we also want to automate tests rather than manual, (of course we can keep both of them for specific timing of test.)

Or we can have this automation in another folder or something thus not break test organization? How do you guys think?

@m-ildefons
Copy link
Member Author

IMO, the library apiclient is focusing for Harvester and related endpoint, we don't and will not have plan to implement kube APIs, as it already have many other libs does.

Well, it does not. There's also a Rancher API client part in it.
I also don't just need a generic K8s client, I need a K8s client library where I can just give it a Harvester endpoint, username and password and it deals with the authentication. This isn't the case for normal K8s API client libraries (as you can tell, because I've had to implement that part).

and as Albin's snippets, it shows we can simply get those CRD info from bash, so I would prefer use that version rather than create a new API set.

It would be more readable and fit our manual test steps.

Please no. We have a test suite, let's not add a second test suite.
Bash is also a terrible choice for maintaining any significant amount of code, since it quickly becomes very hard to read.

and I don't think we need to verify those CRDs every time, usually we will only verify them after the cluster upgraded into new version, so we will not place then in apis.

I see your point, but I don't see where else in the test suite an appropriate place would be. Maybe we should create a separate directory for upgrade tests under harvester_e2e_tests/upgrade?

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
@lanfon72
Copy link
Member

IMO, the library apiclient is focusing for Harvester and related endpoint, we don't and will not have plan to implement kube APIs, as it already have many other libs does.

Well, it does not. There's also a Rancher API client part in it. I also don't just need a generic K8s client, I need a K8s client library where I can just give it a Harvester endpoint, username and password and it deals with the authentication. This isn't the case for normal K8s API client libraries (as you can tell, because I've had to implement that part).

and as Albin's snippets, it shows we can simply get those CRD info from bash, so I would prefer use that version rather than create a new API set.
It would be more readable and fit our manual test steps.

Please no. We have a test suite, let's not add a second test suite. Bash is also a terrible choice for maintaining any significant amount of code, since it quickly becomes very hard to read.

and I don't think we need to verify those CRDs every time, usually we will only verify them after the cluster upgraded into new version, so we will not place then in apis.

I see your point, but I don't see where else in the test suite an appropriate place would be. Maybe we should create a separate directory for upgrade tests under harvester_e2e_tests/upgrade?

  1. We will not implement all Rancher APIs, we would only implement those parts related to Rancher Integration, so the RancherAPI is not be included by default. and we have VCluster addon to enable embedded Rancher, so it is make sense to have the API client.
  2. This repo is for test automation, not a product to modify frequently. For test automation, readability, simply and manually reproducible are important than others. That's why I would prefer to have a bash version and not include another library, less dependencies would let us more easily to address the root cause when test fails.
  3. Ideally, we will check CRD versions after cluster upgraded, so the automation test cases should be placed into upgrade.

@irishgordo
Copy link
Contributor

irishgordo commented Mar 12, 2025

For what it's worth.

semver as a dependency is not very bloated.

du -h -d 1 "$(pip -V | cut -d ' ' -f 4 | sed 's/pip//g')" | grep -vE "dist-info|_distutils_hack|__pycache__" | sort -h

semver:

128K	/home/mike/.local/lib/python3.10/site-packages/semver

kubernetes as a dependency has a bit more size to it:

33M	/home/mike/.local/lib/python3.10/site-packages/kubernetes

But, again it's kubernetes, and I'd expect that package to be a bit bigger.

But even for our packages in requirements.txt :

We are not pinning our dependencies, but maybe we could start?

I get that yeah we could do it in /bin/bash, but yeah that also has other dependencies, taking something like (say you've curl'd all the githubusercontent yamls down to /home/rancher/harvester_yamls):

#! /usr/bin/bash
for file in /home/rancher/harvester_yamls/*; do
    # get the name of the file
    file_name=$(basename $file)
    # get the name of the crd
    crd_name=$(cat $file | yq .metadata.name)
    # get the object path
    obj_path='.spec.versions[].name'
    # get the observation
    observation=$(kubectl get Customresourcedefinitions/$crd_name -o yaml | yq $obj_path | tr '\n' ' ')
    # get the expectation
    expectation=$(cat $file | yq $obj_path | tr '\n' ' ')
    # print the observation and expectation
    echo -e "For $file - Observation: $observation\nExpectation: $expectation\n"
    # check if the observation is equal to the expectation
    [ "$observation" == "$expectation" ] && echo True || echo False
done

But the bash is requiring the Harvester node to have the binaries of:

  • yq
  • kubectl
  • curl
  • others like echo, tr, etc.

Which our Harvester nodes do have those binaries, but it just ends up shifting the dependencies on to Harvester - and Harvester could change at a later point to not support those things, hypothetically, ...but our testing code would instead continue to work because we wouldn't be dependent on the Harvester node actually containing binaries to perform checks written in bash.

So I also agree that it may be better to not rely on a bash script to check the CRD dependencies.

Maybe we could retrofit the apiclient.cluster_version (both Harvester & Rancher) to utilize the semver library, but that might be a big refactor?

To not block @m-ildefons , I'm okay approving this for now and maybe we could iterate on it later -> as long as there's a test run that's been ran to show it passing 😄

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TEST] [Release Testing] Test installed CRD API versions of system charts

4 participants