Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Low: test-findif: findif.sh+IPv6 support, portable, working... #154

Merged
merged 5 commits into from Nov 2, 2012

Conversation

Projects
None yet
4 participants
Contributor

jnpkrn commented Oct 15, 2012

No description provided.

jnpkrn added some commits Oct 15, 2012

@jnpkrn jnpkrn Low: test-findif: make more POSIX shell friendly
Tested to work with bash, bash --posix, dash and ksh (@Fedora 17).

Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
6cae519
@jnpkrn jnpkrn Low: test-findif: make it work with findif.sh
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
298a714
Contributor

jnpkrn commented Oct 15, 2012

Some potential regressions in findif.sh, when compared to findif
compiled binary, discovered (haven't examined thoroughly yet):

------  1) loopback
OCF_RESKEY_ip=127.0.0.1 OCF_RESKEY_cidr_netmask=
[ OK ] 
------  2) DUMMY_IP+1
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=
[ OK ] 
------  3) DUMMY_IP+1, explicit netmask bits
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=24
[ OK ] 
------  4) DUMMY_IP+1, explicit netmask
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=255.255.255.0

FAIL exit code: 6 vs 0
[FAIL] 
------ 10) *invalid* ip (missing last item in quad)
OCF_RESKEY_ip=198.51.100. OCF_RESKEY_cidr_netmask=

[ OK ] 
------ 11) *invalid* ip (random string)
OCF_RESKEY_ip=foobar OCF_RESKEY_cidr_netmask=

[ OK ] 
------ 20) DUMMY_IP+1, explicit *invalid* netmask bits (33)
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=33 

[ OK ] 
------ 21) DUMMY_IP+1, explicit *invalid* netmask bits (-1)
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=-1 
../heartbeat/findif.sh: line 116: -1: command not found
Error: an inet prefix is expected rather than "198.51.100.2/-1".
FAIL exit code: 1 vs 6
[FAIL] 
------ 22) DUMMY_IP+1, explicit *invalid* netmask bits (random string)
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=foobar 

[ OK ] 
------ 30) DUMMY_IP+1, explicit *invalid* netmask (missing last item in quad)
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=255.255.255. 

[ OK ] 
------ 30) DUMMY_IP+1, explicit *invalid* netmask (random string containing dot)
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=foo.bar 

[ OK ] 
--- TOTAL ---
[FAIL] 2
@jpokorny jpokorny Low: test-findif: add --script to the usage string
Signed-off-by: Jan Pokorný <pokorny_jan@seznam.cz>
46c2060
Contributor

kskmori commented Oct 17, 2012

I think that findif.sh should be improved more in error checking for cidr_netmask. I will work on that.

------ 4) DUMMY_IP+1, explicit netmask
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=255.255.255.0

The current findif.sh only accept "CIDR" format, e.g. 24, as its meta-data says.
But I think it's not hard to support the old format for the backward compatibility.

------ 21) DUMMY_IP+1, explicit invalid netmask bits (-1)
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=-1

Should do the better error checking.

Thanks for reporting!

@jnpkrn jnpkrn Low: test-findif: fix tests numbering
Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
c687e3f
Contributor

kskmori commented Oct 18, 2012

Hi

------ 4) DUMMY_IP+1, explicit netmask
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=255.255.255.0

The current findif.sh only accept "CIDR" format, e.g. 24, as its meta-data says.
But I think it's not hard to support the old format for the backward compatibility.

I found that it was not easy because ip command convert from dot-notations to CIDR format internally so we can not monitor/remove the ip address correctly.

As the meta-data already says that it only accept CIDR format, I think that we are not going to support dot-notations anymore.

------ 21) DUMMY_IP+1, explicit invalid netmask bits (-1)
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=-1

Should be fixed in this and I will submit another pull request for it later:

Contributor

jnpkrn commented Oct 23, 2012

I've found out there is a bug in test-findif.sh preventing correct
checks of values other than exit codes. Fix will be added soon
as well as a few other clean-ups.

On 18/10/12 02:23 -0700, Keisuke MORI wrote:

------ 4) DUMMY_IP+1, explicit netmask
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=255.255.255.0

The current findif.sh only accept "CIDR" format, e.g. 24, as its
meta-data says. But I think it's not hard to support the old
format for the backward compatibility.

I found that it was not easy because ip command convert from
dot-notations to CIDR format internally so we can not monitor/remove
the ip address correctly.

As the meta-data already says that it only accept CIDR format, I
think that we are not going to support dot-notations anymore.

I am totally OK with that, it was only trivial regression check
originally targetting findif binary only.

@jnpkrn jnpkrn Low: test-find: make it work properly, support IPv6, style...
Previously, the checks for values other than exit code were not
performed correctly.

The test now works exclusively with netmask form referred
to as CIDR (cidr_netmask).  Downside is that it is no longer
capable to exhibit wrong exit code of findif binary when
cidr_netmask is provided in IPv4-like notation but not in
a proper form (i.e., at least one dot is contained, but as
a whole, it is not a valid address), however this is
a deprecated usage and the issue was fixed anyway.

A very limited test set for IPv6 (targetting findif.sh)
was added.  Options such as --, -4, -6, -46 denotes
which built-in tests to use.  Additionally, one can add
ad-hoc tests definitions by piping them into the script
(for format see ./test-findif -h).

Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
25cd466
Contributor

jnpkrn commented Oct 24, 2012

Keisuke,

test-findif.sh switched completely to CIDR notation, but there are still
some regressions:

------  1) LO4_IP
OCF_RESKEY_ip=127.0.0.1 OCF_RESKEY_cidr_netmask= 
lo, 8, 
FAIL broadcast:  vs 127.255.255.255
[FAIL] 
------  2) LO4_IP+1
OCF_RESKEY_ip=127.0.0.2 OCF_RESKEY_cidr_netmask= 
lo, 8, 
FAIL broadcast:  vs 127.255.255.255
[FAIL] 
------ 10) DUMMY4_IP+1
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask= 
dummy0, 24, 
FAIL broadcast:  vs 198.51.100.255
[FAIL] 
------ 11) DUMMY4_IP+1, explicit netmask
OCF_RESKEY_ip=198.51.100.2 OCF_RESKEY_cidr_netmask=24
dummy0, 24, 
FAIL broadcast:  vs 198.51.100.255
[FAIL] 

This fails as findif.sh is unable to find broadcast parameter via ip
if the respective address is not added with "broadcast" parameter
(in Linux, Fedora 17). Findif binary was able to find out this based
on netmask and also IPaddr2 also declares:

If left empty, the script will determine this from the netmask.

This responsibility used to be left on findif, but findif.sh
does not support it.

For IPv6 tests, this one failed for me:

------ A0) LO6_IP
> test spec.: empty exp_bc
OCF_RESKEY_ip=::1 OCF_RESKEY_cidr_netmask= 
> FAIL exit code: 1 vs 0
2012/10/24_15:52:37 ERROR: Unable to find cidr_netmask.
[FAIL] 

The diagnostic message included.

Jan

Contributor

jnpkrn commented Oct 24, 2012

(oops, Markdown is not interpreted in the mails, wanted to make code blocks with outputs)

Contributor

jnpkrn commented Oct 24, 2012

On 24/10/12 17:13 +0200, Jan Pokorný wrote:

Findif binary was able to find out this based on netmask and also
IPaddr2 also declares:

If left empty, the script will determine this from the netmask.

This responsibility used to be left on findif, but findif.sh
does not support it.

Not saying that the current behavior is wrong. Perhaps it is more
desired -- broadcast would not work even if broadcast address was
computed explicitly as it is not configured at system level,
right?

This was meant to point out the conflict of (meta)documentation
and both implementations of findif.

Contributor

dmuhamedagic commented Oct 25, 2012

Jan, shall we merge this? Or do you want to add more commits?

Contributor

jnpkrn commented Oct 25, 2012

On 25/10/12 02:38 -0700, Dejan Muhamedagic wrote:

Jan, shall we merge this? Or do you want to add more commits?

Dejan, please go ahead, now it should be more stable :)
Any additions, comments, etc. are welcome, indeed.

The main aim of this (sort of) unit test now seems to be to validate
findif(.sh) design/documentation.
And the mentioned weak points actually question the design
-- should broadcast addresses be computed from IP+netmask if not
explicitly configured?, should typical localhost addresses be supported
or accurate error message should be emitted?, what if the setup is
so strange that such address is used network-wide (not sure if this
is legitimate)?, and more probably to come.

Contributor

kskmori commented Oct 26, 2012

Findif binary was able to find out this based on netmask and also
IPaddr2 also declares:

If left empty, the script will determine this from the netmask.

This responsibility used to be left on findif, but findif.sh
does not support it.

Not saying that the current behavior is wrong. Perhaps it is more
desired -- broadcast would not work even if broadcast address was
computed explicitly as it is not configured at system level,
right?

Right. The behavior of findif.sh should be desired than the old findif.
The old findif just guess broadcast by calculation, while findif.sh would follow to the system configuration.

This was meant to point out the conflict of (meta)documentation
and both implementations of findif.

Maybe we should rephrase the meta-data something like:

 Broadcast address associated with the IP. If left empty, the script will
-determine this from the netmask.
+try to determine this if broadcast is configured on the interface.

I think it would be great if you could include it along with your test cases update.

Contributor

kskmori commented Oct 26, 2012

For IPv6 tests, this one failed for me:

------ A0) LO6_IP
> test spec.: empty exp_bc
OCF_RESKEY_ip=::1 OCF_RESKEY_cidr_netmask= 
> FAIL exit code: 1 vs 0
2012/10/24_15:52:37 ERROR: Unable to find cidr_netmask.
[FAIL] 

I do not think that this test case makes sense.
Unlike IPv4, ::1/128 is always reserved and should be handled specially, and no subnet for IPv6 is pre-configured dedicated to loopback like 127.0.0.0/8.

If you want to add a test case for loopback, pick a regular IP address with nic=lo and an appropriate cidr_netmask parameters (or pre-configure a subnet for loopback). It should be same manner to a 'regular' interface.

Contributor

dmuhamedagic commented Nov 2, 2012

Merged. Sorry for the delay.

@dmuhamedagic dmuhamedagic added a commit that referenced this pull request Nov 2, 2012

@dmuhamedagic dmuhamedagic Merge pull request #154 from jnpkrn/test-findif
Low: test-findif: findif.sh+IPv6 support, portable, working...
5792c8b

@dmuhamedagic dmuhamedagic merged commit 5792c8b into ClusterLabs:master Nov 2, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment