-
Notifications
You must be signed in to change notification settings - Fork 39
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
K8s driver improvements #238
Conversation
7af2c1a
to
5e72b5a
Compare
Add the JOB_VOLUME_NAME setting, which is the name of the PVC to mount to /cnab/app/outputs so that outputs from the bundle can be persisted. JOB_VOLUME_PATH specifies where the volume is mounted locally. When the bundle defines outputs, but does not specify a PVC, the driver returns an error. Signed-off-by: Carolyn Van Slyck <[email protected]>
Allow the caller to specify additional labels to apply to resources created by the driver. Signed-off-by: Carolyn Van Slyck <[email protected]>
Signed-off-by: Carolyn Van Slyck <[email protected]>
When you mount a configmap or secret value as a file into a pod, it is read only, which doesn't fit the CNAB spec Signed-off-by: Carolyn Van Slyck <[email protected]>
The only value that ever makes sense for requiredCompletions is 1 because we aren't running parallel pods for the bundle job. Signed-off-by: Carolyn Van Slyck <[email protected]>
* When LimitCPU or LimitMemory is 0, do not set a limit. We don't set a request size, and requesting 0 is not the same as not having a requested resource quantity. This can lead to increased pod evictions. * Clarify that LimitCPU and LimitMemory are also used for the requested resources. * When ActiveDeadlineSeconds is 0, do not set a deadline. * Stop defaulting ActiveDeadlineSeconds to 5 minutes. It will cut off a bundle mid-execution, which isn't a great default. * Split reading the configuration in SetConfig and attempting to connect to the cluster, so that we can unit test the settings logic. * Move validation for KUBE_NAMESPACE into SetConfig so that any errors can be reported immediately. * Add comments for all of the driver settings Signed-off-by: Carolyn Van Slyck <[email protected]>
5e72b5a
to
3ba15dc
Compare
Update our fork of cnab-go to include the PR to cnab-go, fixing the kubernetes driver. cnabio/cnab-go#238 Signed-off-by: Carolyn Van Slyck <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
The only item we might double-check:
Remove requiredCompletions from k8s driver
I understand the reasoning supplied, however, are we sure other consumers of this lib weren't somehow relying on this piece of functionality?
requiredCompletions was not exported, so there wasn't any way to set it to anything other than 1. So we had this configurability in place but there was no way to use it (in addition to it not making any sense to change either). |
Signed-off-by: Carolyn Van Slyck <[email protected]>
* Support outputs in the kubernetes driver Add the JOB_VOLUME_NAME setting, which is the name of the PVC to mount to /cnab/app/outputs so that outputs from the bundle can be persisted. JOB_VOLUME_PATH specifies where the volume is mounted locally. When the bundle defines outputs, but does not specify a PVC, the driver returns an error. Signed-off-by: Carolyn Van Slyck <[email protected]> * Apply custom labels in the kubernetes driver Allow the caller to specify additional labels to apply to resources created by the driver. Signed-off-by: Carolyn Van Slyck <[email protected]> * Fix setting CLEANUP_JOBS Signed-off-by: Carolyn Van Slyck <[email protected]> * Use shared volume for input files too When you mount a configmap or secret value as a file into a pod, it is read only, which doesn't fit the CNAB spec Signed-off-by: Carolyn Van Slyck <[email protected]> * Remove requiredCompletions from k8s driver The only value that ever makes sense for requiredCompletions is 1 because we aren't running parallel pods for the bundle job. Signed-off-by: Carolyn Van Slyck <[email protected]> * Improve defaulting for kubernetes driver * When LimitCPU or LimitMemory is 0, do not set a limit. We don't set a request size, and requesting 0 is not the same as not having a requested resource quantity. This can lead to increased pod evictions. * Clarify that LimitCPU and LimitMemory are also used for the requested resources. * When ActiveDeadlineSeconds is 0, do not set a deadline. * Stop defaulting ActiveDeadlineSeconds to 5 minutes. It will cut off a bundle mid-execution, which isn't a great default. * Split reading the configuration in SetConfig and attempting to connect to the cluster, so that we can unit test the settings logic. * Move validation for KUBE_NAMESPACE into SetConfig so that any errors can be reported immediately. * Add comments for all of the driver settings Signed-off-by: Carolyn Van Slyck <[email protected]> * Review feedback Signed-off-by: Carolyn Van Slyck <[email protected]>
I am working on a Porter Operator that can run CNAB bundles using K8s CRDs. As part of that I had to make some changes to the Kubernetes driver to get real (not sample) bundles to work.
Allow the caller to specify additional labels to apply to resources created by the driver.
I had the if statement inverted... I've added a test to make sure it (and other settings) are correct.
The only value that ever makes sense for requiredCompletions is 1 because we aren't running parallel pods for the bundle job.
With these changes I am able to run @squillace 's kubeflow bundle! 🎉
Closes #73