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

Fix layer2 not arping when IP changes #520

Merged
merged 1 commit into from Mar 17, 2020
Merged

Conversation

KnicKnic
Copy link
Contributor

@KnicKnic KnicKnic commented Jan 27, 2020

Fixed #471

I noticed that SetBalancer in Announcer.go essentially short circuits if the load balancer is already set. I also saw that it was cached in main.go of the speaker for the service.

So to fix this, I just deleted the existing balancer for the service and allowed it to fall through and create a new one with the new IP.

@KnicKnic
Copy link
Contributor Author

@KnicKnic KnicKnic commented Feb 28, 2020

@danderson are you the appropriate person to request a review from?

@champtar
Copy link
Contributor

@champtar champtar commented Feb 28, 2020

@daxmc99 ?

@daxmc99
Copy link
Contributor

@daxmc99 daxmc99 commented Mar 2, 2020

Thanks for the contribution @KnicKnic, I will try to review this later in the week.

@KnicKnic
Copy link
Contributor Author

@KnicKnic KnicKnic commented Mar 12, 2020

@daxmc99 ping for an updated eta :-)

@daxmc99
Copy link
Contributor

@daxmc99 daxmc99 commented Mar 12, 2020

Hi @KnicKnic,
This looks like a good change. I will try and give it a better lookover tonight. In the meantime, could you squash the merge commit and rebase your changes onto master?
Also, while I know this a small change, could you include some of the description you have in your PR in your commit message?

SetBalancer in Announcer.go essentially short circuits if the load balancer is already set.
The fix just deletes the existing balancer for the service and allows it to fall through and create a new one with the new IP.
@KnicKnic
Copy link
Contributor Author

@KnicKnic KnicKnic commented Mar 12, 2020

Hi @KnicKnic,
This looks like a good change. I will try and give it a better lookover tonight. In the meantime, could you squash the merge commit and rebase your changes onto master?
Also, while I know this a small change, could you include some of the description you have in your PR in your commit message?

@daxmc99
Do you mean to rebase on main or master. I rebased of main and force pushed (as master conflicts with main). Btw main/master is confusing :-) .

@daxmc99
Copy link
Contributor

@daxmc99 daxmc99 commented Mar 12, 2020

I am seeing the rebase and merge option available again in the GitHub UI so what you did worked 👍

@danderson
Copy link
Contributor

@danderson danderson commented Mar 17, 2020

Thanks @KnicKnic for being patient with the slow turnaround, and thanks @daxmc99 for reviewing!

@danderson danderson merged commit 5a9fa3b into metallb:main Mar 17, 2020
r4j4h added a commit to r4j4h/minikube that referenced this issue May 14, 2021
This change isn't strictly necessary but the newer versions of metallb contain some really nice quality of life improvements, and better support newer (installed by default) versions of kubernetes better.

A sample of some improvements this brings to metallb add-on installed by minikube:
- [Layer2 doesn't update when when ip changes](metallb/metallb#520) - this hit me, and might be hitting others
- [Allow spaces in configs](metallb/metallb#500) - quality of life
- [selflink is deprecated](metallb/metallb#812) - Kubernetes deprecation (I believe seeing this is in the logs is what originally caused me to look into upgrading it)
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.

4 participants