rlm_redis_ippool: break out lua scripts as defacto schema

[jimdigriz]

Summary of changes:

• extracted lua code into separate files (can be treated as a schema for redis_ippool)
• documented the namespace usage in Redis
• removed 'gateway' and 'counter' as both were not plumbed into anything in the years they have existed (conversed with @mcnewton on this)
• moved rlm_redis_ippool.c:ippool_script to cluster.c:fr_redis_script so to remove code duplication
• moved IP range handling into Lua (partially addresses concerns in Speed up redis IP pool name listing on high latency links #1744) (65k item guard for range based operations) all in preamble.lua:iptool_module
• rework ippool tool to just perform a single task (chaining operations should be via script/xargs/...)
• make IP sticky work (device keys live for 10x lease time)
• fix 'list' in the tool as it looks like it never worked (pool 'start' needed to be non-zero)
• 'modify' removed as you can just re-run 'add' to perform the same task (it will noop already existing bits, and update the 'address' hash key if need be)

Talking points:

• not 100% convinced this patch set can be broken up, most of it is "lift and move the lua code into files"; in doing so some opportunities to simplify things were too hard to ignore
• patch looks large, but most of it is "move into separate files and load them"
• JSON as a transport for 'show' and 'stats'. Though 'stats' could be made an array again, things are a little harder for 'show'. This is helpful though for other consumers that which to poke/peek Redis directly
• replace script loading in the config with (as of yet unimplemented) ${READFILE} /path/to/file.lua (suggested by @arr2036)
• I have not run the unit tests as it is unclear how (what deps, I seem to have lots missing), but that is why you have CI...lolz, I'll wait 30mins and check back in on the carnage there

[jimdigriz]

I effectively reverted 3d07585 in my PR, so we probably should get that resolved properly.

[jpereira]

maybe:

local ippol_rcode = {
   not_found = -1,
   device_mismatch = -3
   ..........
}

then can call using: ippol_rcode.notfound

[jimdigriz]

As all the logic is in a single small file I do not think there is no gain to readability and indirect table access requires an additional load (not checked the JIT output but guessing it turns directs variables into immediates) via the call to GETTABLE.

I think this boils down to coding style, so happy to change it.

$ cat example-direct.lua 
local rcode_one = 1
local rcode_two = 2
local rcode_three = 3

print(rcode_one)

$ luac -l example-direct.lua 
main <example-direct.lua:0,0> (7 instructions, 28 bytes at 0x558023218550)
0+ params, 5 slots, 0 upvalues, 3 locals, 4 constants, 0 functions
        1       [1]     LOADK           0 -1    ; 1
        2       [2]     LOADK           1 -2    ; 2
        3       [3]     LOADK           2 -3    ; 3
        4       [5]     GETGLOBAL       3 -4    ; print
        5       [5]     MOVE            4 0
        6       [5]     CALL            3 2 1
        7       [5]     RETURN          0 1

$ cat example-table.lua 
local rcode = {
        one     = 1,
        two     = 2,
        three   = 3
}

print(rcode.one)

$ luac -l example-table.lua 
main <example-table.lua:0,0> (8 instructions, 32 bytes at 0x556389880550)
0+ params, 3 slots, 0 upvalues, 1 local, 7 constants, 0 functions
        1       [1]     NEWTABLE        0 0 3
        2       [2]     SETTABLE        0 -1 -2 ; "one" 1
        3       [3]     SETTABLE        0 -3 -4 ; "two" 2
        4       [4]     SETTABLE        0 -5 -6 ; "three" 3
        5       [7]     GETGLOBAL       1 -7    ; print
        6       [7]     GETTABLE        2 0 -1  ; "one"
        7       [7]     CALL            1 2 1
        8       [7]     RETURN          0 1

[jpereira]

if failed, it should continue or go to some of those err labels?

[jimdigriz]

Good catch, fixed in jimdigriz@7a6e966

[arr2036]

The gateway ID and counter fields should not be stripped. They're used in customer deployments. Can you add that code back in, and I'll add some additional code to dump them into the current FR request to make them available for general consumption.

[arr2036]

Can you break out your changes to the content of the lua scripts into a separate commit. Because they've been moved bodily to external files, it's hard to see what actually changed.

[jimdigriz]

Why is the gateway ID stuff stripped out everywhere?

---

Summary of changes:
[snipped]

• removed 'gateway' and 'counter' as both were not plumbed into anything in the years they have existed (conversed with @mcnewton on this)

---

I could not see any active use of the gateway parameter in the code base, nothing was plumbed in for processing Accounting-On packets. Maybe I missed it?

[arr2036]

Why is the gateway ID stuff stripped out everywhere?

Summary of changes:
[snipped]

• removed 'gateway' and 'counter' as both were not plumbed into anything in the years they have existed (conversed with @mcnewton on this)

I could not see any active use of the gateway parameter in the code base, nothing was plumbed in for processing Accounting-On packets. Maybe I missed it?

Yes I read the comment and changed the query to a request :)

They're both used, but not queried directly through rlm_redis_ippool in existing deployments. It shouldn't be hard to fix that though and dump them into the request list when the ippool module is called.

[jimdigriz]

Can you break out your changes to the content of the lua scripts into a separate commit. Because they've been moved bodily to external files, it's hard to see what actually changed.

Not sure what you mean by this? As originally most of the Lua was in a large char array it is not really readily comparable; I suspect even a git word diff would not help.

Maybe if you describe what you want to see, maybe with an example, it will be clearer to me?

[jimdigriz]

Why is the gateway ID stuff stripped out everywhere?
[snipped]
They're both used, but not queried directly through rlm_redis_ippool in existing deployments. It shouldn't be hard to fix that though and dump them into the request list when the ippool module is called.

Are you thinking as just a generic KV store? Something that would just extend the address map object?

[arr2036]

Can you break out your changes to the content of the lua scripts into a separate commit. Because they've been moved bodily to external files, it's hard to see what actually changed.

Not sure what you mean by this? As originally most of the Lua was in a large char array it is not really readily comparable; I suspect even a git word diff would not help.

Maybe if you describe what you want to see, maybe with an example, it will be clearer to me?

In addition to changing the location of the lua code, you changed the functionality of the lua code. It would have been nice to see the changes in content first, before you moved it over to external files.

The alternative would be to have a commit moving the lua code out to separate files and changing the return codes, then another commit changing the functionality, which I suspect would be easier.

[arr2036]

Why is the gateway ID stuff stripped out everywhere?
[snipped]
They're both used, but not queried directly through rlm_redis_ippool in existing deployments. It shouldn't be hard to fix that though and dump them into the request list when the ippool module is called.

Are you thinking as just a generic KV store? Something that would just extend the address map object?

Not currently, I was just going to dump the values into attributes, but yeah eventually it might be nice to offer that functionality.

[jimdigriz]

The alternative would be to have a commit moving the lua code out to separate files and changing the return codes, then another commit changing the functionality, which I suspect would be easier.

Compile tested only, good enough to see the affect, shows conceptually how extracting the Lua code looks: master...jimdigriz:rlm_redis_ippool_lua_stepped

I am continuing to add the remains of master...jimdigriz:rlm_redis_ippool_lua as more digestible parts.

[jimdigriz]

This work cycle is now complete, tests have also been fixed up.

TravisCI/clang seems to be upset about HTML entities in a C header file, not really sure how I could have caused this: https://travis-ci.org/FreeRADIUS/freeradius-server/jobs/539870493#L3706

Can someone look over my PR and let me know what the next steps for me are.

[jpereira]

@jimdigriz Try to:

1. Remove the unnused function ippool_wait_check
2. Rename cluster.h by redis_ippool.h in src/modules/rlm_redis_ippool/redis_ippool.h

[jimdigriz]

@jimdigriz Try to:

1. Remove the unnused function `ippool_wait_check`

Done 06bac70, thanks!

2. Rename `cluster.h` by `redis_ippool.h` in src/modules/rlm_redis_ippool/redis_ippool.h

Not sure what you mean by this.

[jpereira]

I mean, update the documentation in src/modules/rlm_redis_ippool/redis_ippool.h replacing the cluster.h by redis_ippool.h

[arr2036]

Can you squash the fixes that address errors in new code? I'll push something for the copyright notices.

[arr2036]

Rebase and doxygen should no longer cause issues

[jimdigriz]

Changes made, tests are now passing.

Can someone look over my PR and let me know what the next steps for me are.

[arr2036]

Can you add the "modify" tests back in, and update the list of changes in the description to reflect the changes you've done recently, and the changes that are included in this PR. As we talked about offline, the utility behaviour must not change as it's already in use at multiple sites.

I really don't like the use of cjson to encode responses to the stats request, as it adds a dependency that wasn't there before. What was the reason for doing this? The Redis protocol already supports returning the contents of hashes.

[jimdigriz]

Can you add the "modify" tests back in, and update the list of changes in the description to reflect the changes you've done recently, and the changes that are included in this PR. As we talked about offline, the utility behaviour must not change as it's already in use at multiple sites.

I will do this.

I really don't like the use of cjson to encode responses to the stats request, as it adds a dependency that wasn't there before. What was the reason for doing this? The Redis protocol already supports returning the contents of hashes.

I brought this up as a possible problem over six weeks ago. I cannot work on a cycle of "make all these changes" and then something that I flagged up before those changes is not a problem that concerns you.

Thank you for wasting time and efforts.

[arr2036]

Where was this raised as an issue?

[arr2036]

Throughout this project there have been communication issues. Ones that I've tried to go out of my way to address, by outlining an effective way of working jointly on this design.

Despite this multiple intermediary design decisions were made without requesting my feedback, which as the person who's actually wrote the code originally and who has deployed it at multiple customer sites, It should have been. If nothing else, to ensure that we don't break existing deployments with changes in behaviour here.

The original PR you created had some major red flags, which is why I didn't give it serious consideration. Those were:

• Large monolithic commits. These make it much harder to determine what's been changed, and whether those changes were appropriate, and makes it very easy to miss issues like this. I admit, I didn't make the connection between requiring JSON as a stats transport, and adding a dependency to the ippool tool. It seems... bizarre to add this when the native transport could handle the encoding just fine.
• Failing tests. Which honestly, you submitted a PR with failing tests and didn't fix them for several weeks... Come on, how do you think that looks from a reviewer's perspective?

Passing tests, and clean, broken out commits are the basic requirements to get good feedback. If those aren't there it just screams "Don't give a shit" and "You should be honoured to review my work!" - don't be surprised if reviews get repeatedly deferred.

[arr2036]

Anyway, just forget the review, I'll pull your changes across manually. I don't think continuing to work on this together is going to be constructive or productive.