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

Add a simple e2e test for the presence of consumed capacity in SPCs #1076

Closed

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Nov 29, 2024

Add a simple e2e test for the presence of consumed capacity in the space provisioner configs. We cannot do much more because we cannot mess with the registered members, nor can we add/remove new members.

Also, tests for appearance and disappearance of toolchain clusters and the reaction of SPCs on that were removed, because the readiness of the SPCs is now more complex and cannot be easily tested in e2e tests without the ability to also add/remove whole new members clusters. This functionality is extensively covered in the host-operator unit tests though.

Paired PRs:

the space provisioner configs. We cannot do much more because we cannot
mess with the registered members, nor can we add/remove new members.
Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I do have one question - why can't we keep using the current approach that's in the tests now , I'm referring to copying the toolchainclusters and creating new SPC's with the reference to the copy? I see that the SPC status is influenced by the TC ready condition : https://github.com/codeready-toolchain/host-operator/pull/1108/files#diff-9b844eb6c3d858337767be70add36b6915bd0d4dcc4df8cee332b5e9eb057dcdR169 .

@metlos
Copy link
Contributor Author

metlos commented Dec 4, 2024

Looks good 👍

I do have one question - why can't we keep using the current approach that's in the tests now , I'm referring to copying the toolchainclusters and creating new SPC's with the reference to the copy? I see that the SPC status is influenced by the TC ready condition : https://github.com/codeready-toolchain/host-operator/pull/1108/files#diff-9b844eb6c3d858337767be70add36b6915bd0d4dcc4df8cee332b5e9eb057dcdR169 .

The readiness of the SPC was before only reliant on the status of the TC. You could have a disabled SPC that was ready. This enabled us to copy SPCs and TCs without affecting the state of the cluster for the rest of the parallel tests. E.g. Having an additional TC, in effect a new member cluster, wouldn't affect the rest of the testsuite because the SPC was always disabled in these tests and therefore these new "synthetic" members wouldn't participate in the provisioning.

With the SPC readiness now being a true reflection of whether the associated cluster can be provisioned to, this no longer is true. Copying the TC and having a ready SPC for it would make it eligible for provisioning, essentially creating another member. This would mess up other tests that assume there are 2 members.

Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Great Job 👍

Thanks for explaining the reason behind the simplification of the e2e test. I agree that unit tests should suffice.

@openshift-ci openshift-ci bot added the approved label Dec 4, 2024
Copy link

sonarqubecloud bot commented Dec 5, 2024

Copy link
Collaborator

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

Nice simplification 👍

Copy link

openshift-ci bot commented Dec 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MatousJobanek, metlos, mfrancisc

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:
  • OWNERS [MatousJobanek,mfrancisc]

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

@MatousJobanek
Copy link
Collaborator

@metlos you could add a check to the usersignup_test.go verifying that when the threshold is reached, then the SPC CR is put into an unready state. We already have these test cases there, so it's just matter of adding the checks of the condition of the SPC CRs. And it's completely sufficient to do it in one or two cases, we don't need to update all the cases there.

@metlos
Copy link
Contributor Author

metlos commented Dec 6, 2024

Closing this in favor of #1079

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.

4 participants