Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign #223

duglin · 2016-11-28T15:02:27Z

Now that projectcalico/libcalico-go#271 is merged.

Bump refs to libcalico-go too

Add pod annotations to enable them:

    spec:
      template:
        metadata:
          annotations:
            cni.calico/ipv4pools: [<ip-pool-1>, <ip-pool-2>]
            cni.calico/ipv6pools: [<ip-pool-1>, <ip-pool-2>]

Signed-off-by: Doug Davis [email protected]

caseydavenport · 2016-11-28T18:20:01Z

@tomdee I don't think this is a blocker for v2.0.0 since it is a feature add.

duglin · 2016-11-29T14:39:10Z

Marking this as WIP because I added support for hooking this into K8s via annotation and I haven't had a chance to test it yet. However, if people could look at the code in k8s/k8s.go to see if it looks right I'd appreciate it.

duglin · 2016-12-02T13:16:10Z

ok I think this is ready for some eyes on it

tomdee

Overall, looks good to me. We're about to do a calico-CNI release that I don't think can include this, but we should be able to merge soon after.

Ideally, we would have tests covering the k8s code too. I'm working on a framework for the k8s testing, but it's not ready enough to share yet. If I can get it out in the next few days then I might ask you to write tests for the k8s side too.

tomdee · 2016-12-06T17:55:11Z

calico_cni_ipam_test.go

