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

Automatically create the kube-system namespace #25196

Merged
merged 1 commit into from
May 12, 2016

Conversation

luxas
Copy link
Member

@luxas luxas commented May 5, 2016

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 Reviewable

@bryk
Copy link
Contributor

bryk commented May 5, 2016

Yes, I was waiting for this. LGTM.

@luxas luxas added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-label-needed labels May 5, 2016
@luxas luxas added this to the v1.3 milestone May 5, 2016
@luxas
Copy link
Member Author

luxas commented May 5, 2016

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
Copy link
Contributor

cheld commented May 5, 2016

Thanks

@smarterclayton
Copy link
Contributor

@liggitt

// EnsureSystemNamespacesExist will make sure both the default and the system namespace always will exist
func (c *Controller) EnsureSystemNamespacesExist() error {
// Try to create the default namespace
if err := c.CreateNamespaceIfNeeded(api.NamespaceDefault); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@liggitt
Copy link
Member

liggitt commented May 5, 2016

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

@luxas
Copy link
Member Author

luxas commented May 5, 2016

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
Copy link
Member

liggitt commented May 5, 2016

I think the babysitting is fine

@luxas
Copy link
Member Author

luxas commented May 5, 2016

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

@luxas luxas force-pushed the auto_create_kube_system branch from 614b8fe to c2454f3 Compare May 5, 2016 16:07
// RunKubernetesNamespaces periodically makes sure that all internal namespaces exist
func (c *Controller) RunKubernetesNamespaces(ch chan struct{}) {
wait.Until(func() {

Copy link
Member

Choose a reason for hiding this comment

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

omit blank line

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@lavalamp
Copy link
Member

lavalamp commented May 5, 2016

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
Copy link
Member

liggitt commented May 5, 2016

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
Copy link
Member

lavalamp commented May 5, 2016

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
Copy link
Member Author

luxas commented May 5, 2016

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 mikedanese assigned mikedanese and unassigned davidopp May 9, 2016
@mikedanese
Copy link
Member

LGTM

@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2016
@mikedanese
Copy link
Member

We should cleanup the addon updater in a follow up.

@luxas luxas added the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 9, 2016
@luxas
Copy link
Member Author

luxas commented May 10, 2016

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

@k8s-bot
Copy link

k8s-bot commented May 11, 2016

GCE e2e build/test passed for commit 8ea3a93.

@luxas
Copy link
Member Author

luxas commented May 11, 2016

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

1 similar comment
@luxas
Copy link
Member Author

luxas commented May 11, 2016

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

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented May 12, 2016

GCE e2e build/test passed for commit 8ea3a93.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@erictune
Copy link
Member

erictune commented Jul 2, 2016

@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
Copy link
Member

I believe no. @luxas please confirm.

@luxas
Copy link
Member Author

luxas commented Jul 2, 2016

@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
Copy link
Member

mikedanese commented Jul 2, 2016

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

@mikedanese mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jul 2, 2016
@luxas
Copy link
Member Author

luxas commented Jul 2, 2016

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

@mikedanese mikedanese added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 2, 2016
@mikedanese
Copy link
Member

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.