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

multi CPI support #52

Merged
merged 18 commits into from Dec 20, 2016
Merged

multi CPI support #52

merged 18 commits into from Dec 20, 2016

Conversation

MatthiasWinzeler
Copy link
Contributor

This PR adds support for the recently merged multi cpi feature in bosh. To support this, each CPI must respect configuration that might be provided dynamically in the context of each CPI call.

What's in the changeset:

  • The bosh_cpi and bosh_common were updated to the version that passes context to the cpi lambda
  • We intentionally merge (and not overwrite) the openstack properties from the director config with those provided in context, so that the default properties from the bosh release are taken into account (and the cpi config does not have to set them)
  • Refactoring the cpi lambda to its own class (to be able to spec it)

Unit tests are passing. Since I was not able to run the integration tests on our environment (we still use keystone v2), @voelzmo will provide a pipeline that runs them. Thanks!

* upstream/master: (39 commits)
  Revert `tags` replacement approach
  Pipeline setup enhancements
  New final release v27
  Update openstack api calls tracking
  Network ID is only checked if neutron is available
  Introduce build log retention
  Consider empty env vars as not set
  Don't use empty ca_cert var in lifecycle tests
  Use v3 variables as fallbacks
  Make BOSH_OPENSTACK_TENANT optional
  Introduce pipeline names
  Keystone V2 lifecycle tests optional
  Update openstack api calls tracking
  New final release v26
  Specifiy identity API version
  Update openstack api calls tracking
  Add tenant required for lifecycle V2 tests
  Make Keystone V3 default for testing and ...
  Make `boot_volume_type` configurable
  Update openstack api calls tracking
  ...
* master: (22 commits)
  Pipelines run now  with `debug_bats=true`
  Modularize terraform scripts
  Print tasks in error after BATs run
  Log all tasks in state error after BATs run
  CI: enable debug output for bosh-init delete
  Allow BATs debugging with one global parameter
  Switch compiled releases to use aws cli
  Allow for old naming of compiled releases
  Introduce 'distro' in pipeline tasks
  Use compiled bosh-release in CI for Ubuntu
  Add s3cmd to boshcpi/openstack-cpi-release docker image
  Add contributing information
  Update openstack api calls tracking
  Redact version property value
  Remove unused version property
  Support non-https URLs in service catalog
  Update openstack api calls tracking
  Disambiguate resource name
  Update openstack api calls tracking
  Enable email resource in pipelines
  ...
…at defaults from bosh release spec are respected
* master: (65 commits)
  Calculate SHA1 based on empty image file
  Update CONTRIBUTING.md
  Use terraform for setting up a lifecycle environment
  Refactor stemcell creation
  Add script for creating light stemcell
  Redact some properties from excon logs
  Revert "Redact some properties from excon logs"
  Redact some properties from excon logs
  Update openstack api calls tracking
  Write user_data after networks have been prepared
  Revert "Revert "Merge remote-tracking branch 'origin/wip-multiple-manual-networks'""
  Update openstack api calls tracking
  Revert "Merge remote-tracking branch 'origin/wip-multiple-manual-networks'"
  Disable `multiple_manual_networks` for bats run
  Update openstack api calls tracking
  Use simpler data structure to store networks in NetworkConfigurator
  Avoid neutron calls when `use_nova_networking=true`
  Don't check for networks if `use_nova_networking=false`
  CPI does not support multiple networks without neutron
  Introduce private network class
  ...
* master:
  Fix floating_ip reassociation
  Ensure files in stemcell tar don't start with './'
  Create terraform scripts for bats job environment setup
  Support light stemcell in stemcell deletion
  Support light stemcell in vm creation
  Don't retry to get image for vm creation
  Cleanup config value handling in CPI
  Rename image_uuid to image_id
  Add light stemcell creation
  e2e terraform scripts prepare environment per job
@cfdreddbot
Copy link

Hey MatthiasWinzeler!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/134693605

The labels on this github issue will be updated when the story is started.

@ljfranklin
Copy link
Contributor

@MatthiasWinzeler The BOSH team ended up removing the nested context.openstack key in favor of passing all keys at the top-level of context. Could you apply the following patch to your PR? Otherwise this PR looks in-line with the changes we've made in AWS and vSphere CPI, nice work! cc @voelzmo to merge

Patch:

diff --git a/src/bosh_openstack_cpi/lib/cloud/openstack/cpi_lambda.rb b/src/bosh_openstack_cpi/lib/cloud/openstack/cpi_lambda.rb
index 61bed34..70102bc 100644
--- a/src/bosh_openstack_cpi/lib/cloud/openstack/cpi_lambda.rb
+++ b/src/bosh_openstack_cpi/lib/cloud/openstack/cpi_lambda.rb
@@ -18,19 +18,17 @@ module Bosh::OpenStackCloud
         end
 
         # allow openstack config to be overwritten dynamically by context
-        if context['openstack']
-          cloud_properties['openstack'].merge!(context['openstack'])
+        cloud_properties['openstack'].merge!(context)
 
-          # write ca cert to disk if given in context
-          connection_options = cloud_properties['openstack']['connection_options']
-          if connection_options && (ca_cert = connection_options.delete('ca_cert'))
-            File.write(ca_cert_from_context, ca_cert)
-            connection_options['ssl_ca_file'] = ca_cert_from_context
-          end
+        # write ca cert to disk if given in context
+        connection_options = cloud_properties['openstack']['connection_options']
+        if connection_options && (ca_cert = connection_options.delete('ca_cert'))
+          File.write(ca_cert_from_context, ca_cert)
+          connection_options['ssl_ca_file'] = ca_cert_from_context
         end
 
         Bosh::Clouds::Openstack.new(cloud_properties)
       end
     end
   end
-end
\ No newline at end of file
+end
diff --git a/src/bosh_openstack_cpi/spec/unit/cpi_lambda_spec.rb b/src/bosh_openstack_cpi/spec/unit/cpi_lambda_spec.rb
index 8be4344..d7ae0a0 100644
--- a/src/bosh_openstack_cpi/spec/unit/cpi_lambda_spec.rb
+++ b/src/bosh_openstack_cpi/spec/unit/cpi_lambda_spec.rb
@@ -47,10 +47,8 @@ describe Bosh::OpenStackCloud::CpiLambda do
     context 'if openstack properties are provided in the context' do
       it 'merges the openstack properties' do
         context = {
-            'openstack' => {
-                'newkey' => 'newvalue',
-                'newkey2' => 'newvalue2',
-            }
+          'newkey' => 'newvalue',
+          'newkey2' => 'newvalue2',
         }
 
         expect(Bosh::Clouds::Openstack).to receive(:new).with({'openstack' => { 'key1' => 'value1',
@@ -65,10 +63,8 @@ describe Bosh::OpenStackCloud::CpiLambda do
         ca_file = Tempfile.new('ca_cert')
 
         context = {
-            'openstack' => {
-                'newkey' => 'newvalue',
-                'connection_options' => {'ca_cert' => 'xyz'}
-            }
+          'newkey' => 'newvalue',
+          'connection_options' => {'ca_cert' => 'xyz'}
         }
 
         expect(Bosh::Clouds::Openstack).to receive(:new).with({'openstack' => { 'newkey' => 'newvalue',
@@ -82,4 +78,4 @@ describe Bosh::OpenStackCloud::CpiLambda do
       end
     end
   end
-end
\ No newline at end of file
+end

@MatthiasWinzeler
Copy link
Contributor Author

@ljfranklin : Thanks for the input. I applied the patch.

@cppforlife
Copy link
Contributor

@voelzmo i think this PR is ready to be reviewed/merged by your team.

@voelzmo
Copy link
Contributor

voelzmo commented Nov 24, 2016

@MatthiasWinzeler @cppforlife @ljfranklin Thanks for the PR and review. We'll setup a pipeline and look at the PR soon.

MatthiasWinzeler and others added 2 commits December 2, 2016 13:56
* master:
  Don't return empty string as valid az for volumes
  Delete old terraform files
  Add terraform resources for bats dynamic and manual
  Harmonize variable names in shell scripts and terraform output
  BAT jobs in pipeline create their own resources
  Include user input in error message
  Don't include backtrace in error messages
  Use image id from director manifest state
  Print cmd to create light stemcell and image id
  New final release v28
  Remove unused resource in e2e pipeline
  Add ent-to-end test for light stecells
Signed-off-by: Cornelius Schumacher <cschum@suse.com>
mauromorales and others added 2 commits December 13, 2016 16:30
Signed-off-by: Tom Kiemes <tom.kiemes@sap.com>
Vcap user is not allowed to write to
`/var/vcap/jobs/openstack_cpi/config/cacert_context.pem`.
Instead of using this fixed path, a tmpdir is created for each cpi
call. The lambda writes the cacert into this tmpdir. The tmpdir is
deleted after the cpi call has finished.

[#134693605](https://www.pivotaltracker.com/story/show/134693605)

Signed-off-by: Jan von Loewenstein <jan.von.loewenstein@sap.com>
@friegger friegger merged commit 41ded96 into cloudfoundry:master Dec 20, 2016
@friegger
Copy link
Contributor

@MatthiasWinzeler Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants