Skip to content
This repository was archived by the owner on Jan 9, 2020. It is now read-only.

Conversation

@f355
Copy link

@f355 f355 commented Jan 4, 2018

What changes were proposed in this pull request?

The default cluster.local zone is not something set in stone, so if kube-dns is configured to use a different zone, Spark needs to know about it in order to allow the executors reach the driver.

How was this patch tested?

The patch includes changes to two unit tests, both are passing. It is hard to integration-test this patch because that would require a custom kubernetes setup, however, current integration tests pass while using the default value for the added option.

The default `cluster.local` zone is not something set in stone,
so if `kube-dns` is configured to use a different zone, Spark needs to
know about it in order to allow the executors reach the driver.
@f355
Copy link
Author

f355 commented Jan 4, 2018

I have no idea why the integration tests have crapped out. I don't think it's related to my change, but if it is, I'd be grateful to get some pointers to what's happening.

@ifilonenko
Copy link
Member

Those integration test failures aren’t related to your additions. That failure is sporadically seen across pull requests.

@foxish
Copy link
Member

foxish commented Jan 8, 2018

@f355 - have you considered just leaving the dns zone entirely out of the equation? If the search path is setup correctly, we shouldn't need it at all.

@f355
Copy link
Author

f355 commented Jan 9, 2018

@foxish you mean <driver-svc>.<namespace>.svc, omitting the domain? Good idea, I didn't think about it. It looks like it would work in the majority of the cases. I'm not an expert on kubernetes though, so I'm not sure if there are cases when this would break.

I think if we go with your suggestion by default, and allow to specify a domain for the weird cases where it's needed, we should be good.

But before I jump to implementing it, I'd like to hear from the maintainers.

@foxish
Copy link
Member

foxish commented Jan 9, 2018

Just fixed in upstream in apache#20187 and confirmed with SIG network that it was an acceptable way to proceed. Thanks for sending this PR btw, we wouldn't have discovered this latent issue if it hadn't been brought up here. Cheers!

@f355
Copy link
Author

f355 commented Jan 9, 2018

Awesome, thank you!

@f355 f355 closed this Jan 9, 2018
@f355 f355 deleted the configurable-dns-zone branch January 23, 2018 14:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants