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

Remove hardcoded namespace - make metallb namespace-agnostic #193

Closed
simt2 opened this issue Mar 14, 2018 · 10 comments
Closed

Remove hardcoded namespace - make metallb namespace-agnostic #193

simt2 opened this issue Mar 14, 2018 · 10 comments
Assignees
Labels
Milestone

Comments

@simt2
Copy link

simt2 commented Mar 14, 2018

Is this a bug report or a feature request?:
I'd consider the hardcoded namespace a bug, making metallb independent from a configured namespace a feature.

What happened:
I deployed metallb in the default namespace.

What you expected to happen:
The speakers not failing to elect a leader with the recurring error message:

E0313 10:35:36.630874       1 leaderelection.go:228] error initially creating leader election record: namespaces "metallb-system" not found

How to reproduce it (as minimally and precisely as possible):
Deploy metallb in the "default" namespace, with --config-ns default set on both the controller and the speakers.

Anything else we need to know?:
I'm using the helm chart of the currenlty pending PR helm/charts/pull/3974 and I'm the the process of patching it.

Environment:

  • MetalLB version: 0.4.3
  • Kubernetes version: 1.9.2
  • BGP router type/version: none, using arp
  • OS (e.g. from /etc/os-release): CentOS 7
  • Kernel (e.g. uname -a): 3.10.0-693.2.2.el7.x86_64
@simt2
Copy link
Author

simt2 commented Mar 14, 2018

Is the reason the --config-ns option exists that metallb should in theory be able to run out of cluster?

@simt2
Copy link
Author

simt2 commented Mar 14, 2018

PR to charts repository addressing those changes: danderson/charts/pull/1

@danderson
Copy link
Contributor

Thanks! Yeah, as part of creating a Helm chart, I need to make MetalLB namespace agnostic. I did some things in that vein, but then the chart was resulting in a broken deployment, and I ran out of time (and lack-of-wrist-pain) to debug it. Thank you for doing the grunt work! I'll take a look at your PR shortly, and resume work on getting the chart upstreamed.

@simt2
Copy link
Author

simt2 commented Mar 15, 2018

Thank you very much @danderson! We're just starting to use metallb in our Lab, for now with ARP. The code in this PR currently serves as a quick-fix for us to integrate metallb better into our deployment chain. I'm happy to contribute, so please feel free to make suggestions.
Could you also elaborate later on the --config-ns parameter?

@danderson
Copy link
Contributor

The config-ns parameter exists mostly because I made a quick bandaid when I converted to helm charts. It didn't occur to me that "just read from my current namespace" is the correct behavior at all times :)

I'm not sure how to tell the go k8s client to "just read from my current NS", but we should just make it do that and get rid of the flag. I specifically want to reduce the number of valid metallb deployments if possible, to just the set of things we think are supportable. "Run in any namespace, but all the objects live in that namespace" is the right balance, I think.

@danderson
Copy link
Contributor

Oh, and in terms of contributing - you're most welcome to contribute at any level of involvement! The simplest is filing bugs for broken things, as you've done. If you'd like to work on some issues, that'd be great too - although please first chime in on the bugs so I can assign them to you, and possibly fill in some context I might have forgotten to add to the bug. There's a lot of "desired" stuff to be done, and I want to make sure we don't step on each other's toes.

@simt2
Copy link
Author

simt2 commented Mar 16, 2018

I agree on reducing the number of valid deployments. I understand not wanting to step on each others toes, but I needed a quick fix for myself. That's why I marked the PR as WIP at first.

Removed the config-ns option completely in my PR, updated the chart also.

@danderson danderson added the bug label Mar 17, 2018
@danderson danderson self-assigned this Mar 17, 2018
danderson added a commit that referenced this issue Mar 17, 2018
This further fixes the weirdnesses in the helm chart, by making
metallb read the namespace-local configuration file.
@danderson
Copy link
Contributor

So, I ended up making the code change mysefl, because I didn't want the manifest changes in the makefile or documentation, and I figured I could just roll the changes in quickly now and fix up everything at once.

The general idea is that if you use Helm, you get to pick your namespace because that's how Helm works. But if you're using the static manifest, that manifest is hardcoded to use metallb-system (to guarantee there's no name clashes and whatnot). If folks want to hand-edit the static manifest to use another namespace, that's great, and metallb should work, but I don't want to dump metallb into the default namespace by default.

Checking on the helm manifests now, since my PR got accepted to the stable chart repo in the meantime... There's probably some tweaking to do.

danderson added a commit that referenced this issue Mar 17, 2018
This further fixes the weirdnesses in the helm chart, by making
metallb read the namespace-local configuration file.

(cherry picked from commit c0036fe)
@danderson
Copy link
Contributor

Okay, did some quick testing with the chart, and with the small tweak of removing --config-ns, everything looks good. Cutting 0.4.6 with the code change, then I'll upstream the chart changes.

@danderson
Copy link
Contributor

helm/charts#4252 open to make 0.4.6 live.

@danderson danderson added this to the 0.4.6 milestone Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants