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 Ingress awaiter for ExternalName Services #320

Merged
merged 2 commits into from
Jan 4, 2019

Conversation

lblackstone
Copy link
Member

The await logic was incorrectly waiting for active Pods for all Service
types. However, Services with type: ExternalName do not have
associated Pods and should succeed as long as the Service exists.

Fixes #317

The await logic was incorrectly waiting for active Pods for all Service
types. However, Services with type: ExternalName do not have
associated Pods and should succeed as long as the Service exists.
@@ -50,6 +51,7 @@ type ingressInitAwaiter struct {
endpointsReady bool
endpointsSettled bool
endpointExists map[string]bool
Copy link
Contributor

Choose a reason for hiding this comment

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

A better name for this is now probably something like serviceReady, as Services with type ExternalName don't have endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Arg, dammit. I'm testing out the GH extension and it apparently doesn't let you batch up review comments?

@@ -195,6 +234,7 @@ func (iia *ingressInitAwaiter) await(
}
case <-timeout:
// On timeout, check one last time if the ingress is ready.
iia.endpointsReady = iia.checkIfEndpointsReady()
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we have to do this also for case <-iia.config.ctx.Done():?

return
}

name := service.GetName()
Copy link
Contributor

Choose a reason for hiding this comment

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

if event.Type == watch.Deleted {, don't we want to delete this from iia.externalServices? And now that I think about it, is there a reason we don't do this for iia.endpointExists as well?

@@ -158,6 +198,7 @@ func (iia *ingressInitAwaiter) read(
glog.V(3).Infof("Error iterating over endpoint list for ingress %q: %v", ingress.GetName(), err)
}

iia.endpointsReady = iia.checkIfEndpointsReady()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm missing something -- seems like it would be better to just get rid of iia.endpointsReady and simply call iia.checkIfEndpointsReady() wherever you would check iia.endpointsReady? Seems redundant to have the function and the variable.

@hausdorff hausdorff force-pushed the lblackstone/external-ingress-fix branch from cc911f2 to 220c000 Compare January 4, 2019 05:42
@hausdorff hausdorff force-pushed the lblackstone/external-ingress-fix branch from 220c000 to e32c939 Compare January 4, 2019 05:48
@hausdorff
Copy link
Contributor

Because @lblackstone is not currently here, I've gone ahead and "addressed" my comments. To my eye, this is semantically equivalent, but with the (small) changes noted in the following list. I'd like to turn this sort of thing around pretty quick, so I'll file a follow-up issue to have @lblackstone do a post-commit review to make sure I didn't do anything silly.

  • Renamed iia.endpointExists -> iia.knownEndpointObjects and iia.externalServices -> iia.knownExternalNameServices, and made both of them sets.String.
    • Rationale: Fundamentally the awaiter now checks readiness by determining if a v1/Endpoint object exists, but only if the v1/Service is not of type ExternalName. If we maintain a set of Endpoints and Service objects that have type ExternalName, it seems to make it quite simple to check whether this condition is true. We rename because we want the names to reflect this usage.
  • Removed iia.endpointsReady, replaced all instances where we check this with iia.checkIfEndpointsReady().
    • Rationale: Unlike iia.ingressReady, this field is now quite complex, and we find ourselves having to update it before every check to ensure correctness. iia.ingressReady, on the other hand, is set only in one place, and is fairly simple. So, we think it makes sense to just call iia.checkIfEndpointsReady() directly.
  • When we receive a v1/Event indicating a deletion of a Service or Endpoint, we now delete them from iia.knownExternalNameServices and iia.knownEndpointObjects, respectively. Before, we'd keep them in a map, and set their values to false.
  • We un-delete on of the test cases, and simply add the new test case alongside it. (I believe the deletion was originally a mistake.)

@hausdorff hausdorff merged commit aba4a95 into master Jan 4, 2019
@pulumi-bot pulumi-bot deleted the lblackstone/external-ingress-fix branch January 4, 2019 06:16
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.

None yet

2 participants