-
Notifications
You must be signed in to change notification settings - Fork 186
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
Implement Gateway status updates #502
Implement Gateway status updates #502
Conversation
🤖 Created branch: z_pr502/mangelajo/implement-status-updates |
main.go
Outdated
if err != nil { | ||
klog.Fatalf("Fatal error occurred creating engine: %v", err) | ||
} | ||
cableEngine, err := cableengine.NewEngine(localSubnets, localCluster, localEndpoint, submarinerClient, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We move this creation ahead in time, so ... the loop reconciling the gateway status will also
work for the PASSIVE gateway.
The driver initialization does not happen until "StartEngine" though.
8e72eb5
to
c2a26c0
Compare
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
🤖 the base of this PR has been updated to master |
c2a26c0
to
8fe88c4
Compare
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
8fe88c4
to
dde698a
Compare
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
40c5a05
to
d673770
Compare
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
1 similar comment
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
e0722fa
to
d2fd46d
Compare
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
ok, removed the workaround after #506 was merged |
E2E tests being handled here: #508 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small nits, otherwise it looks good. I even tried it :)
Thanks for working on this @mangelajo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I handled those comments (need to update the patch) Also, I will move this gateway checking together with the redundancy part which is almost the same.
test/e2e/framework/framework.go
Outdated
|
||
func (f *Framework) AwaitForGatewayGone(cluster framework.ClusterIndex, name string) { | ||
gwClient := SubmarinerClients[cluster].SubmarinerV1().Gateways(framework.TestContext.SubmarinerNamespace) | ||
framework.AwaitUntil("await gateway gone", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
The Host field was redudant with the Gateway name, also the localEndpoint information was missing when looking at the Gateway via describe for more context. Modified as discussed over slack channel.
The active gateway handles the role of cleaning up stale gateway entries, if a gateway has not reported for x3 the reporting interval the entry will be deleted. We handle this via an annotation since k8s API does not include modification timestamp in the object metadata.
In this way, when we use some of those also as labels it's less weird since the unwritten norm in k8s is to use lowcase values in the labels instead of upcase.
d00210a
to
c59ecfc
Compare
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
c59ecfc
to
4288cf7
Compare
🤖 Updated branch: z_pr502/mangelajo/implement-status-updates |
Unrelated globalnet failure, I re-ran that test. |
Thanks for the review @skitt |
Not much of a review, but it works for me, and my concerns have been raised and addressed already! |
return gw.(*submarinerv1.Gateway) | ||
} | ||
|
||
func (f *Framework) AwaitForGatewayRemoved(cluster framework.ClusterIndex, name string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (f *Framework) AwaitForGatewayRemoved(cluster framework.ClusterIndex, name string) { | |
func (f *Framework) AwaitGatewayRemoved(cluster framework.ClusterIndex, name string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
func testBasicGatewayReporting(f *subFramework.Framework) { | ||
clusterAName := framework.TestContext.ClusterIDs[framework.ClusterA] | ||
|
||
By(fmt.Sprintf("Ensuring that one gateway reports as active %q", clusterAName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By(fmt.Sprintf("Ensuring that one gateway reports as active %q", clusterAName)) | |
By(fmt.Sprintf("Ensuring that one gateway reports as active on %q", clusterAName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
By(fmt.Sprintf("Ensuring that one gateway reports as active %q", clusterAName)) | ||
activeGateways := f.GetGatewaysWithHAStatus(subv1.HAStatusActive) | ||
Expect(activeGateways).To(HaveLen(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be wrapped with framework.AwaitUntil
to avoid intermittent failures if the active was not yet running or written it's status. Perhaps AwaitActiveGateway
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there's already AwaitGatewayWithStatus
although it takes the gateway name which could be obtained by finding the active gateway node. But I realize we're actually duplicating what testEnginePodRestartScenario
does so why do we need this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For independence, I wanted an indication that the basic reporting is working if the dataplane is broken. When we have stats then we could identify if it's the dataplane breaking, or the reporting breaking. The reporting would always affect the dataplane tests though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do AwaitActiveGateway at least to avoid possible intermittent failures as you said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh AwaitGateway does not work, because we don't know the name, but we know that we want one. I will add another function that works with any.
activeGateways := f.GetGatewaysWithHAStatus(subv1.HAStatusActive) | ||
Expect(activeGateways).To(HaveLen(1)) | ||
|
||
gw := activeGateways[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gw := activeGateways[0] | |
activeGateway := activeGateways[0] |
For consistency ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
By(fmt.Sprintf("Ensuring that the gateway reports as active %q", clusterAName)) | ||
activeGateways := f.GetGatewaysWithHAStatus(subv1.HAStatusActive) | ||
Expect(activeGateways).To(HaveLen(1)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should verify that the found active Gateway
's name matches that of the gateway node found above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or better yet, call AwaitGatewayWithStatus
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I want to verify there's only "1". I can't do that with Await, I can call Await just before this, which would make sense, also would remove any glitches if it's starts as passive and then moves to active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did it in a slightly different way, let me know what you think
gw := activeGateways[0] | ||
By(fmt.Sprintf("Ensuring that the gateway %q is reporting connections", gw.Name)) | ||
Expect(gw.Status.Connections).NotTo(BeEmpty()) | ||
Expect(gw.Status.Connections[0].Status).To(Equal(subv1.Connected)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could cause an intermittent failure if active not yet reporting connections - it seems this should call AwaitGatewayFullyConnected
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
By(fmt.Sprintf("Deleting submariner engine pod %q", enginePod.Name)) | ||
f.DeletePod(framework.ClusterA, enginePod.Name, framework.TestContext.SubmarinerNamespace) | ||
|
||
newEnginePod := f.AwaitSubmarinerEnginePod(framework.ClusterA) | ||
By(fmt.Sprintf("Found new submariner engine pod %q", newEnginePod.Name)) | ||
|
||
By(fmt.Sprintf("Waiting for the gateway to be up and connected %q", newEnginePod.Name)) | ||
f.AwaitGatewayFullyConnected(framework.ClusterA, gw.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could report fully connected as the last state from the previous engine pod. Perhaps we should move this after the connectivity tests below - that way we know the gateway is re-connected and should be reporting the new state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that is true, I was actually wanting to make sure we had connections before starting the dataplane tests.
This will work better/faster when we I implement the cleanup of state at pod exit, I didn't dig into that, but I will do right after 0.3.0-rc1
#518
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I was thinking that too - delete on engine shutdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a DeleteGateway function for now, right after the pod deletion, which will ensure that at least the gateway status is re-created, and that it eventually moves to "fully connected"
When I do #518 then we remove this, and probably do a different check (I guess an informer to make sure it's deleted and re-created).
|
||
gwActive := f.AwaitGatewayWithStatus(framework.ClusterA, initialGatewayNode.Name, subv1.HAStatusActive) | ||
gwPassive := f.AwaitGatewayWithStatus(framework.ClusterA, initialNonGatewayNode.Name, subv1.HAStatusPassive) | ||
Expect(gwActive.Status.Connections).ToNot(BeEmpty(), "The active gateway must have active connections") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, it would be safest to call AwaitGatewayFullyConnected
for the active gateway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@tpantelis I opened this to handle comments as a follow-up #519 , and #518 if that's ok Currently lighthouse pushed a new ":latest" image, and this will fail when I submit any new changes. In such way we are able to release 0.3.0-rc1 and at the same time test the changes you're proposing here (changes that make sense and I agree with). |
Wait, may be there's no such chicken-egg, since we don't depend on lighthouse here.. ':D |
The technical difficulty with the lighthouse images does not exist, so it should be fine :) anyway we are talking on slack about merging this and do the follow up PR so we can cut RC1 now |
My comments aren't critical - let's merge this and address in the follow-ups @mangelajo created. |
🤖 Closed branches: [z_pr502/mangelajo/implement-status-updates] |
No description provided.