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 VMs into existing OpenStack Neutron network #545

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

MrBatschner
Copy link
Contributor

What this PR does / why we need it:
Adds the ability to specify an already existing OpenStack Neutron network in the subnetID of an OpenStackMachineClass. MCM will deploy new machines into the given subnet by pre-allocating Neutron ports and pass them to the Nova server object. Pre-allocated ports will be specifically removed upon server termination. This is required to specify an existing subnet in a shoot manifest.
With this PR it is only possible to specify an already existing network, not a list of otherwise pre-allocated ports.

Which issue(s) this PR fixes:
Partly fixes #535

Special notes for your reviewer:
This change is a necessary precondition for PR 169 in gardener-extension-provider-openstack.

Release note:

Adds the ability to specify an already existing OpenStack Neutron network in the subnetID of an OpenStackMachineClass. MCM will deploy new machines into the given subnet by pre-allocating Neutron ports and pass them to the Nova server object. 

@MrBatschner MrBatschner requested review from ggaurav10 and a team as code owners October 27, 2020 15:39
@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Oct 27, 2020
@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 Oct 27, 2020
@MrBatschner
Copy link
Contributor Author

/invite @dkistner
/invite @kon-angelo

@gardener-robot-ci-1 gardener-robot-ci-1 added 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 Oct 27, 2020
@MrBatschner MrBatschner changed the title Existing network Deploy VMs into existing OpenStack Neutron network Oct 27, 2020
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 brief look and seems to look good.
I need a little bit more time to play around with it.

@gardener-robot gardener-robot removed the needs/review Needs review label Oct 28, 2020
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 guess we need to make sure that the port gets also deleted when the machine/server is already gone. Currently the port deletion logic is not executed when the machine is already deleted. We need to ensure that we check within each machine deletion flow for existing ports. See here:

func (d *OpenStackDriver) Delete(machineID string) error {
res, err := d.GetVMs(machineID)
if err != nil {
return err
} else if len(res) == 0 {
// No running instance exists with the given machine-ID
klog.V(2).Infof("No VM matching the machine-ID found on the provider %q", machineID)
return nil
}

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Oct 28, 2020
@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) 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 Oct 29, 2020
@MrBatschner
Copy link
Contributor Author

I guess we need to make sure that the port gets also deleted when the machine/server is already gone. Currently the port deletion logic is not executed when the machine is already deleted. We need to ensure that we check within each machine deletion flow for existing ports.

True, we just pushed a commit that puts the port delete logic into a separate function which will always get called - even if the VM no longer exists.

On another note:
This whole PR means that MCM will (for new shoots created with our proposed changes to the openstack-provider-extension) always go through the create port->create VM with port and the delete VM -> delete its port logic. The old code-path create VM and let OpenStack do the port stuff itself is still in the MCM but once the openstack-provider-extension starts providing the subnetID in the OpenStackMachineClass, this code will no longer be triggered.

I do not see a problem with this behaviour but what do you think about it?

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.

looks good to me.

/invite @kon-angelo

@kon-angelo Could you please have a look as well?
@kayrus Could you also please have a brief look if you have time :)

Copy link
Contributor

@prashanth26 prashanth26 left a comment

Choose a reason for hiding this comment

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

/lgtm
/needs second-opinion

@gardener-robot gardener-robot added needs/changes Needs (more) changes and removed needs/changes Needs (more) changes needs/second-opinion Needs second review by someone else labels Nov 2, 2020
@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 Nov 2, 2020
@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 Nov 2, 2020
Copy link
Contributor

@kon-angelo kon-angelo left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging needs/changes Needs (more) changes and removed needs/changes Needs (more) changes needs/second-opinion Needs second review by someone else reviewed/lgtm Has approval for merging labels Nov 2, 2020
@hardikdr
Copy link
Member

hardikdr commented Nov 3, 2020

cc @zuzzas

@hardikdr hardikdr 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 Nov 3, 2020
@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 Nov 3, 2020
@hardikdr
Copy link
Member

hardikdr commented Nov 3, 2020

Hm, earlier the integration test of freeze failed.

Check for freezing and unfreezing of machine-set due to rapid scaleUp and scaleDown	Failed: Freezing of machineSet failed. Exiting Test to avoid further conflicts.

And now the unit test related to the freeze failed:

Summarizing 1 Failure:
13:04:16

13:04:16
[Fail] #machine_safety ##reconcileClusterMachineSafetyOvershooting [It] Freeze a machineSet and its machineDeployment 
13:04:16
/tmp/build/a94a8fe5/pull-request-gardener_machine-controller-manager-pr_master/pkg/controller/machine_safety_test.go:247

Tests are flaky somehow.

@hardikdr
Copy link
Member

hardikdr commented Nov 3, 2020

@MrBatschner Can you please squash the commits?

@gardener-robot gardener-robot added the needs/second-opinion Needs second review by someone else label Nov 3, 2020
@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 Nov 3, 2020
@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 Nov 3, 2020
@MrBatschner
Copy link
Contributor Author

@hardikdr Done, squashed all 11 commits into one.

Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

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

/lgtm
Let's wait for a day, in case @kayrus is willing to take a look.

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging needs/changes Needs (more) changes and removed needs/changes Needs (more) changes needs/second-opinion Needs second review by someone else reviewed/lgtm Has approval for merging labels Nov 3, 2020
@hardikdr hardikdr merged commit e7e2069 into gardener:master Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change API change with impact on API users needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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.

OpenStack: add support for subnet specifying
9 participants