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

Create a standalone k8s binary, capable of running a full cluster #2121

Merged
merged 1 commit into from
Nov 11, 2014

Conversation

brendandburns
Copy link
Contributor

See #108

@brendandburns brendandburns force-pushed the standalone branch 3 times, most recently from eff91f1 to d953a94 Compare November 2, 2014 05:56
@@ -358,3 +364,42 @@ func (h *EtcdHelper) AtomicUpdate(key string, ptrToType runtime.Object, tryUpdat
return err
}
}

func checkEtcd(host string) error {
response, err := http.Get(host + "/version")
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC version can race on startup because it loads earlier than other attributes. Can't find the issue now, but we switched to reading the root key in a few places because of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - /version returns 200s before things are up and really running. See: #963 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's ok. Nothing crashes if etcd isn't fully there, and if there
is a /version there, it will get there eventually...

I can switch to hitting the root key, if we really want to, but unlike the
integration tests, I don't immediately start hammering the server and
expecting correct responses.

--brendan

On Sun, Nov 2, 2014 at 10:11 AM, Joe Beda notifications@github.com wrote:

In pkg/tools/etcd_tools.go:

@@ -358,3 +364,42 @@ func (h *EtcdHelper) AtomicUpdate(key string, ptrToType runtime.Object, tryUpdat
return err
}
}
+
+func checkEtcd(host string) error {

  • response, err := http.Get(host + "/version")

Yeah - /version returns 200s before things are up and really running.
See: #963 (comment)
#963 (comment)


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/2121/files#r19713556
.

@derekwaynecarr
Copy link
Member

Is there a plan to add an option in the future to just start as go-routines what would typically go in the kubernetes-master, and an option to just start what would go on the kubernetes-minions using this single top-level binary instead of the separate daemons? I like this as it more aligns with what we started doing in OpenShift Origin v3, and makes the CLI setup much simpler.

@brendandburns
Copy link
Contributor Author

Yes, I'd like to achieve this. So there's a single binary for everything,
and you pass in --kubelet or --master or whatever.

There's more work to do to achieve this, mostly around networking, but yes,
that's my goal.

--brendan

On Sun, Nov 2, 2014 at 7:03 PM, Derek Carr notifications@github.com wrote:

Is there a plan to add an option in the future to just start as
go-routines what would typically go in the kubernetes-master, and an option
to just start what would go on the kubernetes-minions using this single
top-level binary instead of the separate daemons? I like this as it more
aligns with what we started doing in OpenShift Origin v3, and makes the CLI
setup much simpler.


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

@smarterclayton
Copy link
Contributor

Structurally, we went with a config -> Run pattern for each of the routines. https://github.com/openshift/origin/blob/master/pkg/cmd/server/kubernetes/master.go has some examples of how we launch the master, although some of this we have to change as the authorizer changes @erictune is working on.

@erictune
Copy link
Member

erictune commented Nov 3, 2014

Would you either change this PR or file an issue so that cmd/apiserver calls RunApiServer
and likewise for kubelet, scheduler, and controller?
Otherwise, if I want to write a test that tests whether an apiserver is constructed with the right config, I will have to test two binaries instead of one. And keeping them in sync is a pain.

@brendandburns
Copy link
Contributor Author

Agree that's the long term goal. I'll file an issue.

On Mon, Nov 3, 2014 at 8:48 AM, Eric Tune notifications@github.com wrote:

Would you either change this PR or file an issue so that cmd/apiserver
calls RunApiServer
and likewise for kubelet, scheduler, and controller?
Otherwise, if I want to write a test that tests whether an apiserver is
constructed with the right config, I will have to test two binaries instead
of one. And keeping them in sync is a pain.


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

@erictune
Copy link
Member

erictune commented Nov 3, 2014

So, is your long term goal to get rid of the standalone binaries?

@dchen1107
Copy link
Member

What is the motivation behind this? For testing? or other stuff? I couldn't figure out by reading the PR.

@dchen1107
Copy link
Member

ok, @bgrant0607 just pointed #108 to me.

@brendandburns
Copy link
Contributor Author

The goal of this is to both reduce the size of our download (since each
individual binary contains duplicate code) and simplify our install
experience (since you only need to install one binary instead of N)

Also to make local development/experimentation easier, since you only need
to run a single binary locally to have a cluster to play around with.

--brendan

On Mon, Nov 3, 2014 at 11:49 AM, Dawn Chen notifications@github.com wrote:

ok, @bgrant0607 https://github.com/bgrant0607 just pointed #108
#108 to me.


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

@dchen1107
Copy link
Member

Ok, with this PR, are all cmd/$binary should be removed by this single binary?

@brendandburns
Copy link
Contributor Author

Eventually I will remove binaries, but for now, I want to just check in the standalone version.

Rebased, please take another look.

Thanks!
--brendan

@smarterclayton
Copy link
Contributor

When you move on to the next step I want to sync about standardizing a run in a Docker container config for the Kubelet - I got the first steps working in OpenShift re: running in a container (https://lists.openshift.redhat.com/openshift-archives/dev/2014-November/msg00042.html) but I want to figure out what security implications and patterns are going to work best for that and make sure we match.

On Nov 10, 2014, at 2:28 PM, Brendan Burns notifications@github.com wrote:

Eventually I will remove binaries, but for now, I want to just check in the standalone version.

Rebased, please take another look.

Thanks!
--brendan


Reply to this email directly or view it on GitHub.

@brendandburns
Copy link
Contributor Author

Travis break was a flake. I can have mergez?

@dchen1107
Copy link
Member

LGTM

dchen1107 added a commit that referenced this pull request Nov 11, 2014
Create a standalone k8s binary, capable of running a full cluster
@dchen1107 dchen1107 merged commit 30fcf24 into kubernetes:master Nov 11, 2014
mux := http.NewServeMux()
apiserver.NewAPIGroup(m.API_v1beta1()).InstallREST(mux, "/api/v1beta1")
apiserver.NewAPIGroup(m.API_v1beta2()).InstallREST(mux, "/api/v1beta2")
apiserver.InstallSupport(mux)
Copy link
Member

Choose a reason for hiding this comment

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

This apiserver setup code looks wrong. https://github.com/GoogleCloudPlatform/kubernetes/blob/master/cmd/apiserver/apiserver.go is quite different.

Should extract code into a library-- perhaps in pkg/master/cmd/ or something.

Not much point to an all-in-one binary if it doesn't behave the same as our current binaries.

@jbeda
Copy link
Contributor

jbeda commented Nov 13, 2014

@lavalamp I'm going to be picking this stuff up. I'll continue to refactor this and will make sure you are the reviews. Thanks.

@bgrant0607
Copy link
Member

@jbeda Please hold off on this particular part. I'm merging #2282 with these changes at this moment.

@bgrant0607
Copy link
Member

Yeah, I have to fix this. standalone.go is doing things that now require access to private members of the master.

@lavalamp
Copy link
Member

I think this was forked off of cmd/apiserver.go before @erictune's refactoring. Otherwise I'm not sure how it got like this.

@smarterclayton
Copy link
Contributor

Anything standalone go needs Openshift will need :)

On Nov 12, 2014, at 8:16 PM, bgrant0607 notifications@github.com wrote:

Yeah, I have to fix this. standalone.go is doing things that now require access to private members of the master.


Reply to this email directly or view it on GitHub.

@bgrant0607
Copy link
Member

I think I can just delete the mux and calls to NewAPIGroup. All of that is already done by master.New.

@dchen1107
Copy link
Member

Ahh, just realized you guys were talking about this PR which I merged two days ago. Yes, this was merged before @erictune's refactoring and auth stuff in. Now I believe this shouldn't work with the rest of system with newly PRs for auth. My bad. I should insist on cleaning / consolidate two copies of apiserver, or at least add a TODO in cmd/apiserver/apiserver.go.

@bgrant0607
Copy link
Member

Pleases nobody touch this now

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.

8 participants