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

add NetworkStatus in NetworkPlugin interface for kubelet to consume #24693

Merged
merged 2 commits into from
May 4, 2016
Merged

add NetworkStatus in NetworkPlugin interface for kubelet to consume #24693

merged 2 commits into from
May 4, 2016

Conversation

freehan
Copy link
Contributor

@freehan freehan commented Apr 22, 2016 •
edited
Loading

ref: #24692

cc: @thockin @dcbw

@freehan freehan added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 22, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 22, 2016
yujuhong
yujuhong reviewed Apr 22, 2016
View reviewed changes
pkg/kubelet/kubelet.go
err = fmt.Errorf("Error on adding ip table rules: %v", err)
glog.Error(err)
kl.runtimeState.setNetworkState(err)
if kl.flannelExperimentalOverlay {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we consolidate all the network state initialization/syncing to this function? E.g., if pod CIDR is required for the network plugins, we can still check whether pod CIDR is set and then call setNetworkState(). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me try

@freehan freehan changed the title add capability NET_PLUGIN_CAPABILITY_NETWORK_BOOTSTRAP to kubenet and teach kubelet add NetworkStatus in NetworkPlugin interface for kubelet to consume Apr 23, 2016
@freehan freehan force-pushed the kubenetbootstrapfix branch 3 times, most recently from ad9f075 to ba3bb50 Compare April 25, 2016 17:59
yujuhong
yujuhong reviewed Apr 25, 2016
View reviewed changes
pkg/kubelet/runtime.go
if configureNetwork {
networkError = fmt.Errorf("network state unknown")
}
var networkError = fmt.Errorf("network state unknown")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just initialize in line 97.

@yujuhong
Copy link
Contributor

Looks good overall with a nit. Also, the kubelet unit test failed.

@freehan freehan force-pushed the kubenetbootstrapfix branch 2 times, most recently from 332d60c to e3c9970 Compare April 25, 2016 21:04
@freehan
Copy link
Contributor Author

freehan commented Apr 25, 2016

@k8s-bot e2e test this issue: #24642

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2016
@freehan
Copy link
Contributor Author

freehan commented Apr 26, 2016 •
edited
Loading

Not sure why this happens

!!! 'gofmt -s' needs to be run on the following files: 
./pkg/api/v1/generated.pb.go
FAILED   ./hack/../hack/verify-gofmt.sh 7s

@freehan
Copy link
Contributor Author

freehan commented Apr 26, 2016

@k8s-bot test this issue: #IGNORE

@dcbw
Copy link
Member

dcbw commented Apr 26, 2016

What's the forward plan here for NetworkStatus()? Is it intended to report plugin status out-of-band from setup/teardown operations?

@freehan
Copy link
Contributor Author

freehan commented Apr 26, 2016

@dcbw Yes. Kubelet tries to keep track of network status and it already runs a SyncNetworkStatus loop. But network plugin currently does not has an interface to expose its own status.

BTW, I want to rename the current Status interface to something like GetPodNetworkStatus

@yujuhong
Copy link
Contributor

BTW, I want to rename the current Status interface to something like GetPodNetworkStatus

+1 for renaming the method

@freehan
Copy link
Contributor Author

freehan commented Apr 27, 2016

PTAL

yujuhong
yujuhong reviewed Apr 27, 2016
View reviewed changes
pkg/kubelet/network/plugins.go
GetPodNetworkStatus(namespace string, name string, podInfraContainerID kubecontainer.ContainerID) (*PodNetworkStatus, error)

// NetworkStatus returns error if the network plugin is in error state
NetworkStatus() error
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this can be rename to Status()

@freehan
Copy link
Contributor Author

freehan commented Apr 27, 2016

Renamed. PTAL

@yujuhong
Copy link
Contributor

LGTM

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2016
@freehan
Copy link
Contributor Author

freehan commented Apr 27, 2016

Thanks!

@freehan
Copy link
Contributor Author

freehan commented Apr 29, 2016

@k8s-bot test this issue: #24453

@freehan freehan added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 2, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2016
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 2, 2016
@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2016
@dcbw
Copy link
Member

dcbw commented May 2, 2016

@freehan I'm hoping that syncNetworkStatus() will eventually go away when kubenet replaces the configureCBR0 code. But I guess the current use of the network plugin status for kubenet is OK, so LGTM too.

@freehan
Copy link
Contributor Author

freehan commented May 2, 2016

@k8s-bot test this issue: #24937

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2016
@k8s-bot
Copy link

k8s-bot commented May 4, 2016

GCE e2e build/test passed for commit 04b80f7.

@freehan freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2016
@freehan
Copy link
Contributor Author

freehan commented May 4, 2016

This PR fixes cluster bootstrap problem in kubenet e2e suite.
cc build cop: @dchen1107

@dchen1107 dchen1107 merged commit e09ce36 into kubernetes:master May 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yujuhong yujuhong

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@freehan @yujuhong @dcbw @k8s-bot @googlebot @dchen1107 @k8s-github-robot