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

Use Job instead of helm plugin to generate admission secret #191

Merged
merged 1 commit into from
May 29, 2019

Conversation

TommyLike
Copy link
Contributor

See above
For #184

@@ -98,3 +108,24 @@ spec:
selector:
app: volcano-admission
sessionAffinity: None

---
apiVersion: batch/v1
Copy link
Member

Choose a reason for hiding this comment

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

how about init container for admission controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is we need to run the init-container every time when new admission pod is created, but job would only execute once.

Copy link
Member

Choose a reason for hiding this comment

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

That should be ok, as the script will remote the old one. If using job, how to make sure secret is generated before admission pod running?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pod will wait until the secret is ready because it will mount the secret. when used in init-container it will delete&recreate the secret every time when pod recreated.

Copy link
Member

Choose a reason for hiding this comment

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

that's ok to me. BTW, Do we delete secret when uninstall volcano?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we need investigate all of the uncleaned resource and remove them when uninstalling.

@@ -15,5 +15,16 @@

FROM alpine:latest

# Install requirements
Copy link
Member

Choose a reason for hiding this comment

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

the file name should be Dockerfil.secret, and only include gen-admission-secret.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sh file and binary are kept in one container in default since they are both simple logic and work for admission service, adding one more image can be complicated for maintain and the usage of end user.

Copy link
Member

Choose a reason for hiding this comment

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

adding one more image can be complicated for maintain and the usage of end user

If so, why Dockerfile.only_binay ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will remove this.

@@ -0,0 +1,19 @@
# Copyright 2019 The Volcano Authors.
Copy link
Member

Choose a reason for hiding this comment

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

The file name should be Dockerfile.

@k82cn
Copy link
Member

k82cn commented May 29, 2019

/lgtm
/approve

@volcano-sh-bot volcano-sh-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels May 29, 2019
@volcano-sh-bot volcano-sh-bot merged commit 74ca0d4 into volcano-sh:master May 29, 2019
kevin-wangzefeng pushed a commit to kevin-wangzefeng/volcano that referenced this pull request Jun 28, 2019
Use Job instead of helm plugin to generate admission secret
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants