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.

Sign up for GitHub

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

Jump to bottom

[Federation][init-11] Switch federation e2e tests to use the new federation control plane bootstrap via the kubefed init command. #35961

Closed
wants to merge 1 commit into from
Closed

[Federation][init-11] Switch federation e2e tests to use the new federation control plane bootstrap via the kubefed init command. #35961

wants to merge 1 commit into from

Conversation

madhusudancs
Copy link
Contributor

@madhusudancs madhusudancs commented Nov 1, 2016 •
edited
Loading

This is ready for review now

Please review only the last commit here. This is based on PR #36294 which will be reviewed independently.

Design Doc: PR #35960

cc @kubernetes/sig-cluster-federation @quinton-hoole @nikhiljindal

This change is Reviewable

@madhusudancs madhusudancs added area/cluster-federation release-note-none Denotes a PR that doesn't merit a release note. labels Nov 1, 2016
@madhusudancs madhusudancs added this to the v1.5 milestone Nov 1, 2016
@madhusudancs madhusudancs self-assigned this Nov 1, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 1, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 71e79ef281d4e2874bfe3b24b599667f6ea8b517. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 71e79ef281d4e2874bfe3b24b599667f6ea8b517. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 71e79ef281d4e2874bfe3b24b599667f6ea8b517. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 71e79ef281d4e2874bfe3b24b599667f6ea8b517. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 71e79ef281d4e2874bfe3b24b599667f6ea8b517. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 71e79ef281d4e2874bfe3b24b599667f6ea8b517. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 71e79ef281d4e2874bfe3b24b599667f6ea8b517. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@madhusudancs madhusudancs changed the title [WIP][Federation][init-11] Switch federation e2e tests to use the new federation control plane bootstrap via the kubefed init command. [Federation][init-11] Switch federation e2e tests to use the new federation control plane bootstrap via the kubefed init command. Nov 5, 2016
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 5, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 04c83ce. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 04c83ce. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

nikhiljindal
nikhiljindal reviewed Nov 6, 2016
View reviewed changes
federation/cluster/federation-up.sh
# create_cluster_secrets creates the secrets containing the kubeconfigs
# of the participating clusters in the host cluster. The kubeconfigs itself
# are created while deploying clusters, i.e. when kube-up is run.
function create_cluster_secrets() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Today e2e framework just brings up the federation control plane and each test has to register clusters.
In future, we should register the clusters in the e2e framework as well.

We can do this now since we have federated namespaces and each test runs its own independent namespace. We do not have to unregister and register clusters in each test.

Will not need this code then. We can use kubefed join.

Copy link
Contributor Author

@madhusudancs madhusudancs Nov 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I am not entirely sure if we want to do this in the framework or using kubefed join. Each e2e test should have the autonomy to make this decision. We have tests (and we want them) that test the API server without having the clusters registered. Pre-registering clusters will make it difficult to run these tests. They will have to deregister the pre-registered clusters before running their tests and then register those clusters back. It kind of reverses the setup and teardown order which will in turn make the tests hard to reason about and debug.

It also makes the tests complicated because, every test should now ensure that the resources are cleaned up in all the underlying clusters before ending the test.

We also need tests that incrementally register/deregister clusters. For example, start with 1 cluster, add two, delete one and so on and test how the controllers behave. We don't have good coverage for these kinds of operations today. But the point is, pre-registering clusters makes it hard to write these kinds of tests.

@nikhiljindal
Copy link
Contributor

thanks LGTM

@nikhiljindal
Copy link
Contributor

Also our e2e tests are broken right now because of #36287, so triggering federation e2e on this PR wont be helpful.

Please make sure that federation e2e tests pass with this PR.

cc @colhom

@nikhiljindal
Copy link
Contributor

An idea: Instead of replacing the existing scripts, we can let them be and add support for using kubefed to init using a USE_KUBEFED flag.
If this is true, then we use kubefed to bring up federation, else we use the existing scripts.
This provides an easy escape if we find some bug in new kubefed code.

@madhusudancs What do you think?

@madhusudancs
Copy link
Contributor Author

madhusudancs commented Nov 6, 2016 •
edited
Loading

@nikhiljindal yeah, that's a good idea. Plus, if there is anyone using the existing scripts, we don't unnecessarily break them.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2016
@dims
Copy link
Member

dims commented Nov 9, 2016

@nikhiljindal Does the milestone v1.5 stay on this one too?

@madhusudancs
Copy link
Contributor Author

@dims yeah. This should go into 1.5 for confidence.

@madhusudancs
[Federation][init-11] Switch federation e2e tests to use the new fede…
540eb9b 
…ration control plane bootstrap via the `kubefed init` command.
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 16, 2016
@ghost ghost mentioned this pull request Nov 17, 2016
Closed
22 tasks
@shashidharatd shashidharatd mentioned this pull request Nov 21, 2016
Merged
@dims
Copy link
Member

dims commented Nov 23, 2016

@matchstick @nikhiljindal @madhusudancs : last call for a review (OR) please move it to 1.6

@madhusudancs
Copy link
Contributor Author

@dims even if doesn't make it to v1.5.0, we will likely have to cherry pick this or PR #37215. I would leave the label as is. Does it help to add a non-release-blocker label?

@dims
Copy link
Member

dims commented Nov 23, 2016

yes, it does @madhusudancs please add non-release-blocker label

@dims
Copy link
Member

dims commented Dec 9, 2016

@madhusudancs so this stays on for a point release?

@madhusudancs
Copy link
Contributor Author

I am closing this in favor of PR #37215.

k8s-github-robot pushed a commit that referenced this pull request Dec 16, 2016
Merge pull request #37215 from shashidharatd/fed-ci
aa454ec 
Automatic merge from submit-queue

[Federation][init-11.2] use USE_KUBEFED env var to choose bw old and new federation deployment

This is continuation of #35961
USE_KUBEFED variable is used for deploying federation control plane. if not defined, federation will be brought up using old method i.e scripts.

Have verified that federation comes up using the old method, using following steps
```
$ export FEDERATION=true
$ export E2E_ZONES="asia-east1-c"
$ export FEDERATION_PUSH_REPO_BASE=gcr.io/<my-project>
$ KUBE_RELEASE_RUN_TESTS=n KUBE_FASTBUILD=true go run hack/e2e.go -v -build
$ build-tools/push-federation-images.sh
$ go run hack/e2e.go -v --up
```
Should merge #35961 before this PR

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

@nikhiljindal nikhiljindal nikhiljindal left review comments

Assignees

@nikhiljindal nikhiljindal

@matchstick matchstick

Labels
release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@madhusudancs @k8s-ci-robot @nikhiljindal @dims @googlebot @k8s-github-robot @matchstick