✨ Add extra root volume options #1498

johnharris85 · 2020-01-25T18:00:01Z

Signed-off-by: John Harris joharris@vmware.com

What this PR does / why we need it:
Allows users to specify additional options for a machine's root volume.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1403

Notes:

If a user wants to specify a KMS key, they need to ensure the relevant user / role has use permissions on the key. Should this be documented somewhere? (if so, where?)
If EncryptionKey is set, we set Encrypted to true (even if it was omitted / set to false). Thought this was a reasonable assumption, but open to changes there (error if key set but encrypted missing / false).

k8s-ci-robot · 2020-01-25T18:00:10Z

Hi @johnharris85. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ncdc · 2020-01-27T15:17:29Z

/ok-to-test
/assign

ncdc

Generally LGTM.

/approve

/assign @vincepri

api/v1alpha3/awsmachine_webhook.go

api/v1alpha3/types.go

k8s-ci-robot · 2020-01-27T15:28:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnharris85, ncdc

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:

~~OWNERS~~ [ncdc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

johnharris85 · 2020-01-27T15:29:22Z

/retest

johnharris85 · 2020-01-27T15:47:50Z

@vincepri pointed out on Slack this should have some restore logic built in to make sure we don't miss these fields from annotations. Will add that and address Andy's comments later today.

vincepri · 2020-01-27T15:48:30Z

Sounds good! Holding the PR until we have those in place

/hold

pkg/cloud/services/ec2/instances.go

api/v1alpha2/awsmachine_conversion.go

api/v1alpha3/types.go

pkg/cloud/services/ec2/instances.go

johnharris85 · 2020-02-01T14:01:37Z

Addressed most of @vincepri comments (except the size question) and rebased.

vincepri · 2020-02-03T18:55:37Z

@johnharris85 Regarding #1498, I think the only thing I'd add is a kubebuilder validation tag that checks that Size is greater than 0, and remove all the checks where we do .Size != 0.

This makes sure that when a user sets the rootVolume struct, the size field should never be zero, if the struct is not-nil.

johnharris85 · 2020-02-04T17:52:13Z

pkg/cloud/services/ec2/instances.go

+func (s *Service) getImageSnapshotSize(imageID string) (*int64, error) {
+	input := &ec2.DescribeImagesInput{
+		ImageIds: []*string{aws.String(imageID)},
+	}
+
+	output, err := s.scope.EC2.DescribeImages(input)
+	if err != nil {
+		return nil, err
+	}
+
+	if len(output.Images) == 0 {
+		return nil, errors.Errorf("no images returned when looking up ID %q", imageID)
+	}
+
+	return output.Images[0].BlockDeviceMappings[0].Ebs.VolumeSize, nil
+}
+


This func is basically identical to the one above (to get the root device name), just returning a different attribute. Thoughts on adjusting the above one (which is only used once) to return size and name to save an AWS call?

+1 to consolidating the calls. This can also be handled as a followup.

config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml

vincepri

/lgtm

@ncdc over to you to un-hold

johnharris85 · 2020-02-04T17:55:40Z

Discussion w/ @vincepri on Slack around how to handle the size wrt defaults / validation -> https://app.slack.com/client/T09NY5SBT/CD6U2V71N/thread/CD6U2V71N-1580776492.150700.

Addressed all comments I think but left a couple questions in for feedback, can squash when ready.

johnharris85 · 2020-02-04T18:07:06Z

Failure looks like generation out of date, but make verify succeeds on my machine...? (and make generate doesn't generate any changes?) Am I missing something?

ncdc · 2020-02-04T18:09:47Z

@johnharris85 try to rebase on top of master

vincepri · 2020-02-04T19:30:39Z

api/v1alpha3/awsmachine_webhook_test.go:27: File is not `gofmt`-ed with `-s` (gofmt)
		name       string
api/v1alpha2/awsmachine_conversion_test.go:154: File is not `gofmt`-ed with `-s` (gofmt)
}

@johnharris85 seems linting errors now

randomvariable · 2020-02-06T10:11:12Z

Should probably queue this one after #1525 so we get the object fuzzer, and then this will then need rebasing I think.

johnharris85 · 2020-02-07T19:17:25Z

OK rebased now #1525 landed. Fuzzer found a couple bugs which I also fixed.

AWSClusterStatus conversion funcs were using the auto conversions for Instance, changed to using the manual ones (with the additional volume options)
AWSCluster conversion func needed to restore the bastion if present (from annotation)

randomvariable · 2020-02-10T11:07:39Z

/unhold
as we have the restore logic in place.

randomvariable · 2020-02-10T11:09:38Z

Should merge cleanly via 3-way, but merge after #1535

ncdc · 2020-02-10T18:11:25Z

@johnharris85 could you please rebase & resolve conflicts - thanks!

Signed-off-by: John Harris <joharris@vmware.com>

detiber · 2020-02-11T15:45:37Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2020

k8s-ci-robot requested review from detiber and sethp-nr January 25, 2020 18:00

k8s-ci-robot assigned ncdc Jan 27, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 27, 2020

ncdc reviewed Jan 27, 2020

View reviewed changes

api/v1alpha3/awsmachine_webhook.go Outdated Show resolved Hide resolved

api/v1alpha3/types.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned vincepri Jan 27, 2020

johnharris85 force-pushed the expand-disk-options branch from d187f47 to 1f9b806 Compare January 27, 2020 15:28

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2020

ncdc added this to the v0.5.0 milestone Jan 27, 2020

jzhoucliqr reviewed Jan 30, 2020

View reviewed changes

pkg/cloud/services/ec2/instances.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2020

detiber mentioned this pull request Jan 31, 2020

🏃 Remove deprecated AvailabilityZone field on machine spec #1509

Merged

vincepri reviewed Jan 31, 2020

View reviewed changes

api/v1alpha2/awsmachine_conversion.go Outdated Show resolved Hide resolved

api/v1alpha3/types.go Outdated Show resolved Hide resolved

api/v1alpha3/types.go Outdated Show resolved Hide resolved

pkg/cloud/services/ec2/instances.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2020

johnharris85 force-pushed the expand-disk-options branch from c31d35b to 91ff282 Compare February 1, 2020 13:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2020

johnharris85 commented Feb 4, 2020

View reviewed changes

config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinetemplates.yaml Show resolved Hide resolved

vincepri approved these changes Feb 4, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2020

johnharris85 force-pushed the expand-disk-options branch from c76f066 to 1c785f6 Compare February 4, 2020 19:07

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2020

johnharris85 force-pushed the expand-disk-options branch from 1c785f6 to 66b4292 Compare February 4, 2020 19:28

johnharris85 force-pushed the expand-disk-options branch 2 times, most recently from 0ca9fe7 to 1f80852 Compare February 7, 2020 19:03

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 10, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020

Add extra root volume options

80af1bc

Signed-off-by: John Harris <joharris@vmware.com>

johnharris85 force-pushed the expand-disk-options branch from 1f80852 to 80af1bc Compare February 10, 2020 19:26

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020

ncdc assigned detiber and unassigned ncdc Feb 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2020

k8s-ci-robot merged commit 81e5184 into kubernetes-sigs:master Feb 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add extra root volume options #1498

✨ Add extra root volume options #1498

johnharris85 commented Jan 25, 2020

k8s-ci-robot commented Jan 25, 2020

ncdc commented Jan 27, 2020

ncdc left a comment

k8s-ci-robot commented Jan 27, 2020

johnharris85 commented Jan 27, 2020

johnharris85 commented Jan 27, 2020

vincepri commented Jan 27, 2020

johnharris85 commented Feb 1, 2020

vincepri commented Feb 3, 2020

johnharris85 Feb 4, 2020

detiber Feb 11, 2020

vincepri left a comment

johnharris85 commented Feb 4, 2020

johnharris85 commented Feb 4, 2020

ncdc commented Feb 4, 2020

vincepri commented Feb 4, 2020

randomvariable commented Feb 6, 2020

johnharris85 commented Feb 7, 2020

randomvariable commented Feb 10, 2020

randomvariable commented Feb 10, 2020

ncdc commented Feb 10, 2020

detiber commented Feb 11, 2020

✨ Add extra root volume options #1498

✨ Add extra root volume options #1498

Conversation

johnharris85 commented Jan 25, 2020

k8s-ci-robot commented Jan 25, 2020

ncdc commented Jan 27, 2020

ncdc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 27, 2020

johnharris85 commented Jan 27, 2020

johnharris85 commented Jan 27, 2020

vincepri commented Jan 27, 2020

johnharris85 commented Feb 1, 2020

vincepri commented Feb 3, 2020

johnharris85 Feb 4, 2020

Choose a reason for hiding this comment

detiber Feb 11, 2020

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

johnharris85 commented Feb 4, 2020

johnharris85 commented Feb 4, 2020

ncdc commented Feb 4, 2020

vincepri commented Feb 4, 2020

randomvariable commented Feb 6, 2020

johnharris85 commented Feb 7, 2020

randomvariable commented Feb 10, 2020

randomvariable commented Feb 10, 2020

ncdc commented Feb 10, 2020

detiber commented Feb 11, 2020