OpenStack: allow to specify additional networks and security groups for masters and workers #3291

Fedosin · 2020-03-13T17:19:58Z

In some cases we may need to provide access to additional networks and security groups. An example of this need would be to mount RWX volumes from OpenStack Manila, when shares are in another network (OSP default).
Since we plan to use Manila as the primary backend for the image registry, it is essential that we have this feature in IPI.

This PR adds two new parameters to OpenStack's MachinePool:

additionalNetworkIDs (optional list of strings): IDs of additional networks for machines.
additionalSecurityGroupIDs (optional list of strings): IDs of additional security groups for machines.

pkg/asset/machines/openstack/machines.go

pkg/types/openstack/platform.go

pkg/asset/machines/openstack/machines.go

Fedosin · 2020-03-16T16:09:11Z

/hold

abhinavdahiya · 2020-03-17T02:39:12Z

if this get's merged, it would be very nice to have an enhancement created to document what user stories / use case this is solving.

for other cloud platforms we don't allow these additional sets unless they are necessary for successful installation... because otherwise we are increasing the surface area of configuration at install-time, which needs to be tested, and which goes against the OpenShift 4 goals.

if this is to just ease process of users, i would recommend we look into controller driven OLM operator like component to achieve this as a day-2 configuration.

Fedosin · 2020-03-17T09:48:26Z

@abhinavdahiya yeah, documentation is coming. most likely when you read this message, it's already available.

We need this change to enable OpenStack Manila (shared filesystem) integration in IPI. This is not a hard requirement, but in OSP we create an additional network for shared volumes, and we have to add workers to this network to allow mounting.
This is also important for IPI, because we plan to use Manila as a backend for the image registry (RWX PVC), and without this PR we can't do it properly.

Fedosin · 2020-03-17T10:13:38Z

/hold cancel

Fedosin · 2020-03-17T10:38:31Z

/retest

Fedosin · 2020-03-17T12:43:26Z

/test e2e-aws

Fedosin · 2020-03-20T13:01:56Z

/hold

blocked by openshift/cluster-api-provider-openstack#86

Fedosin · 2020-03-23T13:58:39Z

/hold cancel

Fedosin · 2020-03-23T18:50:33Z

/test e2e-openstack

Fedosin · 2020-03-23T21:31:04Z

/test e2e-openstack

pkg/tfvars/openstack/openstack.go

pkg/types/openstack/machinepool.go

This commit adds validations that all provided network and security group ids are UUID v4 strings.

Fedosin · 2020-04-03T21:22:56Z

/test e2e-openstack

Fedosin · 2020-04-04T20:59:22Z

/test e2e-openstack

Fedosin · 2020-04-05T03:34:03Z

/test e2e-openstack

Fedosin · 2020-04-06T20:06:20Z

/test e2e-openstack

Fedosin · 2020-04-06T20:24:24Z

/test e2e-openstack

abhinavdahiya · 2020-04-06T20:41:02Z

pkg/asset/machines/openstack/machines.go

+	networks := []openstackprovider.NetworkParam{
+		{
+			Subnets: []openstackprovider.SubnetParam{
+				{
+					Filter: openstackprovider.SubnetFilter{
+						Name: fmt.Sprintf("%s-nodes", clusterID),
+						Tags: fmt.Sprintf("%s=%s", "openshiftClusterID", clusterID),
+					},
+				},
+			},
+		},
+	}


nit:

Suggested change

networks := []openstackprovider.NetworkParam{

{

Subnets: []openstackprovider.SubnetParam{

{

Filter: openstackprovider.SubnetFilter{

Name: fmt.Sprintf("%s-nodes", clusterID),

Tags: fmt.Sprintf("%s=%s", "openshiftClusterID", clusterID),

},

},

},

},

}

networks := []openstackprovider.NetworkParam{{

Subnets: []openstackprovider.SubnetParam{{

Filter: openstackprovider.SubnetFilter{

Name: fmt.Sprintf("%s-nodes", clusterID),

Tags: fmt.Sprintf("%s=%s", "openshiftClusterID", clusterID),

},

}},

}}

abhinavdahiya · 2020-04-06T20:45:33Z

/approve

openshift-ci-robot · 2020-04-06T20:45:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, mandre

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~~ [abhinavdahiya]

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

Fedosin · 2020-04-06T20:46:49Z

/hold

iamemilio · 2020-04-07T17:22:52Z

pkg/types/openstack/validation/machinepool.go

@@ -20,5 +21,35 @@ func ValidateMachinePool(p *openstack.MachinePool, fldPath *field.Path) field.Er
 		}
 	}

+	allErrs = append(allErrs, validateUUIDV4s(p.AdditionalNetworkIDs, fldPath.Child("additionalNetworkIDs"))...)


Should we be using the valid values fetcher here to validate that these networks and security groups actually exist in openstack?

This will be done as a part of our Validation epic

iamemilio · 2020-04-07T17:30:00Z

pkg/asset/cluster/tfvars.go

@@ -390,6 +390,7 @@ func (t *TerraformVariables) Generate(parents asset.Parents) error {
 			clusterID.InfraID,
 			caCert,
 			bootstrapIgn,
+			installConfig.Config.ControlPlane.Platform.OpenStack,


You dont have to do this in this patch, but could we just pass the install config here, or make a new struct to pass tfvars? The number of arguments to this function is becoming ludicrous :)

