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] Config drive option #457

Merged
merged 2 commits into from
Jul 1, 2020
Merged

[openstack] Config drive option #457

merged 2 commits into from
Jul 1, 2020

Conversation

zuzzas
Copy link
Contributor

@zuzzas zuzzas commented Apr 29, 2020 •
edited
Loading

What this PR does / why we need it:

Add an option to use config drive while provisioning OpenStack Servers.

Release note:

Added an option to use configDrive in the OpenStackMachineClass

@zuzzas zuzzas requested review from ggaurav10 and a team as code owners April 29, 2020 08:11
@hardikdr
Copy link
Member

Thanks for the PR @zuzzas.

@hardikdr
Copy link
Member

@afritzler @kayrus @dguendisch Would be really nice if you could take a brief look if possible :)

afritzler
afritzler suggested changes May 4, 2020
View reviewed changes
Copy link
Member

@afritzler afritzler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

How about still having an explicit and optional switch in the machineclass to enable config drive support? I like auto detect method - however having an explicit way to configure it would be also nice.

pkg/driver/driver_openstack.go Outdated Show resolved Hide resolved
@afritzler
Copy link
Member

One other reason for explicit support in the OpenStackMachineClass: we had in the past the situation where the dhcp interfaces where present in the subnet. However they became unresponsive. We already thought back where it would be good to have the ConfigDrive support as an alternative available, but didn't follow through.

@zuzzas zuzzas force-pushed the upstreaming-openstack-config-drive branch from dc76d79 to d400e0e Compare June 8, 2020 10:13
@zuzzas
Copy link
Contributor Author

zuzzas commented Jun 8, 2020

@afritzler
I've removed all the auto-detection logic and switched to a simple config option. It was nice, but it relied on some assumptions (as pointed by #457 (comment)) that can clearly break user's expectations.

zuzzas
zuzzas commented Jun 8, 2020
View reviewed changes
pkg/driver/driver_openstack.go
"github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
"github.com/gardener/machine-controller-manager/pkg/metrics"
"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done by goimports.

@zuzzas zuzzas changed the title [openstack] Config drive option auto-detection [openstack] Config drive option Jun 8, 2020
@hardikdr
Copy link
Member

@afritzler It Would be nice if you could take a look again.

@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 Jun 11, 2020
@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 Jun 11, 2020
@hardikdr
Copy link
Member

There seems to be a minor file-conflict, can you please take a look @zuzzas .
If there are no more objections, I'll take a final look and prefer to merge it soon.

zuzzas added 2 commits June 17, 2020 13:32
@zuzzas
[openstack] Config drive auto-detection
1bac3ae 
Signed-off-by: Andrey Klimentyev <andrey.klimentyev@flant.com>
@zuzzas
rebase and API re-generation
ec82f8b
@zuzzas zuzzas force-pushed the upstreaming-openstack-config-drive branch from d400e0e to ec82f8b Compare June 17, 2020 10:37
@zuzzas
Copy link
Contributor Author

zuzzas commented Jun 17, 2020

@hardikdr, it is done.

@hardikdr
Copy link
Member

@kayrus @afritzler I would be nice if you could take a look and approve if possible :)

@afritzler
Copy link
Member

Let me test that against our environment and get back to you by Monday. Pretty swamped atm.

@prashanth26
Copy link
Contributor

Hi @afritzler ,

Did you get a chance to test these changes?

@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 Jun 29, 2020
@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 Jun 29, 2020
@hardikdr
Copy link
Member

cc @dkistner @MartinWeindel

MartinWeindel
MartinWeindel approved these changes Jul 1, 2020
View reviewed changes
Copy link
Member

@MartinWeindel MartinWeindel 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 the reviewed/lgtm Has approval for merging label Jul 1, 2020
@MartinWeindel
Copy link
Member

@zuzzas Can use adjust the PR description to reflect the latest state? There is still described that automatic detection is used, but it's not in the code anymore.

@afritzler
Copy link
Member

Ok, I finally also came around to test this PR. Creating machines without and with config drive support

useConfigDrive: true

in the MachineClass is working on our OpenStack environment.

/lgtm

dkistner
dkistner approved these changes Jul 1, 2020
View reviewed changes
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.

/lgtm

@zuzzas
Copy link
Contributor Author

zuzzas commented Jul 1, 2020

@zuzzas Can use adjust the PR description to reflect the latest state? There is still described that automatic detection is used, but it's not in the code anymore.

Done.

@hardikdr
Copy link
Member

hardikdr commented Jul 1, 2020

Thanks, everyone. I will go ahead merging then.

hardikdr
hardikdr approved these changes Jul 1, 2020
View reviewed changes
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

@hardikdr hardikdr merged commit 721a07c into gardener:master Jul 1, 2020
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

@dkistner dkistner dkistner approved these changes

@MartinWeindel MartinWeindel MartinWeindel approved these changes

@ggaurav10 ggaurav10 Awaiting requested review from ggaurav10

@dguendisch dguendisch Awaiting requested review from dguendisch

Assignees
No one assigned
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@zuzzas @hardikdr @afritzler @prashanth26 @MartinWeindel @kayrus @dkistner @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot