Skip to content
This repository has been archived by the owner on Sep 6, 2024. It is now read-only.

feat(757): Change toleration key from disk to screwdriver.cd/disk #42

Merged
merged 3 commits into from
Sep 14, 2018

Conversation

Filbird
Copy link
Member

@Filbird Filbird commented Sep 12, 2018

Context

We should use a less generic label than disk=high.

Objective

Change the toleration/pod affinity key from disk to screwdriver.cd/disk.

References

screwdriver-cd/screwdriver#757

index.js Outdated
@@ -252,7 +252,8 @@ class K8sVMExecutor extends Executor {
const podConfig = yaml.safeLoad(podTemplate);

const diskConfig = annotations[DISK_RESOURCE] || '';
const nodeSelectors = diskConfig.toUpperCase() === 'HIGH' ? { disk: 'high' } : {};
const nodeSelectors = diskConfig.toUpperCase() === 'HIGH' ?
{ 'screwdriver.cd/disk': 'high' } : {};
Copy link
Member

@minzcmu minzcmu Sep 12, 2018

Choose a reason for hiding this comment

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

Shall we make this label configurable? Otherwise later on if we decide to change the label, we need to actually change this line.

https://github.com/screwdriver-cd/queue-worker/blob/master/config/default.yaml#L31

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, so something like options -> kubernetes -> labels -> disk, then bind it it to the executor as part of the constructer?

Copy link
Member Author

@Filbird Filbird Sep 12, 2018

Choose a reason for hiding this comment

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

Added an extra commit to configure the disk label. Should not be blocked on the queue-worker implementation since we default to screwdriver.cd/disk if the option is missing.

screwdriver-cd/queue-worker#97

Copy link

@chestery chestery left a comment

Choose a reason for hiding this comment

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

This label lgtm. But not sure this will be part the high resource profile or it is a separate dimension of configurations.

index.js Outdated
@@ -157,6 +157,8 @@ class K8sVMExecutor extends Executor {
this.highMemory = hoek.reach(options, 'kubernetes.resources.memory.high', { default: 12 });
this.lowMemory = hoek.reach(options, 'kubernetes.resources.memory.low', { default: 2 });
this.microMemory = hoek.reach(options, 'kubernetes.resources.memory.micro', { default: 1 });
this.diskLabel = hoek.reach(options, 'kubernetes.labels.disk',
{ default: 'screwdriver.cd/disk' });
Copy link
Member

Choose a reason for hiding this comment

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

same concern about setting the default label as my comment in screwdriver-cd/queue-worker#97 (review).

@minzcmu minzcmu merged commit 75079e6 into master Sep 14, 2018
@minzcmu minzcmu deleted the disk branch September 14, 2018 17:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants