Automatically create the kube-system namespace

[luxas]

At the same time we ensure that the default namespace is present, it also creates kube-system if it doesn't exist.

kube-system will now exist from the beginning, and will be recreated every 10s if deleted, in the same manner as the default ns

This makes UX much better, no need for kubectling a kube-system.yaml file anymore for a function that is essential to Kubernetes (addons). For instance, this makes dashboard deployment much easier when there's no need to check for the kube-system ns first.

A follow up in the future may remove places where logic to manually create the kube-system namespace is present.

Also fixed a small bug where CreateNamespaceIfNeeded ignored the ns parameter and was hardcoded to api.NamespaceDefault.

@davidopp @lavalamp @thockin @mikedanese @bryk @cheld @fgrzadkowski @smarterclayton @wojtek-t @dlorenc @vishh @dchen1107 @bgrant0607 @roberthbailey

---

This change is

[bryk]

Yes, I was waiting for this. LGTM.

[luxas]

I'm adding release-note-action-required as folks are manually creating the namespace in their bash/ansible/whatever scripts now, and should update/delete that when they upgrade to v1.3.

[cheld]

Thanks

[smarterclayton]

@liggitt

[liggitt]

can we move the list of namespaces to babysit to a field in Controller, and just iterate over the list here, rather than hard-coding two defaults?

[luxas]

I was thinking about something like this also:

c.SystemNamespacesInterval = 1 * time.Minute
c.SystemNamespaces = []string {api.NamespaceSystem}

// RunKubernetesNamespaces periodically makes sure that all internal namespaces exist
func (c *Controller) RunKubernetesNamespaces(ch chan struct{}) {
    wait.Until(func() {

        // Loop the system namespace list, and create them if they do not exist
        for _, ns := range c.SystemNamespaces {

            if err := c.CreateNamespaceIfNeeded(ns); err != nil {
                runtime.HandleError(fmt.Errorf("unable to create required kubernetes system namespace %s: %v", ns, err))
            }
        }
    }, c.SystemNamespacesInterval, ch)
}

Do you think we should go with that approach instead?
It's more dynamic when it's only a list to modify.
The default ns will be created at the service creation anyway, so there's no need to add it to the list

[bgrant0607]

@liggitt where is the similar code in Openshift, for comparison?

[liggitt]

we don't have controller loops for namespaces, they are just provisioned on startup.

[luxas]

Of course it's possible to only create the namespaces at c.Start, but I wouldn't prefer it as the user may delete it.

[liggitt]

I think the babysitting is fine

[luxas]

So you're okay with changing to #25196 (comment)? I think that's the best long-term solution

[lavalamp]

omit blank line

[luxas]

Thanks, I'm still learning these code formatting conventions :)

[lavalamp]

I'm OK with this change but I think it'd be pretty bad if someone deletes the system or default namespace--this will actually wait for the namespace deleter to finish up before recreating them!

Might consider actually rejecting deletes of kube-system in the namespace registry code.

[liggitt]

Might consider actually rejecting deletes of kube-system in the namespace registry code.

we have a place for that already:
https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/namespace/lifecycle/admission.go#L53-L56

[lavalamp]

ah, great!

On Thu, May 5, 2016 at 11:24 AM, Jordan Liggitt notifications@github.com
wrote:

Might consider actually rejecting deletes of kube-system in the namespace
registry code.

we have a place for that already:

https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/namespace/lifecycle/admission.go#L53-L56

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#25196 (comment)

[luxas]

OK, didn't know about the immortalNamespaces array.
I'm updating this to reject kube-system deletions.
Is it okay to keep the 1-minute babysitting or should we just create the ns at Controller.Start?
What do you think is the best approach when considering code maintainability?

[mikedanese]

LGTM

[mikedanese]

We should cleanup the addon updater in a follow up.

[luxas]

@k8s-bot unit test this please github issue: #IGNORE

[k8s-bot]

GCE e2e build/test passed for commit 8ea3a93.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[luxas]

@k8s-bot unit test this please github issue: #IGNORE

[luxas]

@k8s-bot unit test this please github issue: #IGNORE

[k8s-github-robot]

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

[k8s-bot]

GCE e2e build/test passed for commit 8ea3a93.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-github-robot]

Automatic merge from submit-queue

[erictune]

@mikedanese @luxas Does this PR require action by the user when upgrading from 1.2.x to 1.3.0? (Think about non-developer users.) If so, please edit your first comment to have a release-note block, like in #28132. If it is just an optional feature, please change the label to just release-note. If it is not a complete feature by itself, then apply "release-note-none" label instead.

[mikedanese]

I believe no. @luxas please confirm.

[luxas]

@erictune I don't think it'll cause any trouble/require manual action when upgrading specifically.

If the release-note-action-required labels are for PRs that break upgrade compability, this PR shouldn't have that label.

It's more "you should remove all kubectl create ns kube-system-calls in deployment code"-action-required. But I don't think deployment code that tries to create kube-system will break either.

[mikedanese]

@luxas you were the one who added release-note-action-required which is why we were asking 😃

[luxas]

Yes, I know 😊
Seems like I misunderstood the action-required thing, but I really think it should have release-note

[mikedanese]

SGTM