Add support for multiple block device mappings #299
Conversation
openshift-ci-robot
added
the
size/M
pkg/actuators/machine/instances.go
Outdated
| Encrypted: blockDeviceMappingSpec.EBS.Encrypted, | ||
| }, | ||
| } | ||
| if *volumeType == "io1" { |
There was a problem hiding this comment.
should we have this "io1" value a constant somewhere so it doesn't seem like a magic value ?
| volumeType := blockDeviceMappingSpec.EBS.VolumeType | ||
|
|
||
| blockDeviceMapping := ec2.BlockDeviceMapping{ | ||
| DeviceName: deviceName, |
There was a problem hiding this comment.
This is going to attempt to give all devices the same name.
There was a problem hiding this comment.
I think it's expected that DeviceName should always be set apart from on the root device, though I don't think there's any validation on that at all since these aren't CRDs and we don't have schemas or webhooks for them.
Perhaps we can do validation here and return an error if more than one of the block device mappings doesn't have a name set?
There was a problem hiding this comment.
@JoelSpeed I thought of this but wasn't sure that's a right thing to do in that case
There was a problem hiding this comment.
Given we don't have any webhooks set up and we use a rawextension, I'm not sure what else we can do, I may well be missing something though, there may be a better way
There was a problem hiding this comment.
We don't need webhooks. We can just fail the machine's reconcile and quit attempting. I consider this an advanced usecase, so the user better follow the docs and set it up correctly. This will probably also require investigation from the user to determine what a valid block device name is for their instance type.
There was a problem hiding this comment.
Coming back to this, we should make sure the PR that is adding webhooks should validate this, or we should follow up to update that (cc @enxebre)
There was a problem hiding this comment.
Captured this here -> https://github.com/openshift/machine-api-operator/pull/601/files#diff-98bc71b7347ada0763f9e57ae6c9d235R320
Regardless this should error and return and invalid config which the machine controller can use to fail. This seems to be case already.
openshift-ci-robot
added
size/L
pkg/actuators/machine/instances.go
Outdated
| volumeType := blockDeviceMappingSpec.EBS.VolumeType | ||
|
|
||
| if blockDeviceMappingSpec.DeviceName == nil { | ||
| if onlyOneEmptyDeviceName { |
There was a problem hiding this comment.
I think this name could be a little confusing maybe, since we go into this if statement if there is more than one empty device name. Perhaps rootDeviceFound could be a better name?
|
/retest |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
Just re-reviewed, I'm happy with it code wise, left a comment re validation |
pkg/actuators/machine/instances.go
Outdated
|
|
||
| if blockDeviceMappingSpec.DeviceName == nil { | ||
| if rootDeviceFound { | ||
| return nil, errors.New("more than one block device doesn't have a name") |
There was a problem hiding this comment.
may be "non root device must have deviceName"
There was a problem hiding this comment.
Can we please document with a comment in the API BlockDeviceMappingSpec definition https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/apis/awsprovider/v1beta1/awsproviderconfig_types.go#L86 that a block device with out a deviceName will be used for the root device and only one with no deviceName is allowed.
pkg/actuators/machine/instances.go
Outdated
| } | ||
|
|
||
| if *volumeType == ec2.VolumeTypeIo1 { | ||
| blockDeviceMapping.Ebs.Iops = blockDeviceMappingSpecs[0].EBS.Iops |
There was a problem hiding this comment.
why is this and below setting from blockDeviceMappingSpecs[0]? shouldn't it use blockDeviceMappingSpec?
There was a problem hiding this comment.
👍 leftover after rebase
pkg/actuators/machine/instances.go
Outdated
| } | ||
|
|
||
| if aws.StringValue(blockDeviceMappingSpecs[0].EBS.KMSKey.ID) != "" { | ||
| klog.V(3).Infof("Using KMS key ID %q for encrypting EBS volume", *blockDeviceMappingSpecs[0].EBS.KMSKey.ID) |
There was a problem hiding this comment.
s/ blockDeviceMappingSpecs[0] / blockDeviceMappingSpecs ?
There was a problem hiding this comment.
Isn’t this line still using wrong block device https://github.com/openshift/cluster-api-provider-aws/pull/299/files#diff-24a7c87e11cc6413298ddaaa7e7abda0R240?
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
This PR adds support for multiple block device mappings. The problem it solves is that machine is always created with only one disk. One of the use cases is the ability to store logs, empty dir pods data or docker images in a separate place and not have it in root one.