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

Sync node status from node controller to master. #3733

Merged
merged 1 commit into from
Jan 30, 2015
Merged

Sync node status from node controller to master. #3733

merged 1 commit into from
Jan 30, 2015

Conversation

ddysher
Copy link
Contributor

@ddysher ddysher commented Jan 22, 2015

@bgrant0607 @lavalamp @smarterclayton @erictune @brendandburns

e2e passes, I may pick up this issue #3520 if I can set up some time

@ddysher ddysher force-pushed the sync-node-status branch 4 times, most recently from a2413f6 to 95f17e9 Compare January 24, 2015 18:00
@ddysher ddysher changed the title WIP: Sync node status from node controller to master. Sync node status from node controller to master. Jan 24, 2015
@brendandburns brendandburns self-assigned this Jan 25, 2015
@ddysher ddysher mentioned this pull request Jan 25, 2015
Closed
@ddysher ddysher mentioned this pull request Jan 27, 2015
Closed
bgrant0607
bgrant0607 reviewed Jan 27, 2015
View reviewed changes
cmd/kube-controller-manager/controller-manager.go
@@ -102,6 +106,10 @@ func main() {
controllerManager := replicationControllerPkg.NewReplicationManager(kubeClient)
controllerManager.Run(10 * time.Second)

kubeletClient, err := client.NewKubeletClient(&kubeletConfig)
Copy link
Member

Choose a reason for hiding this comment

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

The apiserver does:
client.BindKubeletClientConfigFlags(flag.CommandLine, &kubeletConfig)

Is that done somewhere here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nop, updated.

@bgrant0607
Copy link
Member

FYI: #3695

/cc @davidopp

bgrant0607
bgrant0607 reviewed Jan 27, 2015
View reviewed changes
pkg/cloudprovider/controller/nodecontroller.go
Status: api.ConditionFull,
})
}
glog.V(2).Infof("NodeController: node %q status was %+v", node.Name, conditions)
Copy link
Member

Choose a reason for hiding this comment

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

This should be V(5) or higher. It's too spammy to leave enabled by default.

@bgrant0607
Copy link
Member

Cool. Happy to see this. Thanks.

@ddysher ddysher force-pushed the sync-node-status branch 3 times, most recently from 71d2ef3 to 9d62942 Compare January 28, 2015 02:38
@ddysher
Copy link
Contributor Author

ddysher commented Jan 28, 2015

As mentioned in one of the inline comment, I accidentally pushed an older working commit to the PR. However, much of the structure are the same.

Comment addressed. PTAL.

@bgrant0607
Copy link
Member

LGTM.
Heads up @mikedanese

@bgrant0607
Copy link
Member

Rerunning travis.

@bgrant0607
Copy link
Member

Waiting on #3695 to go in. Sorry @ddysher, but you'll need to rebase.

@bgrant0607
Copy link
Member

#3695 was merged.

@bgrant0607
Copy link
Member

#3818 was merged, also. Please rebase.

@ddysher ddysher mentioned this pull request Jan 28, 2015
Closed
@ddysher
Copy link
Contributor Author

ddysher commented Jan 28, 2015

@bgrant0607 will rebase after work

EDIT: get off work

@ddysher ddysher mentioned this pull request Jan 29, 2015
Merged
@ddysher
Copy link
Contributor Author

ddysher commented Jan 29, 2015

Rebased. PTAL. Also e2e passed with latest e2e fixes.

@ddysher ddysher mentioned this pull request Jan 29, 2015
Closed
@bgrant0607
Copy link
Member

Thanks. LGTM.

@bgrant0607
Copy link
Member

Waiting for another travis run before merging.

@bgrant0607
Copy link
Member

Sorry, @ddysher. Needs another rebase, apparently.

@ddysher
Copy link
Contributor Author

ddysher commented Jan 30, 2015

ok, resolved two conflicts
FYI:
First one is rest_test.go for minion registry, I've deleleted some tests related to health check, but PR#3856 touches those.
Second is auth_test.go, the url param change in PR#3707 has emerged the 409 error code (#2115). Which is the right error code since we allow minion update from thirdparty for not.

@bgrant0607
Copy link
Member

Thanks, @ddysher. Yes, 409 is the right error code for updates.

LGTM. Will first first thing in the morning.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2015
bgrant0607 added a commit that referenced this pull request Jan 30, 2015
@bgrant0607
Merge pull request #3733 from ddysher/sync-node-status
96e77d3 
Sync node status from node controller to master.
@bgrant0607 bgrant0607 merged commit 96e77d3 into kubernetes:master Jan 30, 2015
@jbeda jbeda mentioned this pull request Jan 30, 2015
Merged
jbeda added a commit to jbeda/kubernetes that referenced this pull request Jan 30, 2015
@jbeda
Convert kube-apiserver to hyperkube.
6bdede6 
Part of kubernetes#108.

Also:
* Added hyperkube cmd (not built by default yet).
* Added version support to hyperkube
* Remove health_check_minions flag from apiserver as it is no longer used with kubernetes#3733
@davidopp
Copy link
Member

davidopp commented Feb 3, 2015

Can we merge this now (after a rebase)?

@bgrant0607
Copy link
Member

This was merged days ago. Wrong PR?

@ddysher ddysher deleted the sync-node-status branch March 13, 2016 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ddysher @bgrant0607 @davidopp @brendandburns