+			          "ipam": {
+			            "type": "%s",
+			            "assign_ipv4": "true",
+						"ipv4_pools": [ "192.168.0.0/16" ]


tomdee · 2016-12-06T17:56:27Z

calico_cni_ipam_test.go

+						"ipv4_pools": [ "192.168.0.0/16" ]
+			          }
+			        }`, os.Getenv("ETCD_IP"), plugin)
+				_, _, _, _, _, _, err := CreateContainer(netconf)


This should assert on something more than there just not being an error. This test case would have passed before you added any other code so it's not really testing anything.

tomdee · 2016-12-06T17:56:54Z

calico_cni_ipam_test.go

+				if err != nil {
+					fmt.Printf("Session Err: %v\n", string(session.Err.Contents()))
+				}
+				Expect(err).ShouldNot(HaveOccurred())


Again, I would like to see more assertions here

tomdee · 2016-12-06T18:00:59Z

utils/utils.go

+			return nil, fmt.Errorf("%q isn't a IPv4 address", ip)
+		}
+		if !isv4 && ip.To4() != nil {
+			return nil, fmt.Errorf("%q isn't a IPv16 address", ip)


v16? Now there's a very futuristic IP protocol!

I think that's called future proofing. :-)
Fixed.

tomdee · 2016-12-06T18:40:24Z

k8s/k8s.go

+				utils.ReleaseIPAllocation(logger, conf.IPAM.Type, args.StdinData)
+				return nil, err
+			}
+			logger.WithField("labels", labels).Info("Fetched K8s labels")


If there are a large number of labels or annotations this could result is many large logs - I wonder if debug would be a better level?

I can't expect there would be that many to cause an issue, but switched anyway.

cmluciano · 2016-12-06T18:56:37Z

I can work with @duglin to write some tests for k8tes if you can share your framework.

duglin · 2016-12-07T02:28:19Z

@tomdee comments addressed and rebased

tomdee · 2016-12-08T22:14:43Z

@duglin / @cmluciano I've just up a PR for an initial k8s test #235

Hopefully I can get it tidied/merged soon, then you'll be able to add test(s) for your new and updated code paths.

tomdee · 2016-12-08T22:17:02Z

Looks like there are some conflicts in your glide.lock

duglin · 2016-12-08T22:20:48Z

hmm, git and github aren't complaining - where are you seeing that?

tomdee · 2016-12-12T13:00:23Z

Hmmm, maybe I was dreaming it. It looks OK now.

tomdee · 2016-12-16T11:47:26Z

@duglin / @cmluciano - My k8s test framework changes are merged, so you should be able to add a test for your code now. I expect you to be able to add a test that's similar to this - https://github.com/projectcalico/calico-cni/blob/master/calico_cni_k8s_test.go#L77 - but to vary the annotations on the pod to hit your code.

duglin · 2016-12-16T12:01:16Z

@tomdee thanks! Still need to catch-up on the other thread too about possibly changing what's passed in.....

duglin · 2017-01-03T15:53:32Z

@tomdee I'm running into a problem. I added the start of a k8s test, following your guidance - thanks. But, when trying to test it I'm not seeing what I expected to see. In particular, my annotations seemed to have no effect, so I started to debug it and got to the point where I added a panic() at https://github.com/projectcalico/cni-plugin/pull/223/files#diff-b396ca928d5bd0e161acd283b56ead9fR114 just to see if that if-statement was being called at all - and I don't think it is. In fact, it doesn't look like that code in k8s/k8s.go is called at all. Another interesting thing is that when I print out the Pod we get back from the Pod.Create() call, all of the IP fields are empty. Does the testing infrastructure invoke k8s and the calico stuff or does it fake some of it?

duglin · 2017-01-03T16:08:33Z

Oh wait - it could be timing thing.... checking....

duglin · 2017-01-03T16:36:48Z

ok - it may be a timing issue but I never see the Pod leave the "Pending" state. When I look at the docker containers running I see the API server and etcd. Shouldn't there be others running - like the kubelet?

caseydavenport · 2017-01-09T21:38:39Z

cni.calico/ipv4pools: [<ip-pool-1>, <ip-pool-2>]

One thought - Kubernetes best practice indicates that annotations should be part of a domain owned by the project, so this should probably be cni.projectcalico.org/ipv4pools, or maybe even ipam.cni.projectcalico.org/ipv4pool

@tomdee WDYT?

tomdee · 2017-01-13T23:12:18Z

@duglin The k8s testing that I added is quite "interesting". the kubelet doesn't actually invoke the CNI plugin, we still do that manually. In fact, there's not even a kubelet running. However, the test framework does run a kubernetes API server, and manually injects the podspec into it. I think this shouil dbe enough to still enable some useful testing. It does explain why you see pods stuck in pending state though, since there's nothing to actually run the pod, they'll never move out of pending.

tomdee · 2017-01-13T23:17:23Z

@caseydavenport do you have a link to the k8s best practices. I can't see anything relevant about annotations in https://kubernetes.io/docs/user-guide/config-best-practices/

In general, I agree with your suggestion of putting the domain in the annotation though. Do you know if it's "normal" to reverse it for k8s annotations though (e.g. org.projectcalico.abc)

tomdee

Some nits and a hint on why updating the test might not be working.

tomdee · 2017-01-13T23:18:39Z

calico_cni_k8s_test.go

@@ -53,11 +53,10 @@ var _ = Describe("CalicoCni", func() {
 			    "subnet": "10.0.0.0/8"
 			  },
 				"kubernetes": {
-					        "k8s_api_root": "http://127.0.0.1:8080"
-


Broken indent

fixed, I hope. Indentation in backticks is gonna kill me ;-)

tomdee · 2017-01-13T23:18:43Z

calico_cni_k8s_test.go

 				},
 				"policy": {"type": "k8s"},
-	"log_level":"info"


Broken indent

"log_level" looks ok to me, but I'll recheck after the next push

tomdee · 2017-01-13T23:19:56Z

calico_cni_k8s_test.go

+					ObjectMeta: v1.ObjectMeta{
+						Name: name2,
+						Annotations: map[string]string{
+							"cni.calico/ipv4pools": "192.169.1.0/24",


This annotation won't be used since you're calling CreateContainer below with name not name2

tomdee · 2017-01-13T23:20:11Z

glide.lock

@@ -1,5 +1,5 @@
 hash: 1efa10f7d9f9793128d78496760c641fb6ee82b2686a4fd476ba83a624f1506c
-updated: 2016-12-13T15:51:57.719804486-08:00


Do you really need to update the glide.lock?

I think I had to update the glide.yaml at one point, which forced the lock file to change. But I think that requirement is now behind me so yea I'll try to remove this from the PR.

caseydavenport · 2017-01-13T23:28:31Z

@tomdee Interestingly, neither can I. However, this section on label syntax does mention the format that I was thinking of: https://kubernetes.io/docs/user-guide/labels/#syntax-and-character-set

It's easy get a picture of the standard notation from other well-known annotations, e.g:

scheduler.alpha.kubernetes.io/critical-pod:
scheduler.alpha.kubernetes.io/tolerations:
net.alpha.kubernetes.io/network-policy:

See this comment for the annotation design: https://github.com/projectcalico/cni-plugin/issues/213#issuecomment-272053112

Now that projectcalico/libcalico-go#271 is merged. Bump refs to libcalico-go too Signed-off-by: Doug Davis <[email protected]>

Add pod annotations to enable them: ``` spec: template: metadata: annotations: cni.calico/ipv4pools: [<ip-pool-1>, <ip-pool-2>] cni.calico/ipv6pools: [<ip-pool-1>, <ip-pool-2>] ``` Signed-off-by: Doug Davis <[email protected]>

Signed-off-by: Doug Davis <[email protected]>

duglin · 2017-01-15T13:55:24Z

re: annotations and forward/reverse : I've seen it both ways in Kube, and been told to do it both ways too. For now I've picked ipam.cni.projectcalico.org/... let me know if you want it changed.

@tomdee do you have some sample code I can use as a guide for how to run the CNI plug-in manually in the k8s testing setup? In the meantime, I've disable the code that waits for the pod to get into the right state, so the test will fail for now.

Signed-off-by: Doug Davis <[email protected]>

duglin · 2017-01-15T15:28:39Z

calico_cni_k8s_test.go

@@ -83,6 +85,7 @@ var _ = Describe("CalicoCni", func() {
 					panic(err)
 				}
 				containerID, netnspath, session, contVeth, contAddresses, contRoutes, err := CreateContainer(netconf, name)


@tomdee this CreateContainer() call is confusing me. Didn't the call to clientset.Pods().Create() create the Pod and the underlying container already?

clientset.Pods().Create() just creates the API object in the kubernetes API server,
CreateContainer() actually invokes the CNI plugin binary directly. It's NOT the kublet that invokes. This test code invokes it directly passing in similar information to what kubernetes would.

caseydavenport · 2017-01-25T17:07:25Z

@tomdee @duglin what do we need to do to get this guy merged?

duglin · 2017-01-25T17:12:20Z

first, let me apologize for this looking like its lingering... I've just been swamped. I think the outstanding issue here is the testing side of things. I think I have the basic structure there, but I've not be able to get the CNI code to be invoked to verify my code works. I'll try to find some time over the next day or so to see if I can push this over the finish line.

caseydavenport · 2017-01-25T17:17:12Z

first, let me apologize for this looking like its lingering... I've just been swamped

No worries, we're pretty swamped too :)

Just want to make sure you're not waiting on us for something and that if you are, we prioritize unblocking you.

duglin · 2017-01-25T17:18:51Z

I don't think I'm blocked on anything other than a possible lack of education on how to get the CNI code invoked, but before I cry for more help I feel I need to play around with it some more first.

ozdanborne · 2017-01-30T22:03:45Z

Are there any other docs changes planned for this feature besides the ones here: projectcalico/calico#275

Do there need to be? Is there a Kubernetes use case this was added for?

tomdee · 2017-01-30T23:09:11Z

@djosborne I'm not aware of any plans and I don't think we'd ask @duglin to add any more. I can see that this would be a great feature to add more docs for though so maybe we should raise a docs issue to track doing that?

gunjan5

few more comments

gunjan5 · 2017-02-01T00:22:44Z

k8s/k8s.go

+			labels, annot, err = getK8sLabelsAnnotations(client, k8sArgs)
+			if err != nil {
+				// Cleanup IP allocation and return the error.
+				utils.ReleaseIPAllocation(logger, conf.IPAM.Type, args.StdinData)


Why do we need to ReleaseIP here? ADD happens later so not sure I see why we need to release IP here.

gunjan5 · 2017-02-01T00:24:48Z

k8s/k8s.go

+		// if the policy type has been set to "k8s". This allows users to
+		// run the plugin under Kubernetes without needing it to access the
+		// Kubernetes API
+		if conf.Policy.PolicyType == "k8s" {


I think we should also add a check conf.IPAM.Type == "calico-ipam" here to make sure this code only gets called for calico-ipam so it doesn't set fields that are not relevant to other IPAMs.

gunjan5 · 2017-02-01T00:25:12Z

k8s/k8s.go

+			if len(v4pools) != 0 || len(v6pools) != 0 {
+				var stdinData map[string]interface{}
+				if err := json.Unmarshal(args.StdinData, &stdinData); err != nil {
+					utils.ReleaseIPAllocation(logger, conf.IPAM.Type, args.StdinData)


same as above. Not sure if we need to release IP here

gunjan5 · 2017-02-01T00:25:20Z

k8s/k8s.go

+				}
+				newData, err := json.Marshal(stdinData)
+				if err != nil {
+					utils.ReleaseIPAllocation(logger, conf.IPAM.Type, args.StdinData)


gunjan5

I think pools won't get assigned to the right fields in stdinData since the string ID doesn't match the json field ID

gunjan5 · 2017-02-01T01:37:01Z

k8s/k8s.go

+					utils.ReleaseIPAllocation(logger, conf.IPAM.Type, args.StdinData)
+					return nil, err
+				}
+				stdinData["ipam"].(map[string]interface{})["ipv4pools"] = v4pools


shouldn't this be ipv4_pools instead of ipv4pools since NetConf's IPv4Pools field processes json by the string ipv4_pools

`IPv4Pools []string `json:"ipv4_pools"`

