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

Support multiple networks in OpenStack driver #376

Merged
merged 1 commit into from Dec 23, 2019
Merged

Support multiple networks in OpenStack driver #376

merged 1 commit into from Dec 23, 2019

Conversation

zuzzas
Copy link
Contributor

@zuzzas zuzzas commented Dec 16, 2019

What this PR does / why we need it:

This PR introduces a new field to the OpenstackMachineClassSpec, which allows an operator to define multiple networks to be attached to a VM. New implementation coexists with the previous one and does not require an API version bump.

Release note:

Allows specifying multiple networks for a VM via the new OpenstackMachineClassSpec "networks" field

@zuzzas zuzzas requested review from ggaurav10 and a team as code owners December 16, 2019 09:45
@prashanth26 prashanth26 added kind/enhancement Enhancement, improvement, extension platform/openstack OpenStack platform/infrastructure labels Dec 17, 2019
@prashanth26
Copy link
Contributor

Hi guys,

Can you PTAL at these changes on OpenStack? @afritzler @mandelsoft @MartinWeindel

afritzler
afritzler approved these changes Dec 17, 2019
View reviewed changes
Copy link
Member

@afritzler afritzler left a comment

Choose a reason for hiding this comment

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

/lgtm & thanks a lot for this PR

@zuzzas can you elaborate a little bin on your use case for this PR. As we are not using multiple networks for our VMs it would be interesting to learn how you guys are using the MCM.

afritzler
afritzler suggested changes Dec 17, 2019
View reviewed changes
go.mod Outdated
@@ -26,7 +26,7 @@ require (
github.com/google/go-cmp v0.2.0 // indirect
github.com/google/gofuzz v0.0.0-20170612174753-24818f796faf // indirect
github.com/googleapis/gnostic v0.2.0 // indirect
github.com/gophercloud/gophercloud v0.0.0-20190212181753-892256c46858
github.com/gophercloud/gophercloud v0.4.0
Copy link
Member

Choose a reason for hiding this comment

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

Can we bump this to a newer version?

Copy link
Member

Choose a reason for hiding this comment

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

e.g v0.7.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is done.

@zuzzas zuzzas force-pushed the openstack-multiple-networks branch 2 times, most recently from ed00bc4 to cc6ea33 Compare December 17, 2019 12:48
@zuzzas
Copy link
Contributor Author

zuzzas commented Dec 17, 2019

@afritzler
We aren't operating in a homogenous environment. Hence, my attempts at tailoring MCM to a wide variety of use cases and underlying cloud platform configurations.

Here are two common use cases:

  1. We may have a separate network that has a database instance inside of it, as a form of simple access control.
  2. Shared networks disallow setting allowed address pairs or disabling security altogether, which impairs pod network routing via a non-overlayed network.

By having a list of networks to attach to a VM, each with options, we can satisfy those cases and so much more.

@prashanth26 prashanth26 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 Dec 18, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 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 Dec 18, 2019
@prashanth26
Copy link
Contributor

There are lint checks failing. Can you please fix them?

+ pull-request-gardener_machine-controller-manager-pr_master/.ci/check
Running go vet...
Running gofmt...
pkg/controller/drain.go
pkg/controller/drain_test.go
Running golint...
./pkg/driver/driver_openstack.go:109:8: var resolvedNetworkId should be resolvedNetworkID
Found 1 lint suggestions; failing.

@zuzzas
Support multiple networks in OpenStack driver
f5f2a03 
Signed-off-by: Andrey Klimentyev <andrey.klimentyev@flant.com>
@zuzzas
Copy link
Contributor Author

zuzzas commented Dec 19, 2019

@prashanth26
Done.

@prashanth26 prashanth26 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 needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 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 Dec 19, 2019
@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 Dec 23, 2019
@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 Dec 23, 2019
@hardikdr
Copy link
Member

@zuzzas thanks for the PR. CI seems to be flaking, I have re-triggered it, once tests are passing I shall merge it.

@hardikdr hardikdr merged commit e31f136 into gardener:master Dec 23, 2019
kayrus
kayrus reviewed Feb 14, 2020
View reviewed changes
pkg/driver/driver_openstack.go

nwClient, err := d.createNeutronClient()
if err != nil {
return "", "", d.deleteOnFail(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

It must not be used here, since d.MachineID is not set here.
/cc @hardikdr

@zuzzas zuzzas deleted the openstack-multiple-networks branch February 14, 2020 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kayrus kayrus kayrus left review comments

@afritzler afritzler afritzler requested changes

@hardikdr hardikdr hardikdr approved these changes

@ggaurav10 ggaurav10 Awaiting requested review from ggaurav10

@mandelsoft mandelsoft Awaiting requested review from mandelsoft

@MartinWeindel MartinWeindel Awaiting requested review from MartinWeindel

Assignees
No one assigned
Labels
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) platform/openstack OpenStack platform/infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@zuzzas @prashanth26 @hardikdr @afritzler @kayrus @gardener-robot-ci-2 @gardener-robot-ci-3