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

KEP test cluster framework #2178

Closed
wants to merge 10 commits into from
Closed

KEP test cluster framework #2178

wants to merge 10 commits into from

Conversation

hoegaarden
Copy link
Contributor

Proposal

We propose an abstraction so that users and tools (e.g. CI) have a consistent
way to spin up and configure clusters they can test against. This abstraction
is targeted mainly to the needs of the following personas and should work
equally well used interactively and non-interactively / automated.

See also: https://docs.google.com/document/d/13bMjmWpsdkgbY-JayrcU-e_QNwRJCP-rHjtqdeeoQHo/edit#

/sig testing

/cc @totherme
/cc @mariantalla
/cc @timothysc
/cc @liztio
/cc @marun

@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 23, 2018
@k8s-ci-robot
Copy link
Contributor

@hoegaarden: GitHub didn't allow me to request PR reviews from the following users: mariantalla.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Proposal

We propose an abstraction so that users and tools (e.g. CI) have a consistent
way to spin up and configure clusters they can test against. This abstraction
is targeted mainly to the needs of the following personas and should work
equally well used interactively and non-interactively / automated.

See also: https://docs.google.com/document/d/13bMjmWpsdkgbY-JayrcU-e_QNwRJCP-rHjtqdeeoQHo/edit#

/sig testing

/cc @totherme
/cc @mariantalla
/cc @timothysc
/cc @liztio
/cc @marun

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels May 23, 2018
@timothysc timothysc self-assigned this May 30, 2018
@timothysc
Copy link
Member

This is definitely on my todo list, but right now I'm booked trying to get 1.11 out the door.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: timothysc

Assign the PR to them by writing /assign @timothysc in a comment when ready.

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:

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


### Goals

1. To design an abstraction of “standing up a kubernetes cluster” which is:
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this differ from what kubetest is trying to accomplish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't know about kubetest before. The intent seems indeed very similar! From an (admittedly very quick) first try there might be room for some UX improvements, specifically for the use case of (new-ish) contributors that want to run it as part of their unit/integration test suite (and would be after fast feedback and low setup overhead).

We'll keep looking at kubetest and come back with more specific suggestions.

Is there additional documentation besides the README in the test-infra repo and the godocs?

cc @mariantalla

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah kubetest is hard to use on its own right now, but I'd be interested in trying to fix that.

The other documentation is: https://github.com/kubernetes/community/blob/master/contributors/devel/e2e-tests.md (hack/e2e.go is just a wrapper to call kubetest)

Copy link
Member

Choose a reason for hiding this comment

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

We're trying to standardize the client tooling abstraction for re-use. I think there is some overlap of concerns, but there are other constraints we are trying to solve here.

## Motivation

With this proposal we would like to address these three points of friction:
- For Carol, we introduce an abstraction which is intended to unify existing
Copy link
Member

Choose a reason for hiding this comment

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

is there something special about a test cluster versus any other cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One example: A partial cluster (running only the apiserver & etcd) might be perfectly fine for a certain type of tests.

Copy link
Member

Choose a reason for hiding this comment

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

@derekwaynecarr - Also (N) nodes on a single host is another example. Similar to other DIND clusters.

Copy link

Choose a reason for hiding this comment

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

Ability to specify cluster and node type for a suite of tests with specific requirements
is a huge advantage. If the abstraction is provider agnostic even better.

methods of standing up clusters
- equally easy to use interactively and in CI
2. To build a lightweight testing framework for CLI use which
- is not dependant on `k8s.io/kubernetes`
Copy link
Member

Choose a reason for hiding this comment

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

this implies that k8.io/kubernetes vendors this test framework?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It should be possible for k/k to vendor this framework.

- To wrap any existing “ways of standing up a cluster” in our abstraction
- Of course, wrapping existing technology may well be the most efficient way
to meet goals 2 and 3 (building CLI and API/Controller testing frameworks).
- To port large numbers of existing tests to use our abstraction.
Copy link
Member

Choose a reason for hiding this comment

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

absent this, are we just creating a new standard?

Copy link
Contributor Author

@hoegaarden hoegaarden Jun 21, 2018

Choose a reason for hiding this comment

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

To understand if this framework is useful we were thinking of porting a couple of tests for each (or most) of the test suites. An example of how such a port could look can be found here, where we experimented with porting different tests from test-cmd.sh. And I believe we should port not only tests from test-cmd.sh but also other tests.

But porting all the tests is not in the scope of this KEP.

@BenTheElder
Copy link
Member