Same with ipv6pools in the line below.

gunjan5 · 2017-02-07T02:13:38Z

Changes moved to #260 . Closing this

duglin mentioned this pull request Nov 28, 2016

Document feature: "pool selection via CNI conf" projectcalico/calico#275

Closed

caseydavenport assigned tomdee Nov 28, 2016

caseydavenport added kind/enhancement priority/P2 labels Nov 28, 2016

duglin changed the title ~~Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign~~ [WIP] Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign Nov 29, 2016

duglin changed the title ~~[WIP] Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign~~ Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign Dec 2, 2016

tomdee suggested changes Dec 6, 2016

View reviewed changes

duglin force-pushed the Issue152 branch 3 times, most recently from 316a9d8 to 13c7e33 Compare December 7, 2016 02:25

duglin force-pushed the Issue152 branch from 13c7e33 to 0b3ad03 Compare December 7, 2016 03:18

duglin force-pushed the Issue152 branch from 0b3ad03 to a98b3a7 Compare December 8, 2016 22:20

tomdee removed their assignment Dec 16, 2016

duglin force-pushed the Issue152 branch 2 times, most recently from 4557b36 to bd079d8 Compare January 2, 2017 18:29

duglin force-pushed the Issue152 branch from 5ca93ee to 0d6d1b7 Compare January 3, 2017 15:54

