Skip to content

Conversation

@jrushford
Copy link
Contributor

@jrushford jrushford commented Sep 25, 2019

This PR adds NextHop selection strategies that may be used in place of the parent selection strategies in parent.config. The NextHop selection strategies are used in remap.config with a new @strategy tag. NextHop strategies are defined in a new config file, 'strategies.yaml'. When the remap.config is loaded, the NextHopStrategy factory loads the strategies.yaml and creates and configures instances of NextHopSelectionStrategies. When remap.config is parsed, a strategy is associated with a remap entry if there is a tag '@strategy=$name' in the config. If the strategy is configured for a remap, wrapper functions used for parent selection in HttpTransact will prefer the NextHopSelection strategies over the ParentSelection strategies when searchin for a NextHop or parent host. Sample strategy.yaml files are at proxy/http/remap/unit-tests.

@jrushford jrushford added this to the 9.1.0 milestone Sep 25, 2019
@jrushford jrushford self-assigned this Sep 25, 2019
@jrushford jrushford force-pushed the next_hop_remap branch 4 times, most recently from f6b21e2 to 2420a8e Compare September 27, 2019 17:07
@jrushford jrushford changed the title WIP: Add Nexthop selection strategies and @strategy tag to remap.config. Add Nexthop selection strategies and @strategy tag to remap.config. Sep 27, 2019
@jrushford jrushford removed the WIP label Oct 2, 2019
@jrushford jrushford force-pushed the next_hop_remap branch 2 times, most recently from 7594037 to 3c0d503 Compare October 4, 2019 14:07
@jrushford jrushford modified the milestones: 9.1.0, 10.0.0 Oct 8, 2019
@a-canary
Copy link
Collaborator

a-canary commented Oct 9, 2019

This is a big PR. I'll try to read through this over the next week.

Copy link
Contributor

@randall randall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the general format here. (strategies, groups, hosts). Personally, I find the #include business weird. Maybe what Alan suggested with include: [filename.yaml, file2.yaml] is better. I don't have a strong enough option here to argue either way.

@jrushford
Copy link
Contributor Author

jrushford commented Oct 18, 2019

@randall the #include is used to concatenate multiple file streams into a single stream as a single YAML document that then may be parsed. @AMC idea of using 'include: [ file1.yaml, file2.yaml] complicates this IMHO. I'd have to parse a partial YAML file, look for a YAML 'include:' name space then re-read all the files so that I can then parse 'strategies'. I think this makes it too complicated and I think the '#include file' is simpler and more flexible. I do like @AMC's idea of reading a directory and concatenating YAML files in 'ls' order and I'll probably add that. But, using a YAML list to include other files just seems to complicated to me so, I'm leaving the '#include' for now.

Copy link
Collaborator

@a-canary a-canary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of comments and questions. Only blocking concerns are:

  • REMAP_OPTFLG_STRATEGY 0x800000000u is too big.
  • NextHopSelectionStrategy::markNextHopDown isn't called from anywhere.

@jrushford jrushford requested a review from a-canary November 1, 2019 20:59
@jrushford
Copy link
Contributor Author

Lots of comments and questions. Only blocking concerns are:

  • REMAP_OPTFLG_STRATEGY 0x800000000u is too big.
  • NextHopSelectionStrategy::markNextHopDown isn't called from anywhere.

@a-canary this is fixed.

a-canary
a-canary previously approved these changes Nov 4, 2019
@jrushford jrushford force-pushed the next_hop_remap branch 2 times, most recently from 44316d7 to c032f79 Compare November 6, 2019 14:37
@jrushford
Copy link
Contributor Author

rebased to pick up fix for FreeBSD CI build

@a-canary
Copy link
Collaborator

a-canary commented Nov 7, 2019

+1

a-canary
a-canary previously approved these changes Nov 7, 2019
@gtenev
Copy link
Contributor

gtenev commented Nov 7, 2019

I am still looking into this PR

Copy link
Contributor

@vmamidi vmamidi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gtenev is looking into this PR.

@a-canary
Copy link
Collaborator

@gtenev @vmamidi This code isn't perfect. But it's a step in the right direction. I expect this to evolve and mature greatly over the next few releases. We'll start testing this for production in Q1.

@vmamidi
Copy link
Contributor

vmamidi commented Nov 12, 2019 via email

@jrushford
Copy link
Contributor Author

rebased to pick up yaml 0.6.3 update.

calavera
calavera previously approved these changes Nov 15, 2019
Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks very useful. I really like the failover strategies part 👍

@jrushford
Copy link
Contributor Author

@calavera still working on some review comments. It'll go in soon.

@jrushford jrushford force-pushed the next_hop_remap branch 4 times, most recently from 161ab8c to d3c57a9 Compare November 18, 2019 17:17
@jrushford jrushford merged commit 128507a into apache:master Nov 19, 2019
@zwoop
Copy link
Contributor

zwoop commented Nov 20, 2019

This is breaking big time on the CI, segfaulting the tests.

@zwoop zwoop modified the milestones: 10.0.0, 9.1.0 Jan 28, 2021
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.

7 participants