Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Adding kubernetes-e2e-gce-federation to list of merge blocking suites #1198

Conversation

nikhiljindal
Copy link
Contributor

@nikhiljindal nikhiljindal commented Jun 13, 2016

cc @kubernetes/sig-cluster-federation @lavalamp
Will wait for the test to be green for a while before merging this.

Ref kubernetes/kubernetes#26723


This change is Reviewable

@goltermann
Copy link
Contributor

@nikhiljindal What issue is blocking these tests being green?

@nikhiljindal
Copy link
Contributor Author

nikhiljindal commented Jun 15, 2016

Fixes waiting to be merged: kubernetes/kubernetes#27407, kubernetes/kubernetes#27411, kubernetes/kubernetes#27466 and kubernetes/kubernetes#27390.
Some of the other fixes are being discussed in kubernetes/kubernetes#26762

@goltermann
Copy link
Contributor

@nikhiljindal each of the 4 blockers you mentioned have been merged.

@nikhiljindal
Copy link
Contributor Author

The tests are still not green.
Sent #1242 so that we have automatic issue filing for failing tests.

@nikhiljindal
Copy link
Contributor Author

We should be ready to merge this once kubernetes/kubernetes#28615 is fixed.

@ghost
Copy link

ghost commented Jul 18, 2016

Yup, agreed.

@lavalamp
Copy link
Contributor

Do you have a presubmit test that covers this? I'm not turning on blocking post submits that aren't covered by presubmits.

@nikhiljindal
Copy link
Contributor Author

By presubmit, do you mean adding this suite to PR builder tests?
We have an issue open for it: kubernetes/test-infra#155, but havent implemented it yet.

@lavalamp
Copy link
Contributor

Yes.

On Jul 19, 2016 4:31 AM, "Nikhil Jindal" notifications@github.com wrote:

By presubmit, do you mean adding this suite to PR builder tests?
We have an issue open for it: kubernetes/test-infra#155
kubernetes/test-infra#155, but havent
implemented it yet.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#1198 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglmN9z_ogHhfhrSQbWBTggPznM4rTks5qXLWUgaJpZM4I01YQ
.

@ghost
Copy link

ghost commented Jul 19, 2016

Sorry, I typed up a long response to this yesterday, but must not have submitted it. I will try again, but in summary:

  1. we already have this problem (many test jobs, like -slow, -serial, and several others are already merge-blockers but not present in pre-submits.
  2. There is some work to do before Federation e2e tests can be added to presubmit checks (see the issue @nikhiljindal quotes above).

@ghost
Copy link

ghost commented Jul 19, 2016

@lavalamp I would strongly suggest that we merge this PR, and address kubernetes/test-infra#155 separately as the next step. Unless I'm missing something?

@ghost
Copy link

ghost commented Jul 19, 2016

belated comment from yesterday, that I accidentally failed to push the submit button on :-)

@lavalamp There is currently no way to run e2e tests against multiple cluster deployments in the presubmit tests (at least that I'm aware of). The presubmit tests currently spin up a single cluster, and run a predefined set of tests against that single cluster.

To add the federation tests to presubmit, we'd have to teach the pre-submit test infrastructure to spin up multiple "clusters" (in this case one of those would actually be a federation of clusters), and then run a different set of tests across each cluster, presumably in parallel, to keep elapsed time to a minimum).

Besides that, we already have this problem. The following test jobs are merge-blockers but not included in presubmit tests:

        "kubernetes-e2e-gce-slow",          
        "kubernetes-e2e-gce-serial",            
        "kubernetes-e2e-gke-serial",            
        "kubernetes-e2e-gke",               
        "kubernetes-e2e-gke-slow",              
        "kubernetes-e2e-gce-scalability",               
        "kubernetes-kubemark-5-gce",                

@lavalamp
Copy link
Contributor

We chatted offline and seem to agree that getting a presubmit test running first is best for the whole team, even though it will cause federation some pain until it is in.

@ghost
Copy link

ghost commented Jul 19, 2016

Ack! I agree.

@madhusudancs
Copy link
Contributor

madhusudancs commented Jul 21, 2016

Is there a middle ground here? Can we run federation tests against a single cluster in the pre-submit. That should help us catch breaking changes in the Kubernetes code we depend on (code that we import as libraries from k8s.io/kubernetes/pkg). For example, this would have also helped us catch kubernetes/kubernetes#29123.

I understand this is less useful than running the tests against multiple clusters, but it is not entirely useless. And it helps us to have something, until we resolve all the other issues. It is better than having nothing.

@lavalamp
Copy link
Contributor

I'm fine with adding that as a presubmit. Warning, though-- if we get a
bunch of post-submit breaks even with that in place, then I'll kick the
post-submit job to nonblocking.

On Thu, Jul 21, 2016 at 10:56 AM, Madhusudan.C.S notifications@github.com
wrote:

Is there a middle ground here? Can we run federation tests against a
single cluster in the pre-submit. That should help us catch breaking
changes in the Kubernetes code we depend on (code that we import as
libraries from k8s.io/kubernetes/pkg). For example, this would have
helped us catch kubernetes/kubernetes#29123
kubernetes/kubernetes#29123.

I understand this is less useful than running the tests against multiple
clusters, but it is not useless. And it helps us have something, instead of
having nothing, until we resolve all the other issues.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1198 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAngllalpRLI-Y_hsyogeY_GssZHO1Q5ks5qX7LmgaJpZM4I01YQ
.

@madhusudancs
Copy link
Contributor

@lavalamp fair enough.

@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot
Copy link

@nikhiljindal PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2016
@test-foxish
Copy link

recomputing cla status...

@k8s-github-robot
Copy link

[CLA-PING] @nikhiljindal

Thanks for your pull request. It looks like this may be your first contribution to a CNCF open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://identity.linuxfoundation.org/projects/cncf to sign.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.


@k8s-github-robot k8s-github-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 22, 2016
@k8s-github-robot
Copy link

[CLA-PING] @nikhiljindal

Thanks for your pull request. It looks like this may be your first contribution to a CNCF open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://identity.linuxfoundation.org/projects/cncf to sign.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.


Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/mungegithub cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants