Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Calico detection #523

Merged
merged 10 commits into from
Dec 22, 2017
Merged

Fix Calico detection #523

merged 10 commits into from
Dec 22, 2017

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Dec 11, 2017 •
edited

Make it compatible with CNI Genie

  • implement Calico detection based on Calico-specific routes
  • run e2e with weave and flannel for branches ending with -net
  • test with CNI Genie
  • clean up code & commits
  • add testing instructions

Testing instructions:

  1. Start k-d-c:
~/dind-cluster-v1.8.sh up
  1. Start CNI genie using https://raw.githubusercontent.com/Huawei-PaaS/CNI-Genie/master/conf/1.8/genie.yaml with the container image changed to lukaszo/cnigenie:env: 
    kubectl apply -f genie.yaml
  2. Deploy Calico: 
    kubectl apply -f https://docs.projectcalico.org/v2.6/getting-started/kubernetes/installation/hosted/kubeadm/1.6/calico.yaml
  3. Wait for Calico & Genie pods to become Running: 
    kubectl get pods -n kube-system -o wide -w
  4. Update CNI configuration to make it support bridge & calico with default gw via Calico only: 
    docker exec -i kube-node-1 /bin/bash -s <<"EOF"
set -u -e
jqm() {
    jq "$1" "$2" >/tmp/x
    mv /tmp/x "$2"
}
jqm ".cniVersion|=\"0.3.1\"" /etc/cni/net.d/00-genie.conf
mv /etc/cni/net.d/{cni.conf,20-bridge.conf}
jqm ".cniVersion|=\"0.3.0\"" /etc/cni/net.d/10-calico.conf
jqm "del(.ipam.gateway,.ipam.routes)|.cniVersion|=\"0.3.0\"" /etc/cni/net.d/20-bridge.conf
EOF
  5. Build and start Virtlet, wait for its pod to become Running: 
    build/cmd.sh build
build/cmd.sh copy-dind
FORCE_UPDATE_IMAGE=1 build/cmd.sh start-dind
kubectl get pods -n kube-system -o wide -w
  6. Start Ubuntu VM with cni: bridge,calico annotation added and wait for it to become Running: 
    kubectl convert --local -o json -f examples/ubuntu-vm.yaml |
  jq '.metadata.annotations.cni="bridge,calico"' |
  kubectl create -f -
kubectl get pods -o wide -w
  7. If Ubuntu pods starts correctly, it should get an IP address like 10.192.2.x, e.g. 10.192.2.3: 
    ubuntu-vm   1/1       Running   0         1m        10.192.2.3   kube-node-1
  8. Perform a quick check of Ubuntu VM: 
    examples/vmssh.sh testuser@ubuntu-vm \
  'ip a; ip r; curl http://kubernetes-dashboard.kube-system.svc.cluster.local; ping -c 1 8.8.8.8'
    This should give you output like this: 
    1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host
       valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 0a:58:0a:c0:02:03 brd ff:ff:ff:ff:ff:ff
    inet 10.192.2.3/16 brd 10.192.255.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::858:aff:fec0:203/64 scope link
       valid_lft forever preferred_lft forever
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether 3a:ac:e9:39:40:df brd ff:ff:ff:ff:ff:ff
    inet 192.168.135.129/24 brd 192.168.135.255 scope global eth1
       valid_lft forever preferred_lft forever
    inet6 fe80::38ac:e9ff:fe39:40df/64 scope link
       valid_lft forever preferred_lft forever
default via 192.168.135.130 dev eth1
10.192.0.0/16 dev eth0  proto kernel  scope link  src 10.192.2.3
192.168.135.0/24 dev eth1  proto kernel  scope link  src 192.168.135.129
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0 <!doctype html> <html ng-app="kubernetesDashboard"> <head> <meta charset="utf-8"> <title ng-controller="kdTitle as $ctrl" ng-bind="$ctrl.title()"></title> <link rel="icon" type="image/png" href="assets/images/kubernetes-logo.png"> <meta name="viewport" content="width=device-width"> <link rel="stylesheet" href="static/vendor.4f4b705f.css"> <link rel="stylesheet" href="static/app.93b90a74.css"> </head> <body> <!--[if lt IE 10]>
      <p class="browsehappy">You are using an <strong>outdated</strong> browser.
      Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your
      experience.</p>
    <![endif]--> <kd-chrome layout="column" layout-fill> </kd-chrome> <script src="static/vendor.6952e31e.js"></script> <script src="api/appConfig.json"></script> <script src="static/app.8a6b8127.js"></script> </body> </html> PING 8.8.8.8 (8.8.8.8) 56(84) bytes of data.
64 bytes from 8.8.8.8: icmp_seq=1 ttl=57 time=4.34 ms

