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

Conversation

@liyinan926
Copy link
Member

@liyinan926 liyinan926 commented Nov 28, 2017

What changes were proposed in this pull request?

This PR allows setting user-specified environment variables and mounting user-specified secrets in the init-container. The driver init-container gets user-specified environment variables and secrets for the driver, whereas the executor init-container gets those for the executors. This is for #562 to enable the init-container to be able to download remote dependencies from Hadoop-compatible sources, e.g., HDFS, GCS, and S3.

How was this patch tested?

Unit tests.

@mccheah @foxish @kimoonkim

@foxish
Copy link
Member

foxish commented Nov 28, 2017

Should we also have secret injection the same way?

@liyinan926
Copy link
Member Author

Yes, we also need that. Do you prefer doing both in the same PR or doing secrets in a separate one?

@foxish
Copy link
Member

foxish commented Nov 28, 2017

The change LGTM

@foxish
Copy link
Member

foxish commented Nov 28, 2017

Question - in some sense, the init container is an implementation detail of the executor, can we assume that the executor pod's envs and secrets should just carry over to the init container? Might let us avoid introducing new options. WDYT?

@liyinan926
Copy link
Member Author

That's a good question. It depends on if there are use cases that would require the init-container to have secrets/envs different than the driver/executor containers.

@foxish
Copy link
Member

foxish commented Nov 28, 2017

cc/ @erikerlandson @mccheah @ash211
In terms of usage, do you see practical cases where we may want to differentiate between the init container and the executor?

@erikerlandson
Copy link
Member

I'm not aware of any use cases for special init-container env. For that matter, inheriting from executor settings doesn't even preclude additional settings if they ever did become necessary in the future.

@liyinan926
Copy link
Member Author

Sounds good. So we will use the relevant driver properties for the driver init-container and executor properties for the executor init-container.

@foxish
Copy link
Member

foxish commented Nov 30, 2017

Sounds good. So we will use the relevant driver properties for the driver init-container and executor properties for the executor init-container.

SGTM

@liyinan926
Copy link
Member Author

Updated per our discussion.

@liyinan926 liyinan926 changed the title Allow setting user-specified environments in the init-container Allow user-specified environment variables and secrets in the init-container Dec 1, 2017
@foxish
Copy link
Member

foxish commented Dec 1, 2017

@erikerlandson @ifilonenko @mccheah can you review this please?

@liyinan926
Copy link
Member Author

PR updated to also mount user-specified secrets. PTAL. Thanks!

Copy link

Choose a reason for hiding this comment

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

Are these imports supposed to be wildcarded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the rule. The IDE automatically did the change. Reverted.

@liyinan926
Copy link
Member Author

Any other comments? Will merge by EOD Monday if no objection.

@mccheah mccheah merged commit 0612195 into apache-spark-on-k8s:branch-2.2-kubernetes Dec 4, 2017
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.

4 participants