mtu-migration-dispatcher script #2871
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
jcaamano
left a comment
jcaamano
left a comment
There was a problem hiding this comment.
In general I think that there is no need to have a phase2 which simplifies things quite a bit.
There was a problem hiding this comment.
We only need the default gateway interface in this case?
Now that I think of it, you might have added a phase2 thinking that MTU changes persist. But they do not. MTU set through iproute2 both on interfaces and routes do not persist accross reboots, so there no need for a phase2. |
The exception to this would be the MTU set in OVSDB but that would be reset anyway by however the admin chooses to deploy the actual target MTU config. |
ricky-rav
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Check for greater than?
|
/retest |
There was a problem hiding this comment.
This should be foundMTUMigrationWorker = ...
There was a problem hiding this comment.
why do you need to do this?
There was a problem hiding this comment.
oops, since we're exiting, there's indeed no need.
There was a problem hiding this comment.
get_mtu_on_dev returns 1 if it isnt found and this logic seems to continue. Could that cause potential issues with assumign MTU is 1? Should you return -1 or something instead and exit?
There was a problem hiding this comment.
get_mtu_on_dev gives back the result of cat ..... The return is just the status of the function that should fail the script if different than 0 because of set -x. But let's change it because it is confusing. Just doing an an exit 1 there should be equivalent.
There was a problem hiding this comment.
would be nice to set the values of the arguments up here rather than having to search to the bottom of the script to find it. Would also be nice to have some comment indicating what these arguments are
There was a problem hiding this comment.
where does CONNECTION_UUID come from?
There was a problem hiding this comment.
It's among the environment variables set by network manager when calling the dispatcher script. I will add a comment that clarifies that and the input arguments as well.
There was a problem hiding this comment.
this check should go under the if statement on line 90 and look like this:
if [ $ORIGINAL_MTU -gt $TARGET_MTU ]; then
if [ $is_vlan_parent -eq 1 ]; then
echo "No need to set MTU as current is greater than target and is a vlan parent"
return 0
fi
routable_mtu=$TARGET_MTU
mtu=$ORIGINAL_MTU
...
There was a problem hiding this comment.
you print this later so no need to print here
There was a problem hiding this comment.
minor nit that doesnt really matter but [0-9]+ (since if the route says mtu there should be at least 1 char here and should be a digit)
|
@ricky-rav: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
A new Network Manager dispatcher script, placed in mtu-migration-dispatcher.yaml, responds to pre-up events on selected host interfaces when an MTU migration is ongoing. It sets appropriate (transient) MTU values for host interfaces and their routes in the first phase of the MTU migration in order to avoid disruption while the cluster MTU is also being modified. Permanently applying an MTU value to host interfaces is instead under the cluster administrators's responsibility; an option for the cluster admin, for instance, would be to apply a MachineConfig object that adds a custom Network Manager profile to the host. Signed-off-by: Riccardo Ravaioli <[email protected]> Co-authored-by: Jaime Caamaño Ruiz <[email protected]>
|
/retest |
|
/assign sinnykumari |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ricky-rav, sinnykumari, trozet The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
5 participants
A new Network Manager dispatcher script, placed in mtu-migration-dispatcher.yaml, responds to UP events on selected host interfaces when an MTU migration is ongoing. It sets appropriate (transient) MTU values for host interfaces and their routes in the first phase of the MTU migration in order to avoid disruption while the cluster MTU is also being modified.
Permanently applying an MTU value to host interfaces is instead under the cluster administrators's responsibility; an option for the cluster admin, for instance, would be to apply a MachineConfig object that adds a custom Network Manager profile to the host.