tomdee suggested changes Jan 13, 2017

View reviewed changes

Doug Davis added 3 commits January 15, 2017 05:11

Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign

7c0e2af

Now that projectcalico/libcalico-go#271 is merged. Bump refs to libcalico-go too Signed-off-by: Doug Davis <[email protected]>

Add support for pools

6a26748

Add pod annotations to enable them: ``` spec: template: metadata: annotations: cni.calico/ipv4pools: [<ip-pool-1>, <ip-pool-2>] cni.calico/ipv6pools: [<ip-pool-1>, <ip-pool-2>] ``` Signed-off-by: Doug Davis <[email protected]>

Add a k8s test

56b5ea7

Signed-off-by: Doug Davis <[email protected]>

duglin force-pushed the Issue152 branch 3 times, most recently from e8b769d to 04c2cfb Compare January 15, 2017 13:38

duglin force-pushed the Issue152 branch from 04c2cfb to ec7a6d3 Compare January 15, 2017 15:05

addressing comments

a9584f6

Signed-off-by: Doug Davis <[email protected]>

duglin force-pushed the Issue152 branch from ec7a6d3 to a9584f6 Compare January 15, 2017 15:26

duglin commented Jan 15, 2017

View reviewed changes

gunjan5 added this to the Calico v2.1.0 milestone Jan 19, 2017

ozdanborne mentioned this pull request Jan 31, 2017

Document cni conf pool selection projectcalico/calico#473

Closed

gunjan5 reviewed Feb 1, 2017

View reviewed changes

gunjan5 suggested changes Feb 1, 2017

View reviewed changes

fasaxc assigned tomdee Feb 2, 2017

gunjan5 mentioned this pull request Feb 2, 2017

Pool selection via CNI conf (and k8s annotations) #260

Merged

gunjan5 closed this Feb 7, 2017

		@@ -1,5 +1,5 @@
		hash: 1efa10f7d9f9793128d78496760c641fb6ee82b2686a4fd476ba83a624f1506c
		updated: 2016-12-13T15:51:57.719804486-08:00

Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign #223

Add support for the new IPv4Pools and IPv6Pools properties on AutoAssign #223

Conversation

duglin commented Nov 28, 2016 • edited Loading

caseydavenport commented Nov 28, 2016

duglin commented Nov 29, 2016

duglin commented Dec 2, 2016

tomdee 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

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

cmluciano commented Dec 6, 2016

duglin commented Dec 7, 2016

tomdee commented Dec 8, 2016

tomdee commented Dec 8, 2016

duglin commented Dec 8, 2016

tomdee commented Dec 12, 2016

tomdee commented Dec 16, 2016

duglin commented Dec 16, 2016

duglin commented Jan 3, 2017

duglin commented Jan 3, 2017

duglin commented Jan 3, 2017

caseydavenport commented Jan 9, 2017

tomdee commented Jan 13, 2017

tomdee commented Jan 13, 2017

tomdee 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

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

caseydavenport commented Jan 13, 2017 • edited Loading

duglin commented Jan 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseydavenport commented Jan 25, 2017

duglin commented Jan 25, 2017

caseydavenport commented Jan 25, 2017

duglin commented Jan 25, 2017

ozdanborne commented Jan 30, 2017

tomdee commented Jan 30, 2017

gunjan5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjan5 Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjan5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjan5 commented Feb 7, 2017

duglin commented Nov 28, 2016 •

edited

Loading

caseydavenport commented Jan 13, 2017 •

edited

Loading

gunjan5 Feb 1, 2017 •

edited

Loading