Reprocess services when an ip is available because of a service change #1645
Conversation
fedepaol
requested review from
johananl,
rata,
russellb,
gclawes and
oribon
as code owners
fedepaol
force-pushed
the
fix/changeservicetypefreesip
branch
from
04e842a
to
c3ff4a6
Compare
.github/workflows/ci.yaml
Outdated
| @@ -303,6 +303,7 @@ jobs: | |||
| sed -i 's/quay.io\/metallb\/controller:main/quay.io\/metallb\/controller:dev-amd64/g' bin/metallb-operator.yaml | |||
| sed -i 's/native/frr/g' bin/metallb-operator.yaml | |||
| kubectl apply -f bin/metallb-operator.yaml | |||
| sleep 30s | |||
There was a problem hiding this comment.
can this use some kubectl wait instead?
There was a problem hiding this comment.
Too late, I already yolo-ed this in another PR because it was blocking all the PRs :P
There was a problem hiding this comment.
seems that it has already bitten us? 😅
https://github.com/metallb/metallb/actions/runs/3272314767/jobs/5383305208
or this is unrelated?
There was a problem hiding this comment.
Nope I think it's the same. Let me see if I can do the same trick we did in #1648 to wait until the webhook is ready before applying the metallb resource.
controller/main.go
Outdated
| if !c.convergeBalancer(l, name, svc) { | ||
| return controllers.SyncStateError | ||
| } | ||
| if wasAllocated && c.ips.Pool(name) == "" { |
There was a problem hiding this comment.
mind adding a comment what's wasAllocated and that it is possibly being changed by c.ConvergeBalancer?
otherwise this if looks a bit confusing
There was a problem hiding this comment.
Yeah, I understand that. I spent a good amount of time and I can assure the previous versions were even worse. Let me try to find something better, if not I'll restort to commenting
e2etest/l2tests/assignement.go
Outdated
| ginkgo.AfterEach(func() { | ||
| if ginkgo.CurrentGinkgoTestDescription().Failed { | ||
| k8s.DumpInfo(Reporter, ginkgo.CurrentGinkgoTestDescription().TestText) | ||
| } | ||
|
|
||
| // Clean previous configuration. | ||
| err := ConfigUpdater.Clean() | ||
| framework.ExpectNoError(err) | ||
| }) | ||
|
|
||
| f = framework.NewDefaultFramework("assignement") | ||
| f.NamespacePodSecurityEnforceLevel = admissionapi.LevelPrivileged | ||
|
|
||
| ginkgo.BeforeEach(func() { | ||
| cs = f.ClientSet | ||
|
|
||
| ginkgo.By("Clearing any previous configuration") | ||
| err := ConfigUpdater.Clean() | ||
| framework.ExpectNoError(err) | ||
| }) |
There was a problem hiding this comment.
unrelated note (for another pr): we should probably have a function that wraps these instead of copying in each e2e file
There was a problem hiding this comment.
Not sure about that. The size is small enough that we could save 2/3 lines at most
e2etest/l2tests/assignement.go
Outdated
| admissionapi "k8s.io/pod-security-admission/api" | ||
| ) | ||
|
|
||
| var _ = ginkgo.Describe("IP Assignement", func() { |
e2etest/l2tests/assignement.go
Outdated
| gomega.Eventually(func() int { | ||
| s, err := cs.CoreV1().Services(svc1.Namespace).Get(context.Background(), svc1.Name, metav1.GetOptions{}) | ||
| framework.ExpectNoError(err) | ||
| return len(s.Status.LoadBalancer.Ingress) |
There was a problem hiding this comment.
wdyt about checking that the freed ip was assigned instead of checking the length? feels a bit more natural imo
e2etest/l2tests/assignement.go
Outdated
| return len(s.Status.LoadBalancer.Ingress) | ||
| }, 5*time.Second, 1*time.Second).Should(gomega.BeZero()) | ||
|
|
||
| ginkgo.By("Changing the service type so the ip is free to be used again") |
There was a problem hiding this comment.
wdyt about testing that the ip is freed when the service is deleted as well if we're already addressing this scenario?
There was a problem hiding this comment.
Iirc the case was already handled and is not affected by the fix in this PR. In any case I have already all the infrastructure in place, so it won't hurt adding a new test.
fedepaol
force-pushed
the
fix/changeservicetypefreesip
branch
2 times, most recently
from
125eebc
to
d923996
Compare
|
@oribon I think I applied the requested changes |
If we stop associating the current service with a pool, we need to reprocess all the services because the newly available ip could be used by another service. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
fedepaol
force-pushed
the
fix/changeservicetypefreesip
branch
from
d923996
to
846cdf1
Compare
Adding a test that verifies that changing the service type makes an ip available and reprocess eventual existing services with externalIP=pending. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
fedepaol
force-pushed
the
fix/changeservicetypefreesip
branch
from
846cdf1
to
de084d6
Compare
When a service changes (i.e. changing the servicetype) and the IP is revoked, we reprocess the services so that the newly available ip is assigned.
Fixes #1586