I know... Abhinav asked the same question #3291 (comment)
But I think we don't have other options, because we need to differenciate between primary and additional objects, and the only way is to read them from the machine pool :(

iamemilio · 2020-04-08T20:49:12Z

/lgtm

Fedosin · 2020-04-08T21:53:53Z

/hold cancel

openshift-bot · 2020-04-08T22:02:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-08T22:28:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-08T23:07:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-08T23:20:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-04-08T23:27:11Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`c1329de`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-libvirt	`c1329de`	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-ci-robot requested review from jhixson74 and patrickdillon March 13, 2020 17:20

iamemilio reviewed Mar 13, 2020

View reviewed changes

pkg/asset/machines/openstack/machines.go Outdated Show resolved Hide resolved

iamemilio reviewed Mar 13, 2020

View reviewed changes

pkg/asset/machines/openstack/machines.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Mar 16, 2020

View reviewed changes

pkg/types/openstack/platform.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Mar 16, 2020

View reviewed changes

pkg/asset/machines/openstack/machines.go Outdated Show resolved Hide resolved

Fedosin force-pushed the secondary_vnics branch from 853ebb9 to 9cdb9ab Compare March 16, 2020 14:29

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

Fedosin force-pushed the secondary_vnics branch from 9cdb9ab to ac8c26d Compare March 16, 2020 23:42

Fedosin changed the title ~~POC: OpenStack: allow to specify additional subnets and sg for workers~~ OpenStack: allow to specify additional networks and security groups for masters and workers Mar 16, 2020

Fedosin force-pushed the secondary_vnics branch 4 times, most recently from 7e241f8 to 0023428 Compare March 17, 2020 00:13

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

Fedosin force-pushed the secondary_vnics branch from b79ba33 to 25ec4aa Compare March 17, 2020 21:19

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

Fedosin force-pushed the secondary_vnics branch from 25ec4aa to cbf221f Compare March 23, 2020 13:58

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

Fedosin force-pushed the secondary_vnics branch from cbf221f to 240f2a3 Compare March 23, 2020 14:16

abhinavdahiya reviewed Apr 3, 2020

View reviewed changes

pkg/tfvars/openstack/openstack.go Show resolved Hide resolved

abhinavdahiya reviewed Apr 3, 2020

View reviewed changes

pkg/types/openstack/machinepool.go Show resolved Hide resolved

Fedosin added 3 commits April 3, 2020 20:32

Allow to specify additional networks and security groups

5654769

Document additionalNetworks and additional_security_group parameters

0399625

Add type validations

c1329de

This commit adds validations that all provided network and security group ids are UUID v4 strings.

Fedosin force-pushed the secondary_vnics branch from 05410bc to c1329de Compare April 3, 2020 18:33

abhinavdahiya reviewed Apr 6, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2020

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

iamemilio reviewed Apr 7, 2020

View reviewed changes

openshift-ci-robot assigned iamemilio Apr 8, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2020

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, 2020

openshift-merge-robot merged commit e11c2b8 into openshift:master Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenStack: allow to specify additional networks and security groups for masters and workers #3291

OpenStack: allow to specify additional networks and security groups for masters and workers #3291

Fedosin commented Mar 13, 2020 •

edited

Fedosin commented Mar 16, 2020

abhinavdahiya commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 20, 2020

Fedosin commented Mar 23, 2020

Fedosin commented Mar 23, 2020

Fedosin commented Mar 23, 2020

Fedosin commented Apr 3, 2020

Fedosin commented Apr 4, 2020

Fedosin commented Apr 5, 2020

Fedosin commented Apr 6, 2020

Fedosin commented Apr 6, 2020

abhinavdahiya Apr 6, 2020

abhinavdahiya commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 2020

Fedosin commented Apr 6, 2020

iamemilio Apr 7, 2020

Fedosin Apr 8, 2020

iamemilio Apr 7, 2020

Fedosin Apr 8, 2020

iamemilio commented Apr 8, 2020

Fedosin commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-ci-robot commented Apr 8, 2020

OpenStack: allow to specify additional networks and security groups for masters and workers #3291

OpenStack: allow to specify additional networks and security groups for masters and workers #3291

Conversation

Fedosin commented Mar 13, 2020 • edited

Fedosin commented Mar 16, 2020

abhinavdahiya commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 17, 2020

Fedosin commented Mar 20, 2020

Fedosin commented Mar 23, 2020

Fedosin commented Mar 23, 2020

Fedosin commented Mar 23, 2020

Fedosin commented Apr 3, 2020

Fedosin commented Apr 4, 2020

Fedosin commented Apr 5, 2020

Fedosin commented Apr 6, 2020

Fedosin commented Apr 6, 2020

abhinavdahiya Apr 6, 2020

Choose a reason for hiding this comment

abhinavdahiya commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 2020

Fedosin commented Apr 6, 2020

iamemilio Apr 7, 2020

Choose a reason for hiding this comment

Fedosin Apr 8, 2020

Choose a reason for hiding this comment

iamemilio Apr 7, 2020

Choose a reason for hiding this comment

Fedosin Apr 8, 2020

Choose a reason for hiding this comment

iamemilio commented Apr 8, 2020

Fedosin commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-bot commented Apr 8, 2020

openshift-ci-robot commented Apr 8, 2020

Fedosin commented Mar 13, 2020 •

edited