ERB template rendering code review: rename old bosh v1 names to v2

[bgandon]

What is this change about?

Here, I’ve reviewed the ERB template rendering code in order to document it in the cloudfoundry/docs-bosh#813 PR.

In that part of the Director’s code, I’ve noticed many misleading use of the term “template”, used instead of “job”. And this makes the code very confusing because “template” could also refer to “ERB template”.

So, I’ve started chasing these names that had not been migrated back in 2017 (when Bosh v1→v2 refactoring effort had been conducted), and I’ve looked for more suitable names. Don’t hesitate to challenge these, by the way.

Then I’ve updated the unit tests. Here also I’ve tried to clarify some ambiguous namings.

In that process, I’ve noticed that when rendering ERB templates, the job name was read from job.MF, freshly extracted from the job blob, whereas the name already persisted in database. I’ve removed the unnecessary unmarshalling of job.MF and added a name verification at release-upload time.

The modifications in this PR are quite large overall, but I’ve paid great attention in separating this into smaller chunks of work in to the individual commits.

Please provide contextual information.

This work was done in order to gather information for authoring cloudfoundry/docs-bosh#813.

What tests have you run against this PR?

Successful run of these test suites:

bundle exec rake spec:unit:parallel

DB=sqlite     bundle exec rake spec:unit:director
DB=postgresql bundle exec rake spec:unit:director
DB=mysql      bundle exec rake spec:unit:director

How should this change be described in bosh release notes?

This change brings no new feature, and only more clarity in the ERB rendering code.

Does this PR introduce a breaking change?

Definitely not.

Tag your pair, your PM, and/or team!

I’m a poor lonesome cowboy 😄

[rkoster]

@bgandon did you get a change yet to look at the changes requested by Aram?

[bgandon]

Sorry for the delay @rkoster, I’ve answered the comments indeed.

Thank you @aramprice for the very valuable review. You’ve pointed out choices that definitely needed some details indeed 👍

I’ve updated the typos, modified the names for fake jobs, and rebased the code on top of the main branch.

Please tell me what you think of the choices that I’ve detailed in my answers to your comments, thank you!

[aramprice]

thanks for the improvements!

[bgandon]

Due to the large diff, we are waiting for a second review, thanks for anyone that can do it!

[jpalermo]

👍

A lot of changes, but functionally seems to be the same. A lot of great cleanup though.

Thanks!

[rkoster]

Thanks! @bgandon

[aramprice]

NOTE: leaving this comment here as a breadcrumb for folks who might encounter similar breakage in the future.

This change appears to have exposed a problem with src/spec/assets/valid_release.tgz.

Integration tests which attempt to deploy this release using a command similar to:

bosh [redacted args] upload-release src/spec/assets/valid_release.tgz

Fail with the following error:

Using environment 'https://127.0.0.1:62404' as client 'test'
[-----------------------------------------------------------------] 100.00%   0s
Task 6

Task 6 | 17:40:22 | Extracting release: Extracting release (00:00:00)
Task 6 | 17:40:22 | Verifying manifest: Verifying manifest (00:00:00)
Task 6 | 17:40:22 | Resolving package dependencies: Resolving package dependencies (00:00:00)
Task 6 | 17:40:22 | Creating new packages: stuff/0.1.17 (00:00:01)
Task 6 | 17:40:23 | Creating new packages: mutator/2.99.7 (00:00:00)
Task 6 | 17:40:23 | Creating new jobs: cacher/1a (00:00:00)
                 L Error: Inconsistent name for job 'cacher'(exptected: 'cacher', got: '')
Task 6 | 17:40:23 | Error: Inconsistent name for job 'cacher'(exptected: 'cacher', got: '')

Task 6 Started  Thu Jan 11 17:40:22 UTC 2024
Task 6 Finished Thu Jan 11 17:40:23 UTC 2024
Task 6 Duration 00:00:01
Task 6 error

This is being run from spec/integration/release/export_release_spec.rb:222

[beyhan]

I didn't have time to get to the bottom of this but it looks like there is a new validation now

bosh/src/bosh-director/lib/bosh/director/jobs/release/release_job.rb

Lines 79 to 84 in dcb90ac

	def validate_name(job_manifest)
	unless name == job_manifest['name']
	raise JobInvalidName, "Inconsistent name for job '#{name}'" +
	"(exptected: '#{name}', got: '#{job_manifest['name']}')"
	end
	end

Actually this change shouldn't have any behaviour changes. @bgandon why that check was added?

[aramprice]

@beyhan - nice find! No need to dig in I only figured this out at the end of my yesterday.

It's likely that src/spec/assets/valid_release.tgz is making some "bosh v1" assumptions.

[aramprice]

Hopefully fixed with e42f767

[beyhan]

@aramprice thanks for the heads-up. I also saw that the last update of that release was way back in 2015 and had an old structure. Is the support for "bosh v1" removed with this refactoring or it was already the case and the validation here just shows it? If it is with this pr I would suggest to have this in the release notes. FYI @rkoster, @jpalermo

[jpalermo]

I think v1 manifests should still work, I don't think release job spec file structure changed much between v1 and v2, and I would have thought it was "required" that jobs in v1 also had a name that matched their actual name.

I kinda bet something blows up at some point if this isn't true, so I don't think this is really a change in behavior, probably just surfaces the error sooner. (could be wrong about all of this though)