--- 8.8.8.8 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 4.342/4.342/4.342/0.000 ms
100   848  100   848    0     0  95570      0 --:--:-- --:--:-- --:--:--  103k
  9. Set up CirrOS VM with bridge+calico: 
    kubectl convert --local -o json -f examples/cirros-vm.yaml |
  jq '.metadata.annotations.cni="bridge,calico"' |
  kubectl create -f -
kubectl get pods -o wide -w
  10. Activate udhcpc on eth1 which corresponds to Calico: 
    examples/vmssh.sh cirros@cirros-vm 'sudo /sbin/udhcpc -R -n -T 60 -i eth1 -s /sbin/cirros-dhcpc -O mtu -O staticroutes -x hostname cirros'
    This should produce the output like this: 
    udhcpc: started, v1.26.2
udhcpc: sending discover
udhcpc: sending select for 192.168.135.132
udhcpc: lease of 192.168.135.132 obtained, lease time 86400
    This step is needed because CirrOS' cloud-init can't set up the
    network properly.
  11. Verify the network inside CirrOS VM: 
    examples/vmssh.sh cirros@cirros-vm \
  '/sbin/ip a; /sbin/ip r; curl http://kubernetes-dashboard.kube-system.svc.cluster.local; ping -c 1 8.8.8.8'
    The output looks like this: 
    1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host
       valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether 0a:58:0a:c0:02:05 brd ff:ff:ff:ff:ff:ff
    inet 10.192.2.5/16 brd 10.192.255.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::858:aff:fec0:205/64 scope link
       valid_lft forever preferred_lft forever
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast qlen 1000
    link/ether da:9a:dc:8a:08:6d brd ff:ff:ff:ff:ff:ff
    inet 192.168.135.132/24 brd 192.168.135.255 scope global eth1
       valid_lft forever preferred_lft forever
    inet6 fe80::d89a:dcff:fe8a:86d/64 scope link
       valid_lft forever preferred_lft forever
default via 192.168.135.130 dev eth1
10.192.0.0/16 dev eth0  src 10.192.2.5
192.168.135.0/24 dev eth1  src 192.168.135.132
  % Total  <!doctype html> <html ng-app="kubernetesDashboard"> <head> <meta charset="utf-8"> <title ng-controller="kdTitle as $ctrl" ng-bind="$ctrl.title()"></title> <link rel="icon" type="image/png" href="assets/images/kubernetes-logo.png"> <meta name="viewport" content="width=device-width"> <link rel="stylesheet" href="static/vendor.4f4b705f.css"> <link rel="stylesheet" href="static/app.93b90a74.css"> </head> <body> <!--[if lt IE 10]>
      <p class="browsehappy">You are using an <strong>outdated</strong> browser.
      Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your
      experience.</p>
    <![endif]--> <kd-chrome layout="column" layout-fill> </kd-chrome> <script src="static/vendor.6952e31e.js"></script> <script src="api/appConfig.json"></script> <script src="static/app.8a6b8127.js"></script> </body> </html> PING 8.8.8.8 (8.8.8.8): 56 data bytes
64 bytes from 8.8.8.8: seq=0 ttl=57 time=4.325 ms

--- 8.8.8.8 ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 4.325/4.325/4.325 ms
   % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   848  100   848    0     0   131k      0 --:--:-- --:--:-- --:--:--  207k

This change is Reviewable

@ivan4th ivan4th force-pushed the ivan4th/fix-calico-detection-net branch from c49ac1c to 3c4527c Compare December 11, 2017 20:04
@ivan4th ivan4th changed the title Fix Calico detection [WiP] Fix Calico detection Dec 11, 2017
@jellonek
Copy link
Contributor

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

pkg/nettools/nettools.go, line 940 at r1 (raw file):

			return errors.New("IPv4 config was expected")
		}
		netConfig.IPs[n].Address.Mask = netmaskForCalico()

Why there is assumption that nth ip address is for nth interface?
It's probably true for all cni plugins which we tried, but e.g. if some plugins will be extended to support ipv4 and ipv6, or will return more than single ipv4 addresses for same interface - will disturb that order.
Maybe better would be to match it using ip address? Same ip address on multiple interfaces is imo less probable than more than one addresses on some interfaces in chain, before calico one.

So imo test in if above is incorrect assumption while test in prior if can also be incorrect (i'm not sure if plugin can configure interface without ip address returned in cni result, but can imagine such one with e.g. ipx configuration (i know, it's almost forgotten, but ...)).

There was also (on kubecon) discussion about interfaces configured by cni which does not have ip addresses, configured e.g. with MPLS but i'm not sure if they are now allowed in cni.

pkg/tapmanager/tapfdsource.go, line 114 at r1 (raw file):

