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

K8s driver improvements #238

Merged
merged 7 commits into from
Feb 26, 2021
Merged

Conversation

carolynvs
Copy link
Contributor

@carolynvs carolynvs commented Feb 1, 2021

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.

  • Mount a persistent volume to share data: injected files and outputs, between the driver and the bundle job.
  • Apply custom labels in the kubernetes driver
    Allow the caller to specify additional labels to apply to resources created by the driver.
  • Fix setting CLEANUP_JOBS
    I had the if statement inverted... I've added a test to make sure it (and other settings) are correct.
  • 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.
  • 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.
    • 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

With these changes I am able to run @squillace 's kubeflow bundle! 🎉

apiVersion: porter.sh/v1
kind: Installation
metadata:
  name: kubeflow
spec:
  reference: "ghcr.io/squillace/aks-kubeflow-msi:v0.1.7"
  action: "install"
  credentialSets:
    - aks

Closes #73

@carolynvs carolynvs requested review from vdice and radu-matei February 1, 2021 16:40
@carolynvs carolynvs force-pushed the k8s-driver-improvements branch from 7af2c1a to 5e72b5a Compare February 1, 2021 17:06
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]>
@carolynvs carolynvs force-pushed the k8s-driver-improvements branch from 5e72b5a to 3ba15dc Compare February 2, 2021 13:48
carolynvs added a commit to carolynvs/porter that referenced this pull request Feb 2, 2021
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]>
Copy link
Member

@vdice vdice left a 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?

@carolynvs
Copy link
Contributor Author

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]>
Base automatically changed from master to main February 5, 2021 15:02
@carolynvs carolynvs merged commit 7896782 into cnabio:main Feb 26, 2021
@carolynvs carolynvs deleted the k8s-driver-improvements branch February 26, 2021 15:48
simongdavies pushed a commit to simongdavies/cnab-go that referenced this pull request Mar 31, 2021
* 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]>
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.

[spec compliance] Implement Outputs in Kubernetes Driver
3 participants