Support Hash-Based Routing #4696
Conversation
hoffmaen
force-pushed
the
hash-based-routing
branch
2 times, most recently
from
f181a31 to
6ae8cb9
Compare
hoffmaen
force-pushed
the
hash-based-routing
branch
from
6ae8cb9 to
fe3a37a
Compare
philippthun
left a comment
philippthun
left a comment
There was a problem hiding this comment.
Did not look at the spec files so far...
| Route.db.transaction do | ||
| route.options = route.options.symbolize_keys.merge(message.options).compact if message.requested?(:options) | ||
| if message.requested?(:options) | ||
| merged_options = message.options.compact |
There was a problem hiding this comment.
Isn't the merge missing, i.e. route.options.merge(message.options)?
|
|
||
| def update | ||
| message = RouteUpdateMessage.new(hashed_params[:body]) | ||
| params = hashed_params[:body] |
There was a problem hiding this comment.
The controller should not manipulate the params and the message should reflect what has been provided by the user. Isn't this redundant with RouteUpdate.update anyway?
| elsif manifest_route[:options] && route[:options] != manifest_route[:options] | ||
| # remove nil values from options | ||
| manifest_route[:options] = manifest_route[:options].compact | ||
| # Merge existing route options with manifest options for partial updates |
There was a problem hiding this comment.
I would move the merge to RouteUpdate.update to have it in a single place.
There was a problem hiding this comment.
On route updates, we need to validate after merging, as we need to allow incremental route option updates. E.g., only setting hash_balance would be invalid for a new route, but is valid for a route that is already properly setup with hash-based routing. RouteUpdate.update is the last step in this chain, so we would also need to migrate the validation logic to RouteUpdate. I think the OptionsValidator is the better place to have the validation, and thus also the merging. Would you agree?
| return if options[:hash_balance].blank? | ||
|
|
||
| errors.add(:base, message: "Route '#{route[:route]}': hash_balance can only be set when loadbalancing is hash") |
There was a problem hiding this comment.
Why not errors.add(...) if options[:hash_balance].present? as above?
| VALID_LOADBALANCING_ALGORITHMS_WITH_HASH = %w[round-robin least-connection hash].freeze | ||
| VALID_LOADBALANCING_ALGORITHMS_WITHOUT_HASH = %w[round-robin least-connection].freeze |
There was a problem hiding this comment.
Would it make sense to define VALID_LOADBALANCING_ALGORITHMS_WITH_HASH as VALID_LOADBALANCING_ALGORITHMS_WITHOUT_HASH + 'hash'?
| if hash_balance_value.present? | ||
| # Always convert to string for consistent storage in JSON | ||
| normalized[hash_balance_key] = hash_balance_value.to_s | ||
| end | ||
|
|
||
| normalized |
There was a problem hiding this comment.
Suggestion:
return opts if hash_balance_value.blank?
normalized = opts.dup
normalized[hash_balance_key] = hash_balance_value.to_s
normalized
| if route_mapping.route.options | ||
| opts = route_mapping.route.options | ||
|
|
||
| route_hash[:options] = {} |
There was a problem hiding this comment.
Why not route_hash[:options] = route_mapping.route.options?
| return if hash_balance.blank? | ||
|
|
||
| errors.add(:hash_balance, 'can only be set when loadbalancing is hash') |
There was a problem hiding this comment.
I would prefer errors.add(...) if hash_balance.present?.
| end | ||
|
|
||
| def validate_hash_balance_format | ||
| return if hash_balance.nil? |
There was a problem hiding this comment.
Should we use blank? instead of nil? as in other places?
2 participants
This Pull-Request adds support for hash-based routing to Cloud Controller.
Summary:
The routes model is enhanced as follows:
hashis added as a validloadbalancingoptionhash_headeris added as a per-route option. The option is mandatory whenloadbalancing=hashhash_balanceis added as a per-route option. The option is optional whenloadbalancing=hashValidation of these options is added when creating and updating both via API and via manifest. When
hash_balanceis provided as an option, its value must be a float or a string representing a float.The options are still stored as a raw JSON string in the routes table. The value for
hash_balanceis always stored as a string inside the JSON for consistency.Logic is introduced to remove
hash_balanceandhash_headerwhen switching fromhashto another load-balancing algorithm. Incremental updates on per-route options are allowed, e.g. only updatinghash_headerorhash_balancewhile keeping other previously set per-route options is possible.Links:
capi-release implementation issue
routing-release Pull-Request for hash-based routing support
RFC-0042
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests