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

Copy a subset of e2e templates to ClusterClass #3346

Closed
sedefsavas opened this issue Mar 24, 2022 · 12 comments
Closed

Copy a subset of e2e templates to ClusterClass #3346

sedefsavas opened this issue Mar 24, 2022 · 12 comments
Assignees
Labels
area/testing Issues or PRs related to testing lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sedefsavas
Copy link
Contributor

We only have a quick start e2e test ported to ClusterClass, missing templates that use ClusterClass patching.

To validate CAPA's ClusterClass support, we need to port some of the e2e tests to ClusterClass templates.

/priority important-soon
/milestone v1.x

@sedefsavas sedefsavas added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 24, 2022
@sedefsavas sedefsavas added this to the v1.x milestone Mar 24, 2022
@sedefsavas sedefsavas added the area/testing Issues or PRs related to testing label Mar 24, 2022
@sedefsavas sedefsavas changed the title Copy a subset e2e templates to ClusterClass Copy a subset of e2e templates to ClusterClass Mar 24, 2022
@yastij
Copy link
Member

yastij commented Mar 24, 2022

@sedefsavas @pydctw - an intersting bit is the upgrade tests and how we're resolving to the right AMI

@sedefsavas
Copy link
Contributor Author

Relevant issue to keep an eye on:
kubernetes-sigs/cluster-api#6320

In the tests, we will need to provide all fields in a list in the templates as a workaround.

@pydctw
Copy link
Contributor

pydctw commented Apr 4, 2022

/assign

@pydctw
Copy link
Contributor

pydctw commented Apr 4, 2022

Continuing discussion from 4/4/22 office hours.

There are ~20 e2e tests in unmanaged test suites (ClusterClass is not supported for EKS cluster at the moment) and only some of tests make sense to run with ClusterClass.

Options I've been thinking

  1. Re-use existing tests and make it select different flavors depending on e2e configuration variable. Existing test-grid pipeline will run as is (UseClusterClassTest=false) while we can create a new pipeline that sets UseClusterClassTest=true and run ClusterClass-based tests.

Pseudo code

if UseClusterClassTest == "true" {
   configCluster.Flavor = "multi-az-topology"
} else {
   configCluster.Flavor = "multi-az"
}
  1. Create a separate test file for ClusterClass based e2e tests and run them with ClusterClass-based templates.
    For example, unmanaged_functional_test.go -> unmanaged_functional_test_cluster_class.go similar to how quick_start tests are set up currently (unmanaged_CAPI_quick_test.go and unmanaged_CAPI_quick_test_cluster_class.go)

We can combine tests and refactor down the road but it will take some iterations and I plan to to port tests individually to make sure existing functionalities are verified correctly first and make debugging easier.

@sedefsavas @richardcase

@sedefsavas
Copy link
Contributor Author

sedefsavas commented Apr 4, 2022

@pydctw Thanks for listing out the options.

I'd opt for having a separate folder for ClusterClass tests for the following reasons:

  • e2e tests are very complicated and have high learning curve as it is. Combining ClusterClass logic into existing tests will make things more complicated as we might have to add customize logic specific to ClusterClass. Debugging will be easier if we have separate folders.
  • Combining multiple tests into one to reduce the number of tests is something we currently try to do with each new feature to reduce (at least not increase) the number of tests. This is a good opportunity to start from scratch and bundle the tests that makes sense together instead of having an exact replica of the flavors we have.
  • Since going forward we will heavily rely on ClusterClass, expecting to add different flavors for ClusterClass that we do not need to add to current tests.
  • Having those tests separate will allow us to deprecate tests without affecting one another.

@pydctw
Copy link
Contributor

pydctw commented Apr 4, 2022

Thanks for the input and it makes sense. A few follow-up questions

Combining multiple tests into one to reduce the number of tests is something we currently try to do with each new feature to reduce (at least not increase) the number of tests. This is a good opportunity to start from scratch and bundle the tests that makes sense together instead of having an exact replica of the flavors we have.

I agree with this goal in the long term but will start porting individual test first because 1) it is already difficult to write ClusterClass correctly, make e2e test work and confirm the parity with existing e2e test. As you said, e2e tests are very complicated and have high learning curve. 2) I don't have context to all the existing e2e tests so it will be difficult to combine them at this stage.

Having said that, my plan is once we have a few tests implemented using ClusterClass, will try to combine them as we see fit - refactoring and deleting code is easy to do once the foundation is set. I am a big believer of continuous refactoring.

@sedefsavas
Copy link
Contributor Author

If the purpose is to reduce code duplication, CAPI is doing it by having specs and calling them with different flavors instead of having flags to switch flavors.

My suggestion is not towards combining tests in the first iteration. We can have a separate file/folder and copy paste the test flavors that we want to replicate with ClusterClass. This will make bundling the tests in the next iteration easier + during debugging existing tests, we don't need to worry about ClusterClass logic.

@pydctw
Copy link
Contributor

pydctw commented Apr 4, 2022

I think we are saying the same thing. Option 1 proposed is to call different flavors as CAPI does. CAPI can do it without flags because their tests accepts flavors as an input variable while our test does not. I was trying to mimic the behavior using a config variable instead of extracting out each test as a separate file as CAPI does (https://github.com/kubernetes-sigs/cluster-api/blob/main/test/e2e/quick_start.go) and provide different flavors as an input.

Based on the discussion, I think I will start with option 2, meaning creating a separate file for ClusterClass test and see how it goes.

As mentioned, we can always change our approach down the road after a few tests are implemented.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 3, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 2, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues or PRs related to testing lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants