Issue 644 Support for multiple AGWs/LBs and named backend pools

[Justin-W]

• Please check this box and fill the data as below once you have run the unit tests.
  Code coverage should keeps at 100%.

  pushd src/bosh_azure_cpi
    ./bin/test-unit
  popd
  ...
  Finished in 5.25 seconds (files took 0.6538 seconds to load)
  1019 examples, 0 failures, 9 pending
  
  Coverage report generated for RSpec to /Users/justinw/go/src/github.com/cloudfoundry/bosh-azure-cpi-release/src/bosh_azure_cpi/coverage. 4247 / 4266 LOC (99.55%) covered.
  

Testing Notes:

• All of the new code added by this PR is fully covered by unit specs.
• I didn't add integration specs for the new features, since it would involve either modifying the existing integration and BATS assets, or else adding a parallel separate set of such assets.
• The 9 Pending specs are new specs that I added, which relate to existing (long before this PR) uncovered lines of code. I didn't have time to implement these 9, but I figured adding these as pending specs would make it easier fo someone else to get the project back up to 100% coverage.
  • I did add some new specs (which ar unrelated to this PR), to cover some of the pre-existing uncovered lines. The 9 remaining Pending specs are the ones left over.

Rubocop:

bosh_azure_cpi % ./bin/rubocop_check
~/go/src/github.com/cloudfoundry/bosh-azure-cpi-release/src/bosh_azure_cpi 
~/go/src/github.com/cloudfoundry/bosh-azure-cpi-release/src/bosh_azure_cpi
Running rubocop with Ruby version ruby-2.7.3...
...
187 files inspected, no offenses detected

Rubocop Notes:

• I fixed some pre-existing rubocop offenses (mostly related to layout and formatting).
  • I was going to fix more, but was worried that it would affect so many files it would make this PR hard to code review. If this PR gets accepted fairly soon, I might have time left to submit additional rubocop fixes as a separate PR.

Changelog

• Fixes Support for application gateways with multiple backend pools #644.
• Modified the vm_type/application_gateway config to allow an Array (of Hashes) to be specified (as an alternative to the currently-supported config data types of String and Hash), to enable multiple AGWs (with independent vm_type/application_gateway/resource_group_name or vm_type/application_gateway/backend_pool_name values).
• Modified the vm_type/load_balancer config to support a new vm_type/load_balancer/backend_pool_name sub-property, to allow the config to exlpicitly specify the name of the backend address pool to use. (This change re-establishes/maintains symmetry between the vm_type/load_balancer and vm_type/application_gateway configs.)
• Modified the vm_type/load_balancer config to allow an Array (of Hashes) to be specified, to enable multiple load balancers (with independent vm_type/load_balancer/resource_group_name or vm_type/load_balancer/backend_pool_name values). (This change re-establishes/maintains symmetry between the vm_type/load_balancer and vm_type/application_gateway configs.)
• Fixed various missing and inaccurate doc comments.
• Fixed various rubocop offenses.
• Fixed the names/contexts of some existing specs with inaccurate names.
• Added missing specs for previously-uncovered code.
• Upgraded to RSpec from v.5.0 to v3.10.0 (and revendored the gems).

[klakin-pivotal]

If you're not officially done with this PR and submitting it for review, please ignore the remainder of this comment.

I'm probably not qualified to be a good reviewer for this.

But. If you're officially done with this PR, would you squash the >150 commits in this PR into one or more commits that group the changes into more-easily-understandable changesets? Thanks!

[Justin-W]

If you're officially done with this PR, would you squash the >150 commits in this PR into one or more commits that group the changes into more-easily-understandable changesets? Thanks!

@klakin-pivotal I am officially done, and ready for the PR to be reviewed. And I'd be happy to squash the commits down if you want.

However, before I squash any further, please note that the current number of commits (169) is after I had already squashed a lot of the intermediate commits, and re-ordered most of the commits into sequential, logical groups. However, some of the files changed (particularly some of the spec files) resulted in some pretty significant changes as a final result of the incremental commits. IMO, squashing some of the remaining incremental commits would make it significantly harder (or impossible) to understand the incremental changes that led to the major changes. And git tools do make it fairly easy to view diffs of arbitrary ranges of commits, but can't show you the incremental commits again after they've been squashed.

I also spent a fair amount of effort on making sure that the individual commits' messages are accurate, to make analysis and review of the PR easier.

