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.

Sign up for GitHub

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

Jump to bottom

OpenStack: Availability zones for root volumes #4707

Merged
merged 6 commits into from
Apr 8, 2021
Merged

OpenStack: Availability zones for root volumes #4707

merged 6 commits into from
Apr 8, 2021

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Mar 3, 2021 •
edited
Loading

This PR allows users to specify custom availability zones for their root volumes.

Implements: https://issues.redhat.com/browse/OSASINFRA-2226
Enhancement proposal: openshift/enhancements#691

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2021
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 3, 2021

/retest

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 3, 2021

/test e2e-openstack

@Fedosin Fedosin force-pushed the volume_az branch 2 times, most recently from 3c0cd44 to d1e3d0b Compare March 3, 2021 21:45
@Fedosin Fedosin changed the title WIP: OpenStack: Availability zones for root volumes OpenStack: Availability zones for root volumes Mar 17, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2021
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 17, 2021

/retest

1 similar comment
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 18, 2021

/retest

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 18, 2021

/test e2e-openstack

3 similar comments
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 19, 2021

/test e2e-openstack

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 21, 2021

/test e2e-openstack

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 23, 2021

/test e2e-openstack

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 23, 2021

/assign @mandre

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 23, 2021

/test openstack-manifests

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 23, 2021

/test e2e-openstack

mandre
mandre reviewed Mar 23, 2021
View reviewed changes
Copy link
Member

@mandre mandre left a comment

Choose a reason for hiding this comment

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

Just a small comment about validation that's going to fail if your compute and cinder AZs don't match, otherwise lgtm.

pkg/asset/installconfig/openstack/validation/machinepool.go Outdated Show resolved Hide resolved
@mandre
Copy link
Member

mandre commented Mar 26, 2021

/retest

1 similar comment
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 26, 2021

/retest

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 26, 2021

/test e2e-openstack

2 similar comments
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 29, 2021

/test e2e-openstack

@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 29, 2021

/test e2e-openstack

@Fedosin
Copy link
Contributor Author

Fedosin commented Apr 1, 2021

/retest

1 similar comment
@Fedosin
Copy link
Contributor Author

Fedosin commented Apr 3, 2021

/retest

@Fedosin
Copy link
Contributor Author

Fedosin commented Apr 3, 2021

/test e2e-openstack

@Fedosin
Copy link
Contributor Author

Fedosin commented Apr 4, 2021

/test e2e-aws-upgrade

staebler
staebler reviewed Apr 5, 2021
View reviewed changes
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

Please improve on the commit messages. Between the commit messages being sparse and the PR description being empty, we are missing a lot of color around these changes.

pkg/asset/machines/openstack/machines.go Outdated
@@ -46,6 +46,11 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine
return nil, err
}

volumeAZs := []string{""}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the default []string{""} value for RootVolume.Zones to defaultOpenStackMachinePoolPlatform so that we don't have to have this code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can't... RootVolume is an optional parameter for machine pool, and it's not enabled by default (in other words it is nil in defaultOpenStackMachinePoolPlatform). If we add []string{""} value for RootVolume.Zones, it means that the default would be:

openstacktypes.MachinePool{
		Zones: []string{""},
		RootVolume: &openstacktypes.RootVolume{
			Zones: []string{""},
		},
	}

And we always create the RootVolume even if it's not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead, I added the default value in "github.com/openshift/installer/pkg/types/openstack/defaults" which imo is more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes sense about not being able to set it in defaultOpenStackMachinePoolPlatform. It would be nice to be able to set the default value when the install config defaults are set then, but I won't hold up this PR for that.

pkg/tfvars/openstack/openstack.go Show resolved Hide resolved
@Fedosin Fedosin force-pushed the volume_az branch 2 times, most recently from c8bfb83 to c14cf84 Compare April 6, 2021 15:47
staebler
staebler reviewed Apr 6, 2021
View reviewed changes
pkg/types/openstack/defaults/machinepool.go Outdated Show resolved Hide resolved
pkg/asset/machines/openstack/machines.go Outdated
@@ -46,6 +46,11 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine
return nil, err
}

volumeAZs := []string{""}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes sense about not being able to set it in defaultOpenStackMachinePoolPlatform. It would be nice to be able to set the default value when the install config defaults are set then, but I won't hold up this PR for that.

@Fedosin Fedosin force-pushed the volume_az branch 2 times, most recently from 65bba9a to 40b23ab Compare April 6, 2021 16:55
staebler
staebler reviewed Apr 8, 2021
View reviewed changes
Copy link
Contributor

@staebler staebler 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.
Please improve the commit messages. At a minimum, make it clear that the changes are for OpenStack.

@staebler
Copy link
Contributor

staebler commented Apr 8, 2021

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021
@Fedosin
OpenStack: Add Zones to Root Volume
d1df8f2 
This commit adds a new optional list of strings parameter called
Zones to OpenStack's Root Volume. When it's set, OpenShift will create
instance root volumes in the specified availability zones.
@Fedosin
Bump github.com/openshift/cluster-api-provider-openstack
b288d88
@Fedosin
OpenStack: Add Volume AZs to MachineProviderSpec
81ffe6b 
This commit allows to generate OpenStack Machine and MachineSet
assets considering Root Volume zones parameter from the install
config.
@Fedosin
OpenStack: Add volume AZs to Terraform
e111c46 
This commit allows to generate Terraform manifests for masters
considering Root Volume availability zones.
@Fedosin
OpenStack: Add manifest tests for root volume AZs
54a61b2 
This commit adds specific manifests tests to check Machines and
MachineSets manifests generating when Root Volume availability
zones are set.
@Fedosin
OpenStack: Update docs for root volume availability zones
b6f737d
@Fedosin
Copy link
Contributor Author

Fedosin commented Apr 8, 2021

/test e2e-aws-upgrade

@mandre
Copy link
Member

mandre commented Apr 8, 2021

/lgtm
/hold for CI results

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 8, 2021
@mandre
Copy link
Member

mandre commented Apr 8, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 8, 2021 •
edited
Loading

@Fedosin: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-workers-rhel7 b6f737d link /test e2e-aws-workers-rhel7
ci/prow/e2e-metal-ipi-ovn-ipv6 b6f737d link /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-crc b6f737d link /test e2e-crc
ci/prow/e2e-libvirt b6f737d link /test e2e-libvirt

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 6d778f9 into openshift:master Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mandre mandre mandre left review comments

@staebler staebler staebler left review comments

@EmilienM EmilienM Awaiting requested review from EmilienM

@jhixson74 jhixson74 Awaiting requested review from jhixson74

Assignees

@mandre mandre

@staebler staebler

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Fedosin @mandre @staebler @openshift-ci-robot @openshift-merge-robot