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

Revert "Temporary disables custom network for pull-kuberntes-e2e-gce-gpu job" #5080

Merged
merged 2 commits into from
Oct 19, 2017
Merged

Revert "Temporary disables custom network for pull-kuberntes-e2e-gce-gpu job" #5080

merged 2 commits into from
Oct 19, 2017

Conversation

MrHohn
Copy link
Member

@MrHohn MrHohn commented Oct 19, 2017 •
edited
Loading

Reverts #5021. xref #4472.

Hey folks, now kubernetes/kubernetes#54009 is in. Could we please try it again? :)

/assign @mindprince @krzyzacy

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 19, 2017
@krzyzacy
Copy link
Member

consider turn --check-leaked-resources on for a while, for better validation

@krzyzacy
Copy link
Member

(I'm fine with flip back on again, and since I'm oncall I'll watch the job if it's on 🔥 )

@rohitagarwal003
Copy link
Member

Yeah, I am fine with it as long as someone is watching it.

@BenTheElder
Copy link
Member

/lgtm
we can monitor these.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, MrHohn

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Oct 19, 2017

Thanks all!

consider turn --check-leaked-resources on for a while, for better validation

Ack, included in a new commit. PTAL

@k8s-ci-robot k8s-ci-robot merged commit c217047 into kubernetes:master Oct 19, 2017
@BenTheElder
Copy link
Member

so far things still look quite green :-) ✅

@BenTheElder
Copy link
Member

@MrHohn looks like there are leaks :-(
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/51760/pull-kubernetes-e2e-gce-gpu/10601/
https://k8s-gubernator.appspot.com/builds/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-e2e-gce-gpu

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017

Humm...It is the static IP for masters leaked, very likely irrelevant to the custom network change. Looking into it.

@krzyzacy
Copy link
Member

it seems match the timeline (everything after 3:38p)

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017

I suspect it is due to the --check-leaked-resources flag change.

@krzyzacy
Copy link
Member

on the other hand, we were not using --check-leaked-resources before, it might be an existing issue. @MrHohn let us know if we should revert or we can just turn --check-leaked-resources off since the network is not leaking

@krzyzacy
Copy link
Member

but is that mean we have problem with static ips all the time? that's worrisome 😂

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017 •
edited
Loading

I checked on the project, nothing is leaking, it seems like there is an issue with how we check the resource is leaked --- it might take a while for master IP to be eventually cleaned up on GCP, but the script think it is leaked.

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017

Ahhh I know

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017

We have multiple jobs running together, the script thinks master IPs for other jobs are leaked...I think we should not turn this flag on PR jobs...

@krzyzacy
Copy link
Member

hummmmmm I thought we checks for cluster prefix :-(

yeah let's turn off the flag

@MrHohn MrHohn mentioned this pull request Oct 20, 2017
Merged
@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017

Sent #5087.

@krzyzacy
Copy link
Member

@MrHohn gpu job looks so far so good, do we want to enable it for other presubmits as well?

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017

gpu job looks so far so good, do we want to enable it for other presubmits as well?

Any other non-blocking presubmit we could try?

@krzyzacy
Copy link
Member

why not enable it for blocking jobs :-)

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017 •
edited
Loading

why not enable it for blocking jobs :-)

What about proceeding to the first blocking job? I'm fine as long as we watch it :)

@krzyzacy
Copy link
Member

yeah, maybe do the gce presubmit, if that works we can propagate to all presubmits :-)

@MrHohn
Copy link
Member Author

MrHohn commented Oct 20, 2017

Sent #5098.

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

@rohitagarwal003 rohitagarwal003 rohitagarwal003 approved these changes

Assignees

@rohitagarwal003 rohitagarwal003

@BenTheElder BenTheElder

@krzyzacy krzyzacy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MrHohn @krzyzacy @rohitagarwal003 @BenTheElder @k8s-github-robot @k8s-ci-robot