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 orphaned /tmp/drone directories in kubernetes #2694

Merged
merged 2 commits into from May 28, 2019

Conversation

chrisghill
Copy link

@chrisghill chrisghill commented May 3, 2019

@CLAassistant
Copy link

CLAassistant commented May 3, 2019

CLA assistant check
All committers have signed the CLA.

var volumes []v1.Volume
source := v1.HostPathDirectoryOrCreate
volume := v1.Volume{
Name: "local",
Copy link

Choose a reason for hiding this comment

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

is local volume the right storage type? we are using host volume mounts with HostPathDirectoryOrCreate in the runtime. For reference https://github.com/drone/drone-runtime/blob/master/engine/kube/util.go#L137

Copy link
Author

Choose a reason for hiding this comment

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

@bradrydzewski Is that not what I'm doing right below here in the next 5 lines? I just named it "local", but we can name it whatever we want. I tried to replicate the same functionality as the runtime. I specify the source as v1.HostpathDirectoryOrCreate on 116 and use that as the source on line 122.

Choose a reason for hiding this comment

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

sorry, I misread the code and thought it was creating a local mount instead of a hostPath mount. That part looks good. One question I have .... does local need to be a unique name? What will happen if two pipelines run on the same machine and try to create local volumes?

Copy link
Author

Choose a reason for hiding this comment

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

Good point on the potential name collision. I don't know if it would cause an issue or not, but I agree we should probably use a more unique or at least explicitly named volume. The name in the runtime code is obfuscated by the engine spec struct so I wasn't able copy that. I thought about maybe using the namespace name. Any other suggestions?

Copy link
Author

Choose a reason for hiding this comment

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

@bradrydzewski I've updated the volume name to be unique. Please take a look.

@bradrydzewski
Copy link

hey there, sorry for the delay on my end, but appreciate you pushing this through. thanks!

@bradrydzewski bradrydzewski merged commit 4ad252e into harness:master May 28, 2019
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

3 participants