So with that said, I'd still be happy to squash the PR's commits down (a little or a lot, as you prefer), if you still want me to. But it is a one-way process that seems to have more drawbacks than benefits (when considering what is easily achievable with git CLI or git clients), so I wanted to double check that you really want me to do so (and by how much) before I do it, since if I were the one reviewing such a PR, I'd prefer more, smaller, simpler commits over fewer, larger, squashed commits. But if you can (re)confirm your preference, I'll do it. 😄

[klakin-pivotal]

However, some of the files changed (particularly some of the spec files) resulted in some pretty significant changes as a final result of the incremental commits...

Are you certain that -for example- this group of four commits:

150a0d2
3a70ca9
f188d09
874d7e4

or this group of two commits:
8cda759
42de316

aren't sufficiently related that the commits in each of the two groups should be squashed down to one commit per group?

If -after reviewing the commits I called out-, you do change your mind about not squashing them together, then I'd politely request that you take an hour or two to do some history grooming on this PR... squashing commits together that are logically related, and even reordering the commits so that it's easier to squash together commits that are logically related but separated by other -unrelated- commits. Squashing and -where appropriate- reordering commits will better ensure that the story your remaining commits do tell makes as much sense as is reasonably possible.

Additionally, it's possible that you and I are looking at two different sets of commits. If we are, then please accept my apologies for the noise, and please do direct me to the list of commits that I should be looking at.

[Justin-W]

I'd politely request that you take an hour or two to do some history grooming on this PR... squashing commits together that are logically related, and even reordering the commits so that it's easier to squash together commits that are logically related but separated by other -unrelated- commits. Squashing and -where appropriate- reordering commits will better ensure that the story your remaining commits do tell makes as much sense as is reasonably possible.

@klakin-pivotal Done. I think it now presents a more easily understood story. If you'd like me to squash further, I certainly can, but doing so may affect the ability to easily revert/amend isolated portions of the PR's changes, if any of the reviewers request changes to a particular design/implementation decision. Also, see notes below.

Notes about the commits in this PR:

• Of the 84 commits, only the 40 commits with issue-644 in the commit msg are directly related to new code/functionality for Support for application gateways with multiple backend pools #644.
• The other 44 commits are related to existing code, and are mostly cleanup/refactoring/enhancement of code sections that relate to or were subsequently modified by the commits directly relating to Support for application gateways with multiple backend pools #644. Most of those 44 commits are clustered at the beginning (before the first issue-644 commit).
• Just in case, I temporarily preserved the original (pre-squashed) version of this PR's commit history here.

[ramonskie]

this PR is massive..
on the one hand awsome that you put so much effort in to it.
the other side of it is that its really hard to review.

i didn't go in detail over all the rspec tests changes that where made.
but the code for moving over to using an array for the loadbalancers looks good to me.

[beyhan]

I also couldn't get into all the details here. I have some small suggestion otherwise the feature implementation looks good. One additional question. Did you consider to extend the integration tests or the unit tests provide good coverage here for the introduced changes?

[Justin-W]

One additional question. Did you consider to extend the integration tests or the unit tests provide good coverage here for the introduced changes?

Yes and yes.

The new and updated unit specs provide much more thorough coverage (of both the new and old formats) of the various config variations than I saw for any other properties' unit specs. E.g. Single vs multiple LB/AGW, single vs multiple backend pools, unspecified vs explicit pool names, and specs for each supported "data format" (including already-supported formats which didn't have any unit nor integration spec coverage before).

And yes, I considered adding integration specs, as well. I had started some preliminary work in 87b33ed, but reverted it in e5e3b1c, because (as noted in the Testing Notes here:

I didn't add integration specs for the new features, since it would involve either modifying the existing integration and BATS assets, or else adding a parallel separate set of such assets.

And since there is now (due to this PR's enhancements) excellent unit spec coverage of the LB and AGW config variants, and since I also wasn't sure if anyone other than my team would even be using the new feature, and since none of the previous LB variant format enhancements had updated the integration specs (and some hadn't even updated the unit specs nor the CPI docs), it seemed like it would probably be unnecessary and/or overkill to implement an entire parallel set of int specs (especially since the int suite costs real money per run).

Plus, the initially-broken dev env setup instructions, LB-related refactoring, retroactive specs, and other repo cleanup work I ended up having to do for this PR already took a lot more time than I had planned on spending on this entire PR. 😄

[RobertBoyd]

Is there any progress on the reviews against this PR? Would really like to take advantage of this new functionality...

Thanks!

[klakin-pivotal]

I have some things that I want to say about the PR, but we've been quite busy with a few very urgent fires. Will hopefully get around to making some comments next week.

(For what it's worth, it's not extremely difficult to roll your own CPI release. So if you're very interested in using the functionality in this PR, you can snag the code and start using it now.)

[beyhan]

@Justin-W Thank you for the fast reaction on my comments. I added additional comments about empty test cases. Sorry, for missing those during the first review. I also adde @klakin-pivotal as reviewer because of the comment here #651 (comment)

[beyhan]

I as commented we should remove the empty test cases which aren't related to this pr. Otherwise it looks good to me.

[Justin-W]

Thanks for the detailed feedback @klakin-pivotal. I believe I have now addressed your concerns (details below).

In terms of total commit count, if merged, this PR would make up ~9% of the commits in this repo. I've looked over the PR, and I don't understand why there are so many commits.

I have squashed most of the original commits down to 6 commits, and removed all other unrelated and "housekeeping" commits (see below for details) which seemed to be "non-essential" to this PR.

I would never reject a patch series just because the best way to split it up is into a whole bunch of commits... if you need 100 or 1000 commits to break some work up into the most logical series of changes, then that's definitely what needs done.

However, there are two big things about this patch series that bother me, and several small things.

The big things:

1. It contains many, many, many commits that have a brief explanation of what was done but absolutely no explanation of why. Not every commit needs to explain the why... but nearly zero of your commits explain why what was done was done.

All 6 remaining commits now have messages explaining the "why" behind the "what".

2. There are a bunch of commits that appear to be pretty spurious. For example: several commits in the patch series that (according to the commit message) just revert previous commits. Is it important to someone who will look at this commit history twelve months down the road to know about things like that?

The "MAYBE?" commit (and the following commit which reverted it) have been removed (refer to the explanation below for details, if interested).

The small things:

1. These "MAYBE?" commits? Is that commit message the correct thing to be telling a future someone who's trying to understand the why of this patch series? Do they need to understand how uncertain you were of that particular commit? If you think they do, I would consider that a signal that commits like that should be squashed into one or more other commits that (when combined) made you sufficiently certain of your changes to submit them as a PR.

TL;DR: I have removed that code from this PR.

Background:

This particular commit was related to the Integration test suite, and my uncertainty of whether or not that suite needed to be updated. Based on previous PR feedback (which indicated that my unit test suite updates seemed sufficient), I have removed that commit (and the revert which followed it) from this PR.

2. What's up with the Rubocop rule changes? Why were any Rubocop rule changes required?

TL;DR: These were changes related to #653 but not related directly to #644, so I have removed such unrelated changes from this PR.

Background:

These were mostly opportunistic "leave it as good or better than you found it" attempts to reduce the (large) quantity of offenses ignored/excluded by the .rubocop_todo.yml file (work which I subsequently created #653 to track). Both the changes to .rubocop.yml and the inline exclusions were my attempts to make the project's explicit rubocop rules match the project's existing design and implementation, in cases where the existing code was in direct conflict with rubocop default configurations. I.e. I was trying to make the rubocop config more accurately reflect certain non-rubocop-default project implementation choices, rather than leaving them as "ignored offenses to be fixed later".

However, for the sake of PR clarity and based on your feedback, I have removed all of those changes from this PR.

3. Why does this patch series contain a change to the version of rspec? Was that a required part of adding this feature? If it wasn't, then that change should not be part of this patch series.

TL;DR: I have removed that code from this PR.

Background:

I encountered a spec failure which was difficult to understand due to rspec truncating the output. This is a known issue with rspec, and there is a semi-official "known fix" for it which requires a newer version of rspec than the one previously configured for this project.

However, after upgrading rspec, the "known fix" didn't work. So technically, the rspec upgrade wasn't required as part of this PR, since I didn't actually enable the not-yet-working "known fix".

I had originally left the rspec upgrade in this PR since the effort to upgrade had already been done, and I had assumed that a newer rspec version would be preferred. However, I have now removed that upgrade from this PR, per your feedback.

4. Some of this patch series has parts that just add or remove a blank line. These changes should not be in this patch series.

These were directly related to the aforementioned opportunistic rubocop offense fixes, and have been removed from this PR.

5. What's up with the return type annotations that are added to some functions? For an example, consider the additional annotations that begin here: https://github.com/cloudfoundry/bosh-azure-cpi-release/pull/651/files#diff-6eaadd97bb87d9f3b51e3214a0aa347de08a67afae0923b61b84cad6073b50b4R1955 Much of the rest of the file doesn't appear to have those... why were they added to otherwise untouched code? If you wanted to add type annotations to all of the functions in the CPI, that would be lovely... but that sort of change should be in a separate PR, not in this one.

TL;DR: I have removed that code from this PR.

Background:

There were a few utility methods called by code related to this PR which had no doc comments. And some of those methods' implementations had changed over time so that their method names had become inaccurate. I added the return type annotations to a few such methods while I was trying to understand the behavior of the original code (before I modified any of the behavior).

Since there were also many other un-annotated methods within azure_client.rb which also had names which were non-indicative of the return type (thus requiring a maintainer to read each method's implementations to understand what type of value would be returned), I opportunistically added @return annotations to many of those methods, assuming that such annotations would be better than completely un-annotated methods. However, per your feedback, I have removed from this PR the opportunistically-added annotations for the methods which were otherwise unmodified and unrelated to this PR.

6. Does this entirely-commented-out block actually do anything? https://github.com/cloudfoundry/bosh-azure-cpi-release/pull/651/files#diff-1a1fbe20391f7dce08027421a87d3efb885328926612e3823ccf57a05259b7f0R172 If it does, I really wanna know... Ruby is magical sometimes, and I'm always excited/horrified to learn of yet another way it can appear to do one thing, but actually do another. If it doesn't actually do something, you should remove it and also do a careful sweep to ensure that you haven't put other commented-out dead code into this patch series.

TL;DR: I have removed that code from this PR.

Background:

That was the code for the aforementioned (see comment above RE: point # 3) "known fix" for the rspec truncation issue, which turned out to not work. Since the known issue is likely to be a problem again in future, I had left the disabled code (plus explanatory comments) for the not-yet-working fix, in case I or a future maintainer needed to attempt to et the fix working in future.

7. Looking at the individual commits, I would have an extremely difficult time reviewing this PR. In fact, I had to have GitHub squash them down into a single diff so that I could understand the changes. That is a very strong sign that anyone twelve months in the future who's trying to make sense of this part of the Git history will have a very bad time.

An ideal commit should contain changes that are logically grouped together, with an explanation of what was done and why.

I have attempted to explain both the what and the why in the messages of all 6 of the remaining commits (referencing #644 where appropriate, but without completely re-explaining the context captured in the description and comments of that issue).

Hopefully the new messages are satisfactory. I looked at many past commits within this repo for examples, but nearly all of them had very minimal, terse commit messages. So I mostly used your commit message guidance above.

This helps future folks who don't have any context on what was changed in the code and why (which -BTW- this future person very often ends up being you) understand the what and why of the code.

Please do squash this PR down to just a few commits that someone who has nearly-zero context on what was done in this PR and why it was done can read and understand. Remove the unnecessary whitespace changes, either help me to understand why you made RuboCop rules changes, or just undo those changes, and address the other stuff I brought up.

I believe each of the issues you previously raised have all now been fully addressed/resolved. If any were not, please let me know.

For now, I strongly NAK merging this PR as-is.

As an aside: if you're like me, and you make a ton of "checkpoint" commits while working on something big and/or unfamiliar, do feel free to keep those in a local branch for as long as you like. I -personally- find that keeping those commits around for quite a while does help me to find and fix errors in the code that I wrote... but I also recognize that the very many commits are only helpful to me because I remember much of what I was thinking about when I authored those commits. I also recognize that that mess of commits are nearly completely useless to folks who lack that context. Other folks need the changes bundled up in larger chunks, with notes attached in the commit messages that explain all of the context that I had in my head while I was making those changes.

Thank you (again) for the time you took in providing detailed PR feedback. It was helpful and much appreciated.

[Justin-W]

Thanks also to @bgandon for his additional tips and advice.

[rkoster]

@klakin-pivotal are you satisfied with the changes made?

[klakin-pivotal]

I hope to get some time this afternoon to review the changes. If I do not get that time, then I will make the time on Friday.

[klakin-pivotal]

Overall, this PR looks good. Thanks for taking the time to reorganize it and clean it up.

If the folks who did the technical-functionality review are still fine with that aspect of the PR, and the few things I mention below get fixed, please feel free to merge it, whoever is in charge of merging it. (And if that "whoever" is me, let me know and I'll push the button on Tuesday (as Monday is a holiday for me).)

The remaining issues:

Commented-out sections of code:

These commented-out sections of code should be removed, if they don't serve as documentation. If they do, then do accept my apologies for not noticing their significance:

c099e1f#diff-5d4d5c53bf030aa246140de1d33e1f300b0093c44e87df37e1f5bd73f2c9508eR151

4ced629#diff-9850a43e9c57d5d53acbfe53ce7dfd26ff1ecbc939f05079faf1f76873e31780R459-R462

4ced629#diff-9850a43e9c57d5d53acbfe53ce7dfd26ff1ecbc939f05079faf1f76873e31780R911-R914

4ced629#diff-9850a43e9c57d5d53acbfe53ce7dfd26ff1ecbc939f05079faf1f76873e31780R603-R606

4ced629#diff-9850a43e9c57d5d53acbfe53ce7dfd26ff1ecbc939f05079faf1f76873e31780R620-R623

4ced629#diff-9850a43e9c57d5d53acbfe53ce7dfd26ff1ecbc939f05079faf1f76873e31780R889-R892

4ced629#diff-9850a43e9c57d5d53acbfe53ce7dfd26ff1ecbc939f05079faf1f76873e31780R1036-R1039

4ced629#diff-9850a43e9c57d5d53acbfe53ce7dfd26ff1ecbc939f05079faf1f76873e31780R1048-R1051

fba4230#diff-0f27965b66475a2daee83501a79ec969ed15ce1124fd6bac0bc899e857485b9aR231

c099e1f#diff-0f27965b66475a2daee83501a79ec969ed15ce1124fd6bac0bc899e857485b9aR371

All of the places that # 'resource_group_name' => 'fake-rg-name',, # 'backend_pool_name' => 'fake-pool2-name', or # 'backend_pool_name' => 'fake-ag2-pool-2-name' appear in dynamic_public_ip_spec.rb: 4ced629#diff-f32b10482e6d4d856d908fa2c875e88c5ade80f3d60c35c754a966f2676f8f2f

Please don't feel obligated to re-work the commits that contain the commented-out sections of code. Adding another commit to the PR that removes any that should be removed seems fine to me.

Comments added that have TODO in them

I'd ask that other folks who have previously evaluated the technical side of PR evaluate whether the TODO comments in the PR are comments that indicate that there are additional tests that should be added now, are comments that are useful to future test-writers, or are comments that should be removed.

[Justin-W]

These commented-out sections of code should be removed, if they don't serve as documentation. If they do, then do accept my apologies for not noticing their significance:

These did seem to have documentation value to me, but since that wasn't apparent to others I've removed all of the referenced commented sections.

I'd ask that other folks who have previously evaluated the technical side of PR evaluate whether the TODO comments in the PR are comments that indicate that there are additional tests that should be added now, are comments that are useful to future test-writers, or are comments that should be removed.

The only TODO comments that I added were the # TODO: Add more expectations here? The expects above only verify that the VM was created, ... comments in dynamic_public_ip_spec.rb. The comment added on line 550 relates to a unit test that predates this PR's changes. All of the other TODO comments are in new unit tests which are derived from that original unit test (basically the same test, but using different configs). I wasn't sure how to fix the expectations in the original test, and therefore the new tests which are derived from it also use the same type of expectations as the original.

IMO, leaving these TODOs as suggestions for future work does no harm, since it does not worsen the state of the existing functionality or tests (the TODOs merely calls out an opportunity for future improvement).

Resolving the TODOs immediately (by improving the expectations of the marked tests) would obviously be better, but I'd need some guidance and/or an example of how to implement more precise expectations for the tests in that file, since these tests relate to a portion of the CPI's functionality about which I have limited understanding and (as I mentioned above) I'm not sure how to correctly resolve these TODOs without additional guidance.

[klakin-pivotal]

@beyhan @ramonskie If you guys already did a technical-correctness review on the PR, would you mind re-checking this PR? Many commits have been merged together, and some of their contents have been changed, so it'd be good if you could ensure that the functionality remains correct.

Check this comment for a summary of what I thought would be good to do: #651 (comment)

(I'm tagging both of you, because it's not clear which of you did a technical review... so if this doesn't apply to one of you, then feel free to ignore this comment.)

[beyhan]

I will be able to go over again most probably tomorrow.

[RobertBoyd]

I will be able to go over again most probably tomorrow.

Thank you both @beyhan @klakin-pivotal - much appreciated.

Given the amount of time this PR has been open with holidays and such, we've now moved onto another thread of work. Hopefully we can tie a bow on this sometime soon.

[Justin-W]

@beyhan Can we get an update on this please?

[beyhan]

I re-checked the pr and the logic to support multiple AGWs/LBs still looks good. @bgandon you requested changes which should be implemented now. Could you please re-check?

[Justin-W]

@klakin-pivotal @rkoster I think this can be merged now, correct? As far as I know, all previously-raised concerns have now been resolved.

[rkoster]

Yes you are right. Thanks @Justin-W for all the work you have put into this PR!