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: request disk_mb resource from k8s #1858

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

kdm9
Copy link
Contributor

@kdm9 kdm9 commented Sep 13, 2022

Description

k8s supports adding ephemeral storage (e.g. node-local SSDs) to the working directory of the container. However, the size of this working directory isn't configured. This PR maps the disk_mb resource to the ephemeral-storage k8s Allocatable.

As an aside, while reviewing this PR, I think I have found a small issue: during the container init code, we are setting the container.volume.mounts property twice with single-entry lists, rather than once with a list of two mounts as their API seems to want. Is my reading correct, and this is a bug? If so, I'll quickly fix it.

container.volume_mounts = [
kubernetes.client.V1VolumeMount(name="workdir", mount_path="/workdir")
]
container.volume_mounts = [
kubernetes.client.V1VolumeMount(name="source", mount_path="/source")
]

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). My (@kdm9) reading of the docs implied this already happened, but I can add a more explicit statement if need be

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kdm9
Copy link
Contributor Author

kdm9 commented Sep 13, 2022

@johanneskoester is there a way to trigger the k8s tests on github? They seem to be skipped (I assume as I'm not a maintainer and the PR isn't approved yet?)

@johanneskoester
Copy link
Contributor

Indeed, they only work for people with access to the repo, because of required tokens (they run on GCE). I always wanted to use something like minikube directly inside of github actions, but I did not yet find the time to implement that (maybe there is even a prebuilt action for that). However, I think that the changes are minimal enough to risk just merging them. If main branch fails afterwards, I will just contact you via this PR. For the future, I think you can also enable in your github settings that the upstream repo is allowed to commit into your PR. This way I could trigger a test run with my permissions.

Regarding the volume mounts, I think you are right! A PR would be very welcome!

@johanneskoester johanneskoester merged commit f68f166 into snakemake:main Sep 20, 2022
kdm9 added a commit to gekkonid/snakemake that referenced this pull request Sep 20, 2022
@kdm9
Copy link
Contributor Author

kdm9 commented Sep 20, 2022

Looks like I did break the tests, but not directly: the gcloud commands relating to local ssds seem to have changed between the version on github actions and what I'm using. I'll make a PR tomorrow.

@johanneskoester
Copy link
Contributor

OK, thanks.

johanneskoester pushed a commit that referenced this pull request Sep 23, 2022
### Description

Fixes a bug in tests introduced in #1858 

### QC

* [x] The PR contains a test case for the changes or the changes are
already covered by an existing test case.
* [x] The documentation (`docs/`) is updated to reflect the changes or
this is not necessary (e.g. if the change does neither modify the
language nor the behavior or functionalities of Snakemake).
johanneskoester pushed a commit that referenced this pull request Sep 23, 2022
As described in the body of PR
#1858, there's a typo whereby
the property `container.volume_mounts` is overwritten, rather than
appended to.

### QC

* [x] The PR contains a test case for the changes or the changes are
already covered by an existing test case. (this is allegedly tested as
part of the k8s tests).
* [ ] ~The documentation (`docs/`) is updated to reflect the changes or
this is not necessary (e.g. if the change does neither modify the
language nor the behavior or functionalities of Snakemake).~
johanneskoester pushed a commit that referenced this pull request Sep 26, 2022
🤖 I have created a release *beep* *boop*
---


##
[7.14.1](v7.14.0...v7.14.1)
(2022-09-23)


### Bug Fixes

* allocation of local ssds in k8s tests
([#1870](#1870))
([d0de4dc](d0de4dc))
* allow script directive to take pathlib Path
([#1869](#1869))
([12cdc96](12cdc96))
* catch errors in remote.AUTO provider list
([#1834](#1834))
([c613ed2](c613ed2))
* consistently use text output in conda shell commands and various
little fixes for failing test cases due to conda package changes
([#1864](#1864))
([4234fe7](4234fe7))
* declare associative arrays
([#1844](#1844))
([90ae449](90ae449))
* fix falsely triggered reruns if input files are obtained via
workflow.source_path()
([#1862](#1862))
([2dc2e6a](2dc2e6a))
* fixed typos
([#1847](#1847))
([a1e49b6](a1e49b6))
* k8s container volume mounts as list
([#1868](#1868))
([5c54df3](5c54df3))
* None type error when invoking Workflow object manually
([#1731](#1731))
([dc45ccb](dc45ccb))
* request disk_mb resource from k8s
([#1858](#1858))
([f68f166](f68f166))
* respect shebang lines in post-deploy scripts (see deployment docs)
([#1841](#1841))
([c26c4b6](c26c4b6))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

2 participants