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

kubelet/rkt: Add KillPod() and GetPodStatus() for rkt. #7605

Merged
merged 1 commit into from May 1, 2015

Conversation

yifan-gu
Copy link
Contributor

@yifan-gu yifan-gu commented May 1, 2015

@vmarmol @dchen1107 @jonboulle @bakins

This adds KillPod() and GetPodStatus() .

Woo! Final PR to close #7244

We need another one or two for implementing the syncPod for rkt, so we can have it running!

⚡ ⚡ ⚡

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

LGTM

@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 1, 2015

Rebased, think the last three PRs will conflict with each other. But I will rebase (#7605 #7599 #7589)

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

LGTM

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

@yifan-gu and this one needs a rebase too

@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 1, 2015

Rebased. Think will need another one for this.

Git doesn't report conflict when both commits add the same thing. Awesome!

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

CI failure:

pkg/kubelet/rkt/rkt.go:551: r.GetPods undefined (type *Runtime has no field or method GetPods)

@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 1, 2015

I know, it depends that GetPods PR..

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

aaah, got it :)

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

@yifan-gu GetPods should be in

@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 1, 2015

@vmarmol Should be good now!

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

LGTM, will wait for the CI before merging. Looks like this is the last one for the day :) I have the SyncPod PR almost ready, will likely send out later tonight. Thanks for the heroic push @yifan-gu! The end is in sight.

@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 1, 2015

THANK YOU FOR FAST REVIEW AND MERGE!!!! @vmarmol !!
I will have the SyncPod for rkt tonight too!

Might need one or two more to make the interfaces same. And will need one for adding flags to enable rkt in kubelet!

🚀 🚀 🚀

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

In #7610 I do have the runningPod in SyncPod() I need to think a bit more if we need it or not. Safer if we have it...

@vmarmol
Copy link
Contributor

vmarmol commented May 1, 2015

IT IS GREEN! Merging.

vmarmol added a commit that referenced this pull request May 1, 2015
kubelet/rkt: Add KillPod() and GetPodStatus() for rkt.
@vmarmol vmarmol merged commit 72708d7 into kubernetes:master May 1, 2015
@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 1, 2015

@vmarmol Yes, as I remember, the pods are splitted into pod by pod workers. We need to rethink about that. But it's safer and smoother to have runningPod for now :)

@pmorie
Copy link
Member

pmorie commented May 1, 2015

Teamwork! 🏆

On Thu, Apr 30, 2015 at 9:53 PM, Yifan Gu notifications@github.com wrote:

@vmarmol https://github.com/vmarmol Yes, as I remember, the pods are
splitted into pod by pod workers. We need to rethink about that. But it's
safer and smoother to have runningPod for now :)


Reply to this email directly or view it on GitHub
#7605 (comment)
.

@yifan-gu yifan-gu deleted the rkt_killpod branch May 7, 2015 17:29
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.

None yet

4 participants