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-0008: Multi-cluster - Proposals for TestCase, TestStep Changes #344

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Miles-Garnsey
Copy link

What this PR does / why we need it:

KEP-0008 is currently light on detail. To get it ready for implementation this PR fleshes out proposed changes to the TestCase and TestStep APIs.

The changes proposed are designed to allow for specification of what clusters a given TestStep should run on, as well as providing injection of appropriate environment variables identifying the kubeconfig and kube context that scripts and commands may require to access a given cluster.

One outstanding question, is whether (in addition to making environment variables available in scripts and commands) we should have some sort of automatic kubeconfig/kube context injection mechanism, so that a kubectl command which does not explicitly use the KUBECONFIG or KUBECONTEXT environment variables automatically has the appropriate argument inserted. If we did include this, we should also include a field to disable that functionality.

@Miles-Garnsey
Copy link
Author

Miles-Garnsey commented Mar 10, 2022

I've added some info here too around the reporting APIs. Initially I thought maybe a modification to TestCase was required. Having examined a bit more carefully, I think the Failure struct is the one that needs to be updated to reflect information about which cluster experienced the failure.

I am open to feedback there.

@Miles-Garnsey Miles-Garnsey marked this pull request as draft March 10, 2022 04:38
@Miles-Garnsey Miles-Garnsey force-pushed the kep-0008-multicluster/teststep-proposals branch from 1779ffa to 095455a Compare March 10, 2022 04:40
@Miles-Garnsey Miles-Garnsey marked this pull request as ready for review March 10, 2022 04:55
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

open questions below. Reporting section is simple and looks good

## Proposal
## Proposals - TestStep API changes

The proposal is to add a new setting within `TestStep`. We propose this field be called `runOnCluster` and of type `[]string`. This setting would allow the user to specify a list of the clusters set up by the `TestSuite` on which to run `TestSteps`.
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'm not following now... I was seeing runOnCluster as 1 cluster string type. here it is an array. Is the proposal intending to have a single defined test to be responsible for more than 1 cluster? if true... do you see a defined order? or run concurrently?

Copy link
Author

Choose a reason for hiding this comment

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

Is the proposal intending to have a single defined test to be responsible for more than 1 cluster? if true... do you see a defined order? or run concurrently?

Yes, I am thinking that a TestStep will run on multiple clusters indeed. I was thinking that running concurrently was probably best, as I think we want synchronisation to be handled via the commencement/end of a TestStep.

What do you think? My rationale is that (at least for the tests I've been writing) we'd probably want most steps to run on all clusters, and it is the exception where we want the clusters to diverge. But if others have different requirements I'm keen to hear about them.

```

If the kubeconfig setting is not set, then the global Kubernetes client is used.
If the runOnCluster setting is not set, then the `TestStep` will be run on all clusters.
Copy link
Member

Choose a reason for hiding this comment

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

it seems like we should have a configuration phase which sets all cluster to run on. In this approach, it would be good if we see no defined clusters to mean what it means today.

Copy link
Author

Choose a reason for hiding this comment

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

it would be good if we see no defined clusters to mean what it means today.

I'm hoping that this backwards compatibility is a consequence of what I'm proposing indeed.

I was conceptualising the current arrangement as one where the tests are run on all clusters BUT there is actually only one cluster to run on. Which gives backwards compatibility if runOnCluster (or however we name that parameter) is unset.

Let me know if that makes sense.

it seems like we should have a configuration phase which sets all cluster to run on.

Yes, I think that this should happen per TestStep is the only thing, and be based on clusters declared via the TestSuite so that we can refer to a kubeconfig + kube context pair using strings.


If the kubeconfig setting is set, then it will be used for all Kubernetes operations within the step: commands (the KUBECONFIG environment variable will be set to the kubeconfig setting, relative to the KUTTL CLI's working directory), applied resources, asserts, and errors. This means that a single step can only be configured to use a single kubeconfig, but multiple steps can be used if a test case needs to interact with more than one cluster. This allows, for example, a federated resource to be created in one step and then another step can be used to verify that it actually exists on the destination cluster.
Where multiple clusters are specified (including if the `runOnCluster` field is unset), each script and command will be run in parallel on the desired clusters.
Copy link
Member

Choose a reason for hiding this comment

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

what are your thoughts of defining the cluster in the config phase, then there is no unset at test run. This may be the first phase of work... to mod the structures for multi-cluster support, then make it work for the current single cluster support, then add more code for full multi-cluster support

Copy link
Author

@Miles-Garnsey Miles-Garnsey Mar 17, 2022

Choose a reason for hiding this comment

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

defining the cluster in the config phase

It depends on whether we're thinking the config phase is the TestSuite config phase or a TestStep config phase. My feeling is that the TestSuite should declare the clusters that will be used and the TestStep should then define which subset of those clusters the TestStep will run on.

I do generally agree that we should probably define the API, make it work for the current single-cluster behaviour, and then add multi-cluster. Seems like the safest way to ensure that we don't break any existing behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants