From ea15d173000ce42beda0445cf190c9510fa0f5e0 Mon Sep 17 00:00:00 2001 From: Tom Kennedy Date: Thu, 26 Mar 2026 11:04:18 -0400 Subject: [PATCH] Install buildpacks based on order on config - Account for existing system buildpacks by offsetting the install position - Skip index for buildpacks with explicit position in config resolves #3798 --- .../runtime/buildpack_installer_factory.rb | 3 +- .../runtime/create_buildpack_installer.rb | 8 +++ lib/cloud_controller/install_buildpacks.rb | 72 ++++++++++++------- .../buildpack_installer_factory_spec.rb | 49 ++++++++----- .../create_buildpack_installer_spec.rb | 38 ++++++++++ .../install_buildpacks_spec.rb | 60 ++++++++++++++-- 6 files changed, 180 insertions(+), 50 deletions(-) diff --git a/app/jobs/runtime/buildpack_installer_factory.rb b/app/jobs/runtime/buildpack_installer_factory.rb index fd03d1ef91b..01643347ddc 100644 --- a/app/jobs/runtime/buildpack_installer_factory.rb +++ b/app/jobs/runtime/buildpack_installer_factory.rb @@ -56,7 +56,8 @@ def plan(buildpack_name, manifest_fields) name: buildpack_name, stack: buildpack_fields[:stack], file: buildpack_fields[:file], - options: buildpack_fields[:options] + options: buildpack_fields[:options], + config_index: buildpack_fields[:config_index] }) end end diff --git a/app/jobs/runtime/create_buildpack_installer.rb b/app/jobs/runtime/create_buildpack_installer.rb index 7d29130a9d1..d0930eb973a 100644 --- a/app/jobs/runtime/create_buildpack_installer.rb +++ b/app/jobs/runtime/create_buildpack_installer.rb @@ -2,6 +2,13 @@ module VCAP::CloudController module Jobs module Runtime class CreateBuildpackInstaller < BuildpackInstaller + attr_accessor :config_index + + def initialize(job_options) + super + @config_index = job_options[:config_index] + end + def perform logger.info "Installing buildpack name `#{name}' with stack `#{stack_name}'" @@ -11,6 +18,7 @@ def perform buildpacks_lock.db.transaction do buildpacks_lock.lock! buildpack = Buildpack.create(name: name, stack: stack_name, lifecycle: options[:lifecycle]) + buildpack.move_to(config_index + 1) if !config_index.nil? && !options.key?(:position) end begin buildpack_uploader.upload_buildpack(buildpack, file, File.basename(file)) diff --git a/lib/cloud_controller/install_buildpacks.rb b/lib/cloud_controller/install_buildpacks.rb index fa356d2a051..c4725fb4f2b 100644 --- a/lib/cloud_controller/install_buildpacks.rb +++ b/lib/cloud_controller/install_buildpacks.rb @@ -12,34 +12,15 @@ def install(buildpacks) CloudController::DependencyLocator.instance.buildpack_blobstore.ensure_bucket_exists job_factory = VCAP::CloudController::Jobs::Runtime::BuildpackInstallerFactory.new + existing_count_by_lifecycle = { + Lifecycles::BUILDPACK => Buildpack.where(lifecycle: Lifecycles::BUILDPACK).count, + Lifecycles::CNB => Buildpack.where(lifecycle: Lifecycles::CNB).count + } + factory_options = [] buildpacks.each do |bpack| - buildpack_opts = bpack.deep_symbolize_keys - - buildpack_opts[:lifecycle] = Lifecycles::BUILDPACK if buildpack_opts[:lifecycle].nil? - - buildpack_name = buildpack_opts.delete(:name) - if buildpack_name.nil? - logger.error "A name must be specified for the buildpack_opts: #{buildpack_opts}" - next - end - - package = buildpack_opts.delete(:package) - buildpack_file = buildpack_opts.delete(:file) - if package.nil? && buildpack_file.nil? - logger.error "A package or file must be specified for the buildpack_opts: #{bpack}" - next - end - - buildpack_file = buildpack_zip(package, buildpack_file) - if buildpack_file.nil? - logger.error "No file found for the buildpack_opts: #{bpack}" - next - elsif !File.file?(buildpack_file) - logger.error "File not found: #{buildpack_file}, for the buildpack_opts: #{bpack}" - next - end - factory_options << { name: buildpack_name, file: buildpack_file, options: buildpack_opts, stack: detected_stack(buildpack_file, buildpack_opts) } + opts = buildpack_factory_options(bpack, existing_count_by_lifecycle, factory_options) + factory_options << opts if opts end buildpack_install_jobs = generate_install_jobs(factory_options, job_factory) @@ -55,6 +36,45 @@ def logger private + def buildpack_factory_options(bpack, existing_count_by_lifecycle, factory_options) + buildpack_opts = bpack.deep_symbolize_keys + buildpack_opts[:lifecycle] = Lifecycles::BUILDPACK if buildpack_opts[:lifecycle].nil? + + buildpack_name = buildpack_opts.delete(:name) + if buildpack_name.nil? + logger.error "A name must be specified for the buildpack_opts: #{buildpack_opts}" + return + end + + package = buildpack_opts.delete(:package) + buildpack_file = buildpack_opts.delete(:file) + if package.nil? && buildpack_file.nil? + logger.error "A package or file must be specified for the buildpack_opts: #{bpack}" + return + end + + buildpack_file = buildpack_zip(package, buildpack_file) + if buildpack_file.nil? + logger.error "No file found for the buildpack_opts: #{bpack}" + return + elsif !File.file?(buildpack_file) + logger.error "File not found: #{buildpack_file}, for the buildpack_opts: #{bpack}" + return + end + + lifecycle = buildpack_opts[:lifecycle] + existing_offset = existing_count_by_lifecycle[lifecycle] || 0 + # exclude counting buildpacks with position because they don't need a config index + unless buildpack_opts.key?(:position) + config_index = existing_offset + factory_options.count do |opts| + opts[:options][:lifecycle] == lifecycle && !opts[:options].key?(:position) + end + end + + { name: buildpack_name, file: buildpack_file, options: buildpack_opts, stack: detected_stack(buildpack_file, buildpack_opts), + config_index: config_index } + end + def generate_install_jobs(factory_options, job_factory) buildpack_install_jobs = [] buildpacks_by_lifecycle = factory_options.group_by { |options| options[:options][:lifecycle] } diff --git a/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb b/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb index 144b640f442..077f5bf30f6 100644 --- a/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb +++ b/spec/unit/jobs/runtime/buildpack_installer_factory_spec.rb @@ -15,7 +15,7 @@ module Jobs::Runtime end context 'when the manifest has one buildpack' do - let(:buildpack_fields) { [{ file: file, options: opts }] } + let(:buildpack_fields) { [{ file: file, options: opts, config_index: 0 }] } let(:single_buildpack_job) { jobs.first } shared_examples_for 'passthrough parameters' do @@ -34,7 +34,7 @@ module Jobs::Runtime context 'when there is no matching buildpack record by name' do context 'and there is a detected stack in the zipfile' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: 'detected stack' }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: 'detected stack', config_index: 0 }] } include_examples 'passthrough parameters' @@ -45,6 +45,18 @@ module Jobs::Runtime it 'sets the stack to the detected stack' do expect(single_buildpack_job.stack_name).to eq('detected stack') end + + it 'passes config_index to the create job' do + expect(single_buildpack_job.config_index).to eq(0) + end + + context 'with a specific config_index' do + let(:buildpack_fields) { [{ file: file, options: opts, stack: 'detected stack', config_index: 3 }] } + + it 'passes the config_index through' do + expect(single_buildpack_job.config_index).to eq(3) + end + end end context 'and there is not a detected stack in the zipfile' do @@ -66,7 +78,7 @@ module Jobs::Runtime let!(:existing_buildpack) { Buildpack.make(name: name, stack: existing_stack.name, key: 'new_key', guid: 'the-guid') } context 'and the buildpack zip has the same stack' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name, config_index: 0 }] } include_examples 'passthrough parameters' @@ -84,7 +96,7 @@ module Jobs::Runtime end context 'and the buildpack zip has a different stack' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack' }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack', config_index: 0 }] } include_examples 'passthrough parameters' @@ -98,7 +110,7 @@ module Jobs::Runtime end context 'and the manifest stack is nil' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }] } it 'errors' do expect do @@ -112,7 +124,7 @@ module Jobs::Runtime let!(:existing_buildpack) { Buildpack.make(name: name, stack: nil, key: 'new_key', guid: 'the-guid') } context 'and the buildpack zip also has a nil stack' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }] } include_examples 'passthrough parameters' @@ -130,7 +142,7 @@ module Jobs::Runtime end context 'but the buildpack zip /has/ a stack' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack' }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack', config_index: 0 }] } include_examples 'passthrough parameters' @@ -157,7 +169,7 @@ module Jobs::Runtime let!(:another_existing_buildpack) { Buildpack.make(name: name, stack: another_existing_stack.name, key: 'new_key', guid: 'another-guid') } context 'and one matches the manifest stack' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name, config_index: 0 }] } include_examples 'passthrough parameters' @@ -175,7 +187,7 @@ module Jobs::Runtime end context 'and the manifest stack is nil' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }] } it 'errors' do expect do @@ -185,7 +197,7 @@ module Jobs::Runtime end context 'and none match the manifest stack' do - let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack' }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: 'manifest stack', config_index: 0 }] } include_examples 'passthrough parameters' @@ -202,7 +214,9 @@ module Jobs::Runtime context 'when the manifest has multiple buildpack entries for one name, with different stacks' do let(:another_file) { 'the-other-file' } - let(:buildpack_fields) { [{ file: file, options: opts, stack: 'existing stack' }, { file: another_file, options: opts, stack: 'manifest stack' }] } + let(:buildpack_fields) do + [{ file: file, options: opts, stack: 'existing stack', config_index: 0 }, { file: another_file, options: opts, stack: 'manifest stack', config_index: 1 }] + end context 'and there are no matching Buildpacks' do it 'plans to create all the Buildpacks' do @@ -277,7 +291,10 @@ module Jobs::Runtime let(:another_existing_stack) { Stack.make(name: 'another existing stack') } let!(:another_existing_buildpack) { Buildpack.make(name: name, stack: another_existing_stack.name, key: 'a_different_key', guid: 'a-different-guid') } - let(:buildpack_fields) { [{ file: file, options: opts, stack: existing_stack.name }, { file: another_file, options: opts, stack: another_existing_stack.name }] } + let(:buildpack_fields) do + [{ file: file, options: opts, stack: existing_stack.name, config_index: 0 }, + { file: another_file, options: opts, stack: another_existing_stack.name, config_index: 1 }] + end context 'and none of them has a nil stack' do it 'creates a job for each buildpack' do @@ -297,7 +314,7 @@ module Jobs::Runtime end context 'when one of them is stackless' do - let(:buildpack_fields) { [{ file: file, options: opts }] } + let(:buildpack_fields) { [{ file: file, options: opts, config_index: 0 }] } before do Stack.make(name: 'existing stack') @@ -316,7 +333,7 @@ module Jobs::Runtime context 'when the manifest has multiple buildpack entries for one name, with the same stack' do let(:another_file) { 'the-other-file' } - let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack' }, { file: another_file, options: opts, stack: 'stack' }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack', config_index: 0 }, { file: another_file, options: opts, stack: 'stack', config_index: 1 }] } it 'raises a DuplicateInstall error' do expect do @@ -327,7 +344,7 @@ module Jobs::Runtime context 'when the manifest has multiple buildpack entries for one name, neither specifying a stack' do let(:another_file) { 'the-other-file' } - let(:buildpack_fields) { [{ file: file, options: opts, stack: nil }, { file: another_file, options: opts, stack: nil }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: nil, config_index: 0 }, { file: another_file, options: opts, stack: nil, config_index: 1 }] } it 'raises a DuplicateInstall error' do expect do @@ -338,7 +355,7 @@ module Jobs::Runtime context 'when the manifest has multiple buildpack entries for one name, one stackful and one stackless' do let(:another_file) { 'the-other-file' } - let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack' }, { file: another_file, options: opts, stack: nil }] } + let(:buildpack_fields) { [{ file: file, options: opts, stack: 'stack', config_index: 0 }, { file: another_file, options: opts, stack: nil, config_index: 1 }] } it 'raises a StacklessBuildpackIncompatibilityError error' do expect do diff --git a/spec/unit/jobs/runtime/create_buildpack_installer_spec.rb b/spec/unit/jobs/runtime/create_buildpack_installer_spec.rb index b5463818764..a08300c85c2 100644 --- a/spec/unit/jobs/runtime/create_buildpack_installer_spec.rb +++ b/spec/unit/jobs/runtime/create_buildpack_installer_spec.rb @@ -78,6 +78,44 @@ module Jobs::Runtime end end + context 'when config_index is provided' do + let!(:existing_stack) { Stack.make(name: stack_name) } + let!(:existing_buildpack1) { Buildpack.make(name: 'first_buildpack', stack: stack_name) } + let!(:existing_buildpack2) { Buildpack.make(name: 'second_buildpack', stack: stack_name) } + let!(:existing_buildpack3) { Buildpack.make(name: 'third_buildpack', stack: stack_name) } + let!(:existing_buildpack4) { Buildpack.make(name: 'fourth_buildpack', stack: stack_name) } + let!(:existing_buildpack5) { Buildpack.make(name: 'fifth_buildpack', stack: stack_name) } + let(:job_options) { { name: 'mybuildpack', stack: stack_name, file: zipfile, options: { enabled: true, locked: false }, config_index: 5 } } + + it 'moves the buildpack to position config_index + 1' do + job.perform + buildpack = Buildpack.find(name: 'mybuildpack') + expect(buildpack.position).to eq(6) + end + end + + context 'when config_index is provided but options includes an explicit position' do + let!(:existing_stack) { Stack.make(name: stack_name) } + let(:job_options) { { name: 'mybuildpack', stack: stack_name, file: zipfile, options: { enabled: true, locked: false, position: 7 }, config_index: 5 } } + + it 'uses the explicit position from options instead of config_index' do + job.perform + buildpack = Buildpack.find(name: 'mybuildpack') + expect(buildpack.position).to eq(7) + end + end + + context 'when config_index is nil' do + let!(:existing_stack) { Stack.make(name: stack_name) } + let(:job_options) { { name: 'mybuildpack', stack: stack_name, file: zipfile, options: { enabled: true, locked: false } } } + + it 'does not call move_to and uses default list position' do + job.perform + buildpack = Buildpack.find(name: 'mybuildpack') + expect(buildpack.position).to eq(1) + end + end + context 'when uploading the buildpack fails' do let!(:existing_stack) { Stack.make(name: stack_name) } diff --git a/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb b/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb index 932edeb18a4..95ae83fd350 100644 --- a/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb +++ b/spec/unit/lib/cloud_controller/install_buildpacks_spec.rb @@ -43,7 +43,7 @@ module VCAP::CloudController context 'when a cnb buildpack is specified' do let(:file) { 'buildpack.cnb' } let(:buildpack_fields) do - { name: 'cnb_buildpack', file: file, stack: nil, options: { lifecycle: Lifecycles::CNB } } + { name: 'cnb_buildpack', file: file, stack: nil, options: { lifecycle: Lifecycles::CNB }, config_index: 0 } end let(:install_buildpack_config) do @@ -77,7 +77,7 @@ module VCAP::CloudController context 'when a cnb buildpack is specified with .tgz extension' do let(:file) { 'buildpack.tgz' } let(:buildpack_fields) do - { name: 'cnb_buildpack', file: file, stack: nil, options: { lifecycle: Lifecycles::CNB } } + { name: 'cnb_buildpack', file: file, stack: nil, options: { lifecycle: Lifecycles::CNB }, config_index: 0 } end let(:install_buildpack_config) do @@ -114,13 +114,13 @@ module VCAP::CloudController let(:buildpack2_file) { 'abuildpack2.zip' } let(:buildpack1a_fields) do - { name: 'buildpack1', file: buildpack1a_file, stack: 'cflinuxfs11', options: { lifecycle: Lifecycles::BUILDPACK } } + { name: 'buildpack1', file: buildpack1a_file, stack: 'cflinuxfs11', options: { lifecycle: Lifecycles::BUILDPACK }, config_index: 0 } end let(:buildpack1b_fields) do - { name: 'buildpack1', file: buildpack1b_file, stack: 'cflinuxfs12', options: { lifecycle: Lifecycles::BUILDPACK } } + { name: 'buildpack1', file: buildpack1b_file, stack: 'cflinuxfs12', options: { lifecycle: Lifecycles::BUILDPACK }, config_index: 1 } end let(:buildpack2_fields) do - { name: 'buildpack2', file: buildpack2_file, stack: nil, options: { lifecycle: Lifecycles::BUILDPACK } } + { name: 'buildpack2', file: buildpack2_file, stack: nil, options: { lifecycle: Lifecycles::BUILDPACK }, config_index: 2 } end before do @@ -221,7 +221,8 @@ module VCAP::CloudController # call install # verify that job_factory.plan was called with the right file expect(File).to receive(:file?).with('another.zip').and_return(true) - expect(job_factory).to receive(:plan).with('buildpack1', [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: { lifecycle: Lifecycles::BUILDPACK } }]) + expect(job_factory).to receive(:plan).with('buildpack1', + [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: { lifecycle: Lifecycles::BUILDPACK }, config_index: 0 }]) installer.install(TestConfig.config_instance.get(:install_buildpacks)) end @@ -235,7 +236,8 @@ module VCAP::CloudController it 'succeeds when no package is specified' do TestConfig.config[:install_buildpacks][0].delete('package') expect(File).to receive(:file?).with('another.zip').and_return(true) - expect(job_factory).to receive(:plan).with('buildpack1', [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: { lifecycle: Lifecycles::BUILDPACK } }]) + expect(job_factory).to receive(:plan).with('buildpack1', + [{ name: 'buildpack1', file: 'another.zip', stack: nil, options: { lifecycle: Lifecycles::BUILDPACK }, config_index: 0 }]) installer.install(TestConfig.config_instance.get(:install_buildpacks)) end @@ -291,6 +293,7 @@ module VCAP::CloudController [{ name: 'buildpack1', file: 'abuildpack.zip', stack: nil, + config_index: nil, options: { enabled: true, lifecycle: Lifecycles::BUILDPACK, @@ -302,6 +305,49 @@ module VCAP::CloudController expect(installer.logger).not_to have_received(:error) end + + context 'when buildpacks mix explicit positions and sequential positions' do + let(:bp1_file) { 'buildpack1.zip' } + let(:bp2_file) { 'buildpack2.zip' } + let(:bp3_file) { 'buildpack3.zip' } + + let(:install_buildpack_config) do + { + install_buildpacks: [ + { 'name' => 'buildpack1', 'package' => 'pkg1' }, + { 'name' => 'buildpack2', 'package' => 'pkg2', 'position' => 5 }, + { 'name' => 'buildpack3', 'package' => 'pkg3' } + ] + } + end + + before do + allow(Dir).to receive(:[]).with('/var/vcap/packages/pkg1/*[.zip|.cnb|.tgz|.tar.gz]').and_return([bp1_file]) + allow(File).to receive(:file?).with(bp1_file).and_return(true) + allow(Dir).to receive(:[]).with('/var/vcap/packages/pkg2/*[.zip|.cnb|.tgz|.tar.gz]').and_return([bp2_file]) + allow(File).to receive(:file?).with(bp2_file).and_return(true) + allow(Dir).to receive(:[]).with('/var/vcap/packages/pkg3/*[.zip|.cnb|.tgz|.tar.gz]').and_return([bp3_file]) + allow(File).to receive(:file?).with(bp3_file).and_return(true) + + allow(job_factory).to receive(:plan).and_return([canary_job]) + allow(Jobs::Enqueuer).to receive(:new).and_return(enqueuer) + allow(enqueuer).to receive(:enqueue) + end + + it 'assigns nil config_index to positioned buildpacks and sequential indices to others' do + installer.install(TestConfig.config_instance.get(:install_buildpacks)) + + expect(job_factory).to have_received(:plan).with('buildpack1', [ + { name: 'buildpack1', file: bp1_file, stack: nil, options: { lifecycle: Lifecycles::BUILDPACK }, config_index: 0 } + ]) + expect(job_factory).to have_received(:plan).with('buildpack2', [ + { name: 'buildpack2', file: bp2_file, stack: nil, options: { lifecycle: Lifecycles::BUILDPACK, position: 5 }, config_index: nil } + ]) + expect(job_factory).to have_received(:plan).with('buildpack3', [ + { name: 'buildpack3', file: bp3_file, stack: nil, options: { lifecycle: Lifecycles::BUILDPACK }, config_index: 1 } + ]) + end + end end end end