Make calico MTU configurable in bootstrap config #2677

TeddyAndrieux · 2020-07-23T18:05:56Z

Component:

'salt', 'kubernetes'

Context:

Summary:

Allow multiple CIDRs for workload plane and control plane networks
Make calico MTU configurable through bootstrap config
Check MTU defined is smaller than the one on the local workload network interface

New bootstrap config network layout (still backward compatible with old style)

apiVersion: metalk8s.scality.com/v1alpha3
networks:
  workloadPlane:
    cidr:
      - 10.200.10.0/24
      - 10.200.20.0/24
    mtu: 4242
  controlPlane:
    cidr: 10.200.30.0/24

TODO:

Add unit test for get_mtu_from_ip
Add unit test for get_ip_from_cidrs
Update documentation for BootstrapConfig changes
Update tests (and vagrant) to use the new BootstrapConfig style
Add documentation about MTU configuration in bootstrap config
Add changelog entry

Fixes: #1095

bert-e · 2020-07-23T18:05:58Z

Hello teddyandrieux,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2020-07-23T18:06:03Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

slaperche-scality · 2020-07-23T18:27:30Z

salt/_modules/metalk8s_network.py

@@ -79,3 +79,26 @@ def get_oidc_service_ip():
    range.
    '''
    return _pick_nth_service_ip(OIDC_ADDRESS_NUMBER)
+
+
+def get_mtu_from_ip(ip):


IP or CIDR?

Reading the doc, I would say CIDR.

Not really the doc is wrong here it's not a cidr but an IP (maybe not full)

(And btw it's a bit bugged if you not specify the full IP because he consider 0 as wildcard)

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000 link/ether fa:16:3e:2b:fd:d5 brd ff:ff:ff:ff:ff:ff inet 10.200.2.69/16 brd 10.200.255.255 scope global dynamic eth0 valid_lft 71176sec preferred_lft 71176sec inet6 fe80::f816:3eff:fe2b:fdd5/64 scope link valid_lft forever preferred_lft forever 3: tunl0@NONE: <NOARP,UP,LOWER_UP> mtu 1440 qdisc noqueue state UNKNOWN group default qlen 1000 link/ipip 0.0.0.0 brd 0.0.0.0 inet 10.233.132.64/32 brd 10.233.132.64 scope global tunl0 valid_lft forever preferred_lft forever [root@bootstrap ~]# salt-call network.ifacestartswith 10.200.0.0/16 local: [root@bootstrap ~]# salt-call network.ifacestartswith 10.200 local: - tunl0 - eth0 [root@bootstrap ~]# salt-call network.ifacestartswith 10.200. local: - eth0

gdemonet · 2020-07-24T09:11:34Z

salt/metalk8s/kubernetes/cni/calico/configured.sls

+{#- Check that service MTU configured is inferior to the local workload interface one #}
+{%- set workload_local_mtu = salt.metalk8s_network.get_mtu_from_ip(grains.metalk8s.workload_plane_ip) %}
+{%- if pillar['networks']['mtu'] > workload_local_mtu %}
+  {{ raise('Trying to configure CNI with ' ~ pillar['metalk8s']['networks']['mtu']
+           ~ 'MTU but local workload interface MTU is inferior ' ~ workload_local_mtu) }}
+{%- endif %}


Neat! Do you think we can add a similar check earlier in the bootstrap/deploy_node orchestrate?

Maybe consider adding these checks in metalk8s.internal.preflight?

Yes I could add check earlier (maybe not in the preflight because to me preflight should be removed at some point, since it does not bring any value)

I will try to find a better place earlier I agree it fail a bit late here (at least for bootstrap because for new node it will fail when rendering the "bring new node to highstate" so not really late I think)

Could be done later, and as you said, since we may want to do some refactoring around the pre-checks, would make sense to add as a task in #1999

salt/_pillar/metalk8s.py

salt/metalk8s/kubernetes/cni/calico/configured.sls

bert-e · 2020-07-28T08:18:44Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

gdemonet

Some minor comments, but overall behaviour and user-facing features LGTM!

salt/_modules/metalk8s_network.py

gdemonet · 2020-07-30T06:17:20Z

salt/_pillar/metalk8s.py

@@ -15,6 +15,7 @@

 DEFAULT_POD_NETWORK = '10.233.0.0/16'
 DEFAULT_SERVICE_NETWORK = '10.96.0.0/12'
+DEFAULT_MTU = 1460


That's the default Calico uses based on GCP. Should we keep it (to smooth upgrades), or find one that better suits our use cases? (for on-prem, I don't what the lowest reasonable MTU we'd find, but maybe we could start by considering the ones on Scality Cloud - 1500 apparently)

Don't really know, but to me it's not really part of this PR, but indeed it's a good question

Let's keep it for now, shouldn't hurt unless we start deploying in envs where this default value is too low.

tests/install/steps/test_expansion.py

gdemonet

LGTM!

CHANGELOG.md

gdemonet

Perfect! Thanks, I guess we just need to create some tracking tickets for open questions:

how to get the MTU checks "standalone" so we can run them earlier
what default MTU should we use
what's the impact of having multiple CIDRs (manual + automated tests to consider)

No need to provide much details, just get a title, link to this PR, and the state:question label and we'll get back to them later.

NicolasT · 2020-07-31T08:32:29Z

Something to keep in mind: when multiple CIDRs are used, you almost certainly require tunneling (IPIP/VXLAN) to be enabled! (Unless Calico is configured to peer with routers on the data path etc. which is rather unlikely in our deployments).

bert-e · 2020-07-31T11:01:30Z

Build failed

The build for commit did not succeed in branch improvement/GH-1095-make-calico-mtu-configurable.

The following options are set: approve

bert-e · 2020-07-31T12:56:51Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

gdemonet

Minor wording comments, and we should be good to go!

docs/installation/advanced-guide.rst

docs/installation/bootstrap.rst

bert-e · 2020-07-31T14:43:39Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@gdemonet

Before this commit we use networks pillar to compute the salt-api address in salt-api tests, but it's wrong we should directly use the MetalK8s grains containing the control plane IP we want. Use this grains instead of computing the address from CIDR in the pillar

Add a module function to retrieve the first IP available from several CIDRs with an optionnal argument for `current_ip` so that we return this current_ip if this IP still available in one of the provided CIDRs

Add unit tests for `get_ip_from_cidrs` function from `metalk8s_network` salt custom module

Add a function in `metalk8s_network` salt module to retrieve MTU configured on an interface from an IP (will be needed to check MTU on workload plane network before configuring calico #1095)

Add unit tests for `get_mtu_from_ip` function from `metalk8s_network` salt custom module

In metalk8s we may have node not in the same L2 networks so that some routing was needed to communicate between node and it's perfectly find just we need to allow multiple CIDRs for workload plane and control plane networks. Now in bootstrap config you can provide a list of CIDRs for this 2 networks. Bump apiVersion of bootstrap config as the layout of the file changed

Add a utils in tests to get a pillar key with salt

In expansions test we need to have the CIDR of the control-plane network to create the new nodes, instead of retrieving the CIDR from bootstrap config directly, retrieve it from the pillar. NOTE: In expansions test we expect to have only one CIDR for control plane network

Use the new version `v1alpha3` for BootstrapConfiguration file generated with vagrant

Use the new version `v1alpha3` for BootstrapConfiguration file generated by openstack worker for multiple-nodes

Use the new version `v1alpha3` for BootstrapConfiguration file generated by openstack worker for single-node rhel

BootstrapConfig layout just changed, this commit update the documentation according to this Bootstrap config changes

Add a `mtu` key in bootstrap configuration to configure the value for MTU on Pod network, this value is used by calico to create the interface for Pod network. NOTE: We set the calico MTU to the workload MTU - 20 so that if IPinIP is enabled the MTU is still valid Fixes: #1095

Add a small paragraph about MTU value that may be provided for workloadPlane network in BootstrapConfiguration Refs: #1095

Add a changelog entry about the ability to use multiple CIDRs for workload and control plane networks and to configure the MTU used by Calico Refs: #1095

bert-e · 2020-07-31T15:20:05Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

TeddyAndrieux · 2020-07-31T15:21:02Z

/approve

bert-e · 2020-07-31T16:19:32Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/2.6

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2020-07-31T16:19:38Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/2.6

The following branches have NOT changed:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5

Please check the status of the associated issue GH-1095.

Goodbye teddyandrieux.

TeddyAndrieux requested a review from a team July 23, 2020 18:05

slaperche-scality reviewed Jul 23, 2020

View reviewed changes

gdemonet reviewed Jul 24, 2020

View reviewed changes

NicolasT reviewed Jul 24, 2020

View reviewed changes

salt/_pillar/metalk8s.py Outdated Show resolved Hide resolved

NicolasT reviewed Jul 24, 2020

View reviewed changes

salt/metalk8s/kubernetes/cni/calico/configured.sls Outdated Show resolved Hide resolved

TeddyAndrieux force-pushed the improvement/GH-1095-make-calico-mtu-configurable branch from bb3674e to de68d1d Compare July 28, 2020 08:18

TeddyAndrieux force-pushed the improvement/GH-1095-make-calico-mtu-configurable branch 9 times, most recently from 3a68daf to 58f2755 Compare July 29, 2020 14:11

gdemonet reviewed Jul 30, 2020

View reviewed changes

TeddyAndrieux force-pushed the improvement/GH-1095-make-calico-mtu-configurable branch 2 times, most recently from 53a31d1 to 6b0e6b7 Compare July 30, 2020 09:16

TeddyAndrieux marked this pull request as ready for review July 30, 2020 11:22

gdemonet previously approved these changes Jul 30, 2020

View reviewed changes

TeddyAndrieux dismissed gdemonet’s stale review via 15b657c July 30, 2020 12:12

gdemonet reviewed Jul 31, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

TeddyAndrieux force-pushed the improvement/GH-1095-make-calico-mtu-configurable branch from 15b657c to 60877dc Compare July 31, 2020 08:25

gdemonet previously approved these changes Jul 31, 2020

View reviewed changes

TeddyAndrieux dismissed gdemonet’s stale review via 2275742 July 31, 2020 14:06

TeddyAndrieux force-pushed the improvement/GH-1095-make-calico-mtu-configurable branch from 60877dc to 2275742 Compare July 31, 2020 14:06

gdemonet suggested changes Jul 31, 2020

View reviewed changes

docs/installation/advanced-guide.rst Outdated Show resolved Hide resolved

docs/installation/bootstrap.rst Outdated Show resolved Hide resolved

docs/installation/bootstrap.rst Outdated Show resolved Hide resolved

TeddyAndrieux added 15 commits July 31, 2020 17:08

salt: Add module function to get the first IP available

f0bb94a

Add a module function to retrieve the first IP available from several CIDRs with an optionnal argument for `current_ip` so that we return this current_ip if this IP still available in one of the provided CIDRs

salt/tests: Add unit tests for get_ip_from_cidrs salt function

bd8bcbb

Add unit tests for `get_ip_from_cidrs` function from `metalk8s_network` salt custom module

salt: Add get_mtu_from_ip function in `metalk8s_network

ca8397f

Add a function in `metalk8s_network` salt module to retrieve MTU configured on an interface from an IP (will be needed to check MTU on workload plane network before configuring calico #1095)

salt/tests: Add unit tests for get_mtu_from_ip salt function

48d07b6

Add unit tests for `get_mtu_from_ip` function from `metalk8s_network` salt custom module

tests: Add utils to get pillar key

6da6e5d

Add a utils in tests to get a pillar key with salt

vagrant: Use v1alpha3 version of BootstrapConfiguration file

801d8c0

Use the new version `v1alpha3` for BootstrapConfiguration file generated with vagrant

eve: Use v1alpha3 version of BootstrapConfiguration file

96a5a64

Use the new version `v1alpha3` for BootstrapConfiguration file generated by openstack worker for multiple-nodes

eve: Use v1alpha3 version of BootstrapConfiguration file

bc735c4

Use the new version `v1alpha3` for BootstrapConfiguration file generated by openstack worker for single-node rhel

docs: Update documentation about BootstrapConfig networks

95a1596

BootstrapConfig layout just changed, this commit update the documentation according to this Bootstrap config changes

docs: Add documentation about workloadPlane MTU configuration

d964cc5

Add a small paragraph about MTU value that may be provided for workloadPlane network in BootstrapConfiguration Refs: #1095

changelog: Add entry about multiple CIDRs support configurable MTU

25193fe

Add a changelog entry about the ability to use multiple CIDRs for workload and control plane networks and to configure the MTU used by Calico Refs: #1095

TeddyAndrieux force-pushed the improvement/GH-1095-make-calico-mtu-configurable branch from 2275742 to 25193fe Compare July 31, 2020 15:08

TeddyAndrieux requested a review from gdemonet July 31, 2020 15:10

gdemonet approved these changes Jul 31, 2020

View reviewed changes

bert-e merged commit 25193fe into development/2.6 Jul 31, 2020

bert-e deleted the improvement/GH-1095-make-calico-mtu-configurable branch July 31, 2020 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make calico MTU configurable in bootstrap config #2677

Make calico MTU configurable in bootstrap config #2677

TeddyAndrieux commented Jul 23, 2020 •

edited

Loading

bert-e commented Jul 23, 2020

bert-e commented Jul 23, 2020

slaperche-scality Jul 23, 2020

TeddyAndrieux Jul 24, 2020

gdemonet Jul 24, 2020

TeddyAndrieux Jul 24, 2020

gdemonet Jul 30, 2020

bert-e commented Jul 28, 2020

gdemonet left a comment

gdemonet Jul 30, 2020

TeddyAndrieux Jul 30, 2020

gdemonet Jul 30, 2020

gdemonet left a comment

gdemonet left a comment

NicolasT commented Jul 31, 2020

bert-e commented Jul 31, 2020

bert-e commented Jul 31, 2020

gdemonet left a comment

bert-e commented Jul 31, 2020

bert-e commented Jul 31, 2020

TeddyAndrieux commented Jul 31, 2020

bert-e commented Jul 31, 2020

bert-e commented Jul 31, 2020

Make calico MTU configurable in bootstrap config #2677

Make calico MTU configurable in bootstrap config #2677

Conversation

TeddyAndrieux commented Jul 23, 2020 • edited Loading

bert-e commented Jul 23, 2020

Hello teddyandrieux,

bert-e commented Jul 23, 2020

Waiting for approval

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bert-e commented Jul 28, 2020

Waiting for approval

gdemonet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdemonet left a comment

Choose a reason for hiding this comment

gdemonet left a comment

Choose a reason for hiding this comment

NicolasT commented Jul 31, 2020

bert-e commented Jul 31, 2020

Build failed

bert-e commented Jul 31, 2020

Waiting for approval

gdemonet left a comment

Choose a reason for hiding this comment

bert-e commented Jul 31, 2020

Waiting for approval

bert-e commented Jul 31, 2020

Waiting for approval

TeddyAndrieux commented Jul 31, 2020

bert-e commented Jul 31, 2020

In the queue

bert-e commented Jul 31, 2020

TeddyAndrieux commented Jul 23, 2020 •

edited

Loading