Bump Maximum MTU to 65535

[noahmehl]

This is an updated PR based on the following closed PR:

https://github.com/joyent/smartos-live/pull/782

I have found the updated file and changed the max MTU to 65536.

[jlevon]

This particular changes looks OK to me.

But what about vmadm - do we definitely want that clamped at 9000 still?

And for Triton, what about napi's clamp at 9000? And sdc-api doc file in sdc-portolan

[noahmehl]

@jlevon I can't speak to vmadm, and I have already added an issue for sdc-napi: https://github.com/joyent/sdc-napi/issues/18 Either way, if the physical host can't have an MTU high enough (9216 in my test case), then there can't be any update to anything else. I will likely add a PR to sdc-napi for this, as I want the overlay network (or other dedicated networks) to be 9216, allowing for full 9000 mtu use depending on the guest/network.

[noahmehl]

@jlevon do you need anything further from me on this?

[jlevon]

I'd like either @rzezeski and/or @jasonbking to take a look at this

[noahmehl]

@jlevon thanks! @rzezeski and @jasonbking let me know if you have any questions :)

[rzezeski]

Sorry for the late reply. My inbox has too many github notifications these days and I missed this.

@jclulow's original response has an off-by-one error: the max for a 16-bit unsigned int is 65535. I don't think it needs to be limited to 16-bit (that would be the IP datagram max), but if we are going to go with that we might as well use the correct value. Besides, 16-bit should keep us good for quite a long time.

[noahmehl]

Sorry for the late reply. My inbox has too many github notifications these days and I missed this.

@jclulow's original response has an off-by-one error: the max for a 16-bit unsigned int is 65535. I don't think it needs to be limited to 16-bit (that would be the IP datagram max), but if we are going to go with that we might as well use the correct value. Besides, 16-bit should keep us good for quite a long time.

@rzezeski I have updated to 65535.

[jlevon]

Hi @noahmehl I tried out this latest version, and it fails to boot:

/lib/svc/method/net-physical[32]: .: syntax error at line 159: `~(E)^[0-9]{1,5}$ ]] ; then

[noahmehl]

@jlevon I'm actually at a loss as to why this would fail. I tried the function on a platform image and it doesn't fail in bash. My guess is that whatever sources this script isn't using bash?

[sjorge]

If I am not mistaken it is ksh

[noahmehl]

@sjorge I get the same error if I test that IF statement in KSH. I will review shortly.

[jlevon]

As you can see that's from net-physical itself during a headnode boot. So I'm a bit confused as to how you could be booting fine but not me.

But yes, that script itself is ksh93. I should have tested this previously.

The issue is that the regexp needs to be surrounded by single quotes (') Can you test yourself a version of that and push a fix, and I'll give it another round?

[noahmehl]

@jlevon it's my fault for not building a platform image and testing boot. I assumed incorrectly because the script has #!/bin/bash that was all that needed to be considered. I will make sure it works sourced from ksh93, and push an update.

[noahmehl]

@jlevon I'm actually at a loss as to why this would fail. I tried the function on a platform image and it doesn't fail in bash. My guess is that whatever sources this script isn't using bash?

This was confusing, what I mean by this is I tested via bash on a running system from a platform image.

[noahmehl]

Well, this creates another question: should we update the script declarative:

https://github.com/joyent/smartos-live/blob/21db1efe6ca4346c110039187886c2a9b1872a9e/src/lib/sdc/network.sh#L1

[jlevon]

Well, this creates another question: should we update the script declarative:

We should probably remove it - this is always included by a script, not a script itself.

We could also add a comment saying that functions should work under both ksh93 and bash.

[noahmehl]

@jlevon adding quotes does work under ksh93, but breaks under bash. I don't know if it's reasonable to have bash compatibility?

[noahmehl]

@jlevon adding quotes does work under ksh93, but breaks under bash. I don't know if it's reasonable to have bash compatibility?

We could use grep -E to return a value at all, and then use an if [ -z "$var" ];

[noahmehl]

@jlevon @rzezeski I have updated for this.

[jlevon]

Please don't force-push to a PR, it makes incremental reviews impossible. I filed this internally as OS-8059 I tested this latest set, and it all seems to work OK for me. So once @rzezeski OK's this newest version, I'll get it integrated.

[noahmehl]

@jlevon sorry about that, I tend to work on projects that always want us to rebase, rebase, rebase....

[noahmehl]

Thank you so much @rzezeski and @jlevon!!! Sorry it's been a crazy road to get this in!

[jlevon]

Ugh, sorry to do this, but I've just noticed a bug:

    if ((mtu > 65535 || $mtu < 1500 )); then

This should be $mtu

[noahmehl]

Ugh, sorry to do this, but I've just noticed a bug:

    if ((mtu > 65535 || $mtu < 1500 )); then

This should be $mtu

I'm sorry to have left that terrible typo in there! Updated :) @jlevon please review :)

[jlevon]

Thanks @noahmehl