func (s *TapFDSource) getDummyGateway() (net.IP, error) {
	if s.dummyGateway == nil {
		dummyResult, err := s.cniClient.GetDummyNetwork()

Is that adding a "fake" pod to calico on first attempt to spawn any pod with calico networking?
Is it removed at any time, or it should leak resources on calico side?

pkg/tapmanager/tapfdsource.go, line 118 at r1 (raw file):

			return nil, err
		}
		if len(dummyResult.IPs) != 1 {

So that should not work with multiple cni plugins behind e.g. cni genie? (But looks like that should work if there is only calico behind the genie)

Comments from Reviewable

@ivan4th ivan4th force-pushed the ivan4th/fix-calico-detection-net branch 5 times, most recently from b9b8b28 to 6d77370 Compare December 19, 2017 21:31
@ivan4th ivan4th changed the title [WiP] Fix Calico detection Fix Calico detection Dec 19, 2017
@jellonek
Copy link
Contributor

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

pkg/nettools/nettools.go, line 941 at r2 (raw file):

// namespace.
func FixCalicoNetworking(netConfig *cnicurrent.Result, getDummyNetwork func() (*cnicurrent.Result, string, error)) error {
	// linkNameMap := make(map[string]*cnicurrent.IPConfig)

commented code

pkg/nettools/nettools_test.go, line 40 at r2 (raw file):

	secondOuterHwAddr = "42:b5:b7:33:91:3e"
	dummyInnerHwAddr  = "42:a4:a6:22:80:30"
	dummyOuterHwAddr  = "42:b5:b7:33:91:3f"

It has the same value as outerHwAddr. idk if that can be problematic, but seems to be suspicious.

Comments from Reviewable

@ivan4th
Copy link
Contributor Author

ivan4th commented Dec 20, 2017

Reviewed 1 of 4 files at r1, 2 of 4 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

pkg/nettools/nettools.go, line 940 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

Why there is assumption that nth ip address is for nth interface?
It's probably true for all cni plugins which we tried, but e.g. if some plugins will be extended to support ipv4 and ipv6, or will return more than single ipv4 addresses for same interface - will disturb that order.
Maybe better would be to match it using ip address? Same ip address on multiple interfaces is imo less probable than more than one addresses on some interfaces in chain, before calico one.

So imo test in if above is incorrect assumption while test in prior if can also be incorrect (i'm not sure if plugin can configure interface without ip address returned in cni result, but can imagine such one with e.g. ipx configuration (i know, it's almost forgotten, but ...)).

There was also (on kubecon) discussion about interfaces configured by cni which does not have ip addresses, configured e.g. with MPLS but i'm not sure if they are now allowed in cni.

This is fixed. (no more index assumptions, dummy interface extraction revamped)

pkg/nettools/nettools.go, line 941 at r2 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

commented code

Removed.

pkg/nettools/nettools_test.go, line 40 at r2 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

It has the same value as outerHwAddr. idk if that can be problematic, but seems to be suspicious.

Fixed.

pkg/tapmanager/tapfdsource.go, line 114 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

Is that adding a "fake" pod to calico on first attempt to spawn any pod with calico networking?
Is it removed at any time, or it should leak resources on calico side?

This may leak, but only during Virtlet pod restarts. We can fix this later as it's not a critically serious leak, but requires some extra work on Virtlet resource GC.

pkg/tapmanager/tapfdsource.go, line 118 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

So that should not work with multiple cni plugins behind e.g. cni genie? (But looks like that should work if there is only calico behind the genie)

Fixed.

Comments from Reviewable

@jellonek
Copy link
Contributor

:lgtm:

Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

lukaszo
lukaszo reviewed Dec 22, 2017
View reviewed changes
pkg/cni/client.go Outdated
}

func (c *Client) AddSandboxToNetwork(podId, podName, podNs string) (*cnicurrent.Result, error) {
rtConf := c.cniRuntimeConf(podId, podName, podNs)
rtConf.Args = append(rtConf.Args, [2]string{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment that it's for genie only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. BTW, please use Reviewable for Virtlet reviews.

@ivan4th ivan4th force-pushed the ivan4th/fix-calico-detection-net branch from 92a53ad to 288efa7 Compare December 22, 2017 13:14
@jellonek
Copy link
Contributor

:lgtm:

Reviewed 1 of 2 files at r3, 2 of 2 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

@ivan4th
Copy link
Contributor Author

ivan4th commented Dec 22, 2017 •
edited

Re-verified the PR after resolving the merge conflict. It still works.

@lukaszo
Copy link
Contributor

lukaszo commented Dec 22, 2017

Reviewed 1 of 4 files at r1, 1 of 2 files at r3, 2 of 2 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

@lukaszo
Copy link
Contributor

lukaszo commented Dec 22, 2017

:lgtm:

Review status: all files reviewed at latest revision, 6 unresolved discussions.

Comments from Reviewable

@ivan4th ivan4th merged commit 4d88fa7 into master Dec 22, 2017
@ivan4th ivan4th deleted the ivan4th/fix-calico-detection-net branch December 22, 2017 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaszo lukaszo lukaszo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ivan4th @jellonek @lukaszo