How does this relate to the cluster api? AFAIK having a consistent way to spin up clusters is a goal already, more broadly than just test clusters.

@krzyzacy
Copy link
Member

cc @krousey ^^

@hoegaarden
Copy link
Contributor Author

hoegaarden commented Jun 21, 2018

@BenTheElder
That's a good point. I think we need to have a close look on the work that sig-cluster-api is doing.

Ultimately, users of the test framework would need to be able to describe the shape of clusters they need in their tests, i.e. number of worker nodes, number of master nodes. In this context, we're not clear yet what the relationship would be between Cluster API and the testing framework. Would using the cluster api become an implementation detail of a specific test cluster implementation - put differently, would some cluster types leverage the cluster API and others not? Or, would the testing framework implement the cluster API itself?

cc @mariantalla

@BenTheElder
Copy link
Member

BenTheElder commented Jun 21, 2018 via email

@timothysc
Copy link
Member

I think we can channel much of cluster-api TBH. There are concerns that aren't covered there though, and we may be overloading the heck out of the ProviderConfig.

/cc @detiber @liztio

@k8s-ci-robot k8s-ci-robot requested a review from detiber June 26, 2018 14:25
### Goals

1. To design an abstraction of “standing up a kubernetes cluster” which is:
- expressive enough that most or all e2e tests ought to be able to use it,
Copy link
Member

Choose a reason for hiding this comment

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

our e2e tests do not stand up clusters directly today? they expect a cluster to be available in $KUBECONFIG, tooling above that does standup.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, this is necessary for say, conformance test e2es, which must not be aware of the underlying provider at all, just talking to "a cluster" of some sorts, provisioned elsewhere.

There is a clear tension between Alice and Carol’s frustrations. Left to their
own devices, Alice is likely to contribute to the proliferation of test
infrastructure that frustrates Carol, and Carol may find herself advocating
that Alice be prevented from acting to resolve her own frustration with the
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I've misread this persona section, but it reads to me as somewhat hostile to Alice, which I don't approve of.

I don't think anything we check in as a KEP should suggest preventing others from developing anything, let alone test infrastructure...? Why can't "Carol" let "Alice" build whatever tooling they want?

Copy link
Member

Choose a reason for hiding this comment

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

This whole section beyond "We propose an abstraction so ..." (anything after the first paragraph) seems fairly unnecessary. I'd suggest a motivation more along the lines of "we'd like a standard framework for spinning up clusters for testing with varying requirements and backing implementations" or something along those lines.

@stevekuznetsov
Copy link
Contributor

stevekuznetsov commented Jul 12, 2018

I would second Ben's comments that I think it would be good for this to sync with cluster API as that approach (declarative k8s-style objects backed by controllers) seems to be a very powerful approach and unless there is some architectural reason blocking "clusters that tests run against" from being stood up using it, it seems like converging on one approach for "clusters that tests run against" and production clusters would be good. I concur that the concept of a "test cluster' is a little problematic as it necessarily divides that from "cluster". If the current cluster API controllers don't allow specific cluster configurations, why not create custom controllers on those objects to achieve what would be necessary for testing?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2018
@k8s-ci-robot
Copy link
Contributor

@hoegaarden: PR needs rebase.

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.

@BenTheElder
Copy link
Member

This came up today at sig-testing, I think I might understand the goals a bit better now... See https://github.com/kubernetes/test-infra/tree/master/kind for some early tooling that may relate.

@mariantalla
Copy link
Contributor

@BenTheElder Will take a look at kind, thanks. Is there anything that can be improved in the KEP phrasing to help with expressing the goals more clearly? I'll have a look at the notes from the last meeting too.

cc @hoegaarden

@justaugustus
Copy link
Member

/kind kep

@justaugustus
Copy link
Member

REMINDER: KEPs are moving to k/enhancements on November 30. Please attempt to merge this KEP before then to signal consensus.
For more details on this change, review this thread.

Any questions regarding this move should be directed to that thread and not asked on GitHub.

@justaugustus
Copy link
Member

KEPs have moved to k/enhancements.
This PR will be closed and any additional changes to this KEP should be submitted to k/enhancements.
For more details on this change, review this thread.

Any questions regarding this move should be directed to that thread and not asked on GitHub.
/close

@k8s-ci-robot
Copy link
Contributor

@justaugustus: Closed this PR.

In response to this:

KEPs have moved to k/enhancements.
This PR will be closed and any additional changes to this KEP should be submitted to k/enhancements.
For more details on this change, review this thread.

Any questions regarding this move should be directed to that thread and not asked on GitHub.
/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
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.