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.

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

Deploy shoots in existing Neutron networks #317

Merged
merged 3 commits into from
Aug 6, 2021

Conversation

kon-angelo
Copy link
Contributor

How to categorize this PR?

/area control-plane
/kind enhancement
/platform openstack

What this PR does / why we need it:
Allows the creation of shoots into existing user-managed networks.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Shoots can now be deployed in existing Neutron networks. The network can be specified by its ID in the respective shoot's infrastructure configuration.

@kon-angelo kon-angelo requested review from a team as code owners July 20, 2021 16:33
@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension platform/openstack OpenStack platform/infrastructure needs/review Needs review size/l Size of pull request is large (see gardener-robot robot/bots/size.py) labels Jul 20, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 20, 2021
@dkistner
Copy link
Member

/assign

Copy link
Member

@dkistner dkistner left a comment

Choose a reason for hiding this comment

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

Had a first look and looks overall good.
Just a few minor comments and the question how we proceed with the subnet id in the machine classes.

@@ -62,6 +63,8 @@ If you don't know which floating pools are available look it up in the respectiv

With `floatingPoolSubnetName` you can explicitly define to which subnet in the floating pool network (defined via `floatingPoolName`) the router should be attached to.

If `networks.id` is an optional field. If it is given, you can specify the uuid of an existing Neutron network (created manually, by other tooling, ...) that should be reused. A new subnet for the Shoot will be created in it.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If `networks.id` is an optional field. If it is given, you can specify the uuid of an existing Neutron network (created manually, by other tooling, ...) that should be reused. A new subnet for the Shoot will be created in it.
If `networks.id` is an optional field. If it is given, you can specify the uuid of an existing private Neutron network (created manually, by other tooling, ...) that should be reused. A new subnet for the Shoot will be created in it.

@@ -41,6 +41,8 @@ type Networks struct {
Worker string
// Workers is a CIDRs of a worker subnet (private) to create (used for the VMs).
Workers string
// ID is the name of an existing network that should be reused
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// ID is the name of an existing network that should be reused
// ID is the name of an existing private network that should be reused.

pkg/apis/openstack/v1alpha1/types_infrastructure.go Outdated Show resolved Hide resolved
pkg/apis/openstack/validation/infrastructure.go Outdated Show resolved Hide resolved
pkg/apis/openstack/validation/infrastructure_test.go Outdated Show resolved Hide resolved
@@ -158,6 +163,10 @@ func (w *workerDelegate) generateMachineConfig(ctx context.Context) error {
},
}

if !infrastructureStatus.Networks.ManagedPrivateNetwork {
machineClassSpec["subnetID"] = subnet.ID
Copy link
Member

Choose a reason for hiding this comment

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

Here we had the discussion if not now always want to pass the subnet id right?
That would force mcm for all new machine to manage also the port. Existing machines would not be rolled as the subnet id is not passed to the worker pool hash, right?

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 27, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 27, 2021
Co-authored-by: Dominic Kistner <dominic.kistner@sap.com>
Co-authored-by: Thomas Buchner <MrBatschner@users.noreply.github.com>
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 27, 2021
@gardener-robot gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/l Size of pull request is large (see gardener-robot robot/bots/size.py) labels Jul 30, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
Copy link
Member

@dkistner dkistner left a comment

Choose a reason for hiding this comment

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

I played a little with it. Just a small comment.

Otherwise it looks good to me.

@@ -37,6 +37,9 @@ providerSpec:
availabilityZone: {{ $machineClass.availabilityZone }}
flavorName: {{ $machineClass.machineType }}
keyName: {{ $machineClass.keyName }}
{{- if $machineClass.subnetID }}
Copy link
Member

Choose a reason for hiding this comment

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

Is that check now still required when we always pass a subnet id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else platform/openstack OpenStack platform/infrastructure size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants