Add per-node config infrastructure and per-node BGP peering #464

robbrockbank · 2017-06-28T02:16:39Z

Should be ready for initial review.

This PR adds generic per-node configuration (so it could if necessary be used for per-node Felix config and per-node BGP config). Longer term the per-node stuff should disappear as we take a more selector based approach to configuration - but having the generic code should make it easier if we need to add the additional per-node config as an interim measure.

This also adds per-node BGP Peering and includes e2e tests that test against etcdv2 and kdd.

Note that due to the node being an oft-updated resource, it is necessary to retry the update if there is a conflict. I added a generic retryWrapper to handle this. Any resource where we are tacking Calico data onto another Kubernetes resource could benefit from being wrapped.

caseydavenport

Okey doke, I've done a first pass and have some questions :)

caseydavenport · 2017-06-29T20:11:56Z

lib/backend/k8s/resources/customnoderesource.go

+	// Get the names and the latest Node settings associated with the Key.
+	_, resName, node, err := c.getNamesAndNodeFromKey(kvp.Key)
+	if err != nil {
+		logContext.WithError(err).Info("Error getting current settings when creating resource.")


I think this should be an Error level log, right?

Plus "getting current settings" is a bit odd - isn't this more "Error looking up resource" or something like that?

caseydavenport · 2017-06-29T20:16:10Z

lib/backend/k8s/resources/customnoderesource.go

+	})
+	logContext.Debug("Update per-Node resource")
+
+	// Get the names and the latest Node settings associated with the Key.


This code (down until line 122) seems common with above - might it make sense to pull out into a helper?

caseydavenport · 2017-06-29T20:27:06Z

lib/backend/k8s/resources/customnoderesource.go

+		logContext.WithError(err).Error("Error extracting annotation when updating resource")
+		return nil, err
+	}
+	if len(kvps) != 1 {


Is it possible we'll ever get say, len 2 back? I think probably not..

Nope, and it'd be an error if we did.

caseydavenport · 2017-06-29T20:32:41Z

lib/backend/k8s/resources/customnoderesource.go

+	for _, node := range nodes {
+		nodeKVPs, err := c.extractFromAnnotations(&node, resName)
+		if err != nil {
+			logContext.WithField("NodeName", node.GetName()).WithError(err).Warning("Error listing resources for Node")


Hm, I wonder if we should fail the list request if this happens.

I think the contract should probably be that either it succeeds (and all the desired nodes are listed) or it fails and you get an error. WDYT?

I think we should always return what we can (otherwise stuffing up a single entry in a Node annotation could potentially take down all resources and render them useless.

My take would be, for all operations:

If the node name is specified and the node annotation cannot be parsed at all due to corrupted data: return an error

If the resource name is explicitly specified, and that specific entry within the annotation is corrupted: return an error

If the resource name is not specified (which will only be the case for List) and some of the entries are corrupt (but not all of them) then return what entries within the annotation that we can. This would minimize the impact of someone having attempted to hand edit (badly) an annotation.

These are all unexpected error cases - but I think it's nice to be able to handle as gracefully as we can these cases.

caseydavenport · 2017-06-29T20:35:03Z

lib/backend/k8s/resources/customnoderesource.go

+
+// getNamesAndNodeFromKey extracts the Node name and the Resource name from the Key
+// and gets the current Node resource config from the Kubernetes API.
+// Returns: the Node name, the Resource name, The Node resource.


Why do we need to return the node Name and the Node itself?

The name is accessible from the returned Node.

caseydavenport · 2017-06-29T20:37:15Z

lib/backend/k8s/resources/customnoderesource.go

+	// Get the current node settings.
+	node, err := c.clientSet.Nodes().Get(nodeName, metav1.GetOptions{})
+	if err != nil {
+		logContext.WithError(err).Info("Error getting Node configuration")


Should probably be an error log that says "Error getting Node from Kubernetes API" and include the node name.

caseydavenport · 2017-06-29T20:42:22Z

lib/backend/k8s/resources/customnoderesource.go

+		logContext.Debug("Restrict results to only include requested resource name")
+		val, ok := raw[reqName]
+		raw = make(map[string]string, 0)
+		if ok {


So, if reqName != "" and !ok, shouldn't that be an error?

No, this is used for Listing too.

Ah, ok. I see now.

caseydavenport · 2017-06-29T20:47:31Z

lib/backend/k8s/resources/retrywrapper.go

+	var kvp *model.KVPair
+	var err error
+	for i := 0; i < maxActionRetries; i++ {
+		if kvp, err = r.client.Update(object); err == nil {


So, how does this work?

If we get get a CAS error here, we'll need to re-read the object before writing, right?

Yup this wrapper is wrapped around the outside of the BGPPeer Client. So if that client filters up a "please retry this" error, this wrapper will re-invoke the Update on the BGPPeer Client (which will perform the Get/Modify/Update).

caseydavenport · 2017-06-29T20:52:29Z

lib/client/bgppeer_e2e_test.go

@@ -186,109 +189,3 @@ var _ = testutils.E2eDatastoreDescribe("BGPPeer tests", testutils.DatastoreEtcdV
 		})
 	})
 })
-
-// Perform CRUD operations on Global BGP Peer Resources
-var _ = testutils.E2eDatastoreDescribe("Global BGPPeer tests", testutils.DatastoreAll, func(config api.CalicoAPIConfig) {


Where did all of these guys go?

The guys ^^ should be sufficient. Previously I couldn't flag the guys ^^ to be tested against KDD because of the lack of Per-Node BGP support. Now that I can, I can get rid of these guys down below which were basically a copy of the guys above (but without any Per-node BGP Peers).

robbrockbank · 2017-06-30T18:28:35Z

@caseydavenport : Feedback applied (plus a tad more).

I tweaked the interface for the custom-node-resource helper ... so that it the responsibility of the derived resource to know how to marshal and unmarshal between the annotations and the resource.

Main reason for doing this is that we may (and I think probably do) need to support per-node felix and bgp config. I suspect in this case we might just want annotation entries such as:

felix.projectcalico.org/<config>=<value>

and

bgp.projectcalico.org/<config>=<value>

rather than having a single entry that stores a dict of all the config values.

Hmmm, I wonder if we should do that for the peers now that i think about it:

bgppeer.projectcalico.org/peer1=<marshalled value1>
bgppeer.projectcalico.org/peer2=<marshalled value2>
bgppeer.projectcalico.org/peer3=<marshalled value3>

rather than

projectcalico.org/bgppeers=<marshalled dictionary of peers>

caseydavenport

Ok, I think this is the last round from me. A bunch of nitty comments.

caseydavenport · 2017-07-05T22:47:55Z

lib/backend/k8s/k8s_fv_test.go

@@ -742,6 +742,163 @@ var _ = Describe("Test Syncer API for Kubernetes backend", func() {
 		})
 	})

+	It("should handle a CRUD of Node BGP Peer", func() {
+		var kvp1, kvp1_2, kvp2, kvp2_2 *model.KVPair


ooo, underscores in variable names.

Haven't seen that in a while :)

okay okay - i've changed to 1a,1b and 2a,2b.

caseydavenport · 2017-07-05T22:51:24Z

lib/backend/k8s/k8s_fv_test.go

+			Expect(kvps[0].Key).To(Equal(kvp1_2.Key))
+			Expect(kvps[0].Value).To(Equal(kvp1_2.Value))
+			Expect(kvps[1].Key).To(Equal(kvp2_2.Key))
+			Expect(kvps[01].Value).To(Equal(kvp2_2.Value))


caseydavenport · 2017-07-05T22:52:21Z

lib/backend/k8s/k8s_fv_test.go

+			Expect(kvps[0].Value).To(Equal(kvp1_2.Value))
+		})
+
+		By("Deleting an existing Node BGP Peer", func() {


Maybe one more which is "Deleting a non-existent Node BGP Peer"?

caseydavenport · 2017-07-05T22:53:52Z

lib/backend/k8s/resources/customresource.go

-	// Update the revision information from the response.
-	kvp.Revision = resOut.GetObjectMeta().GetResourceVersion()
-	return kvp, nil
+	// Return the Key and Value with updated Revision information.


Is this just a preference or does it do something meaningful?

Well we don't really need it although it makes the testing easier. Reasoning is, if you call

kvp_out, err := client.Create(kvp_in)

You wouldn't expect kvp_in to change, and therefore you might wish to use it again. But as it stands we'll update kvp_in and just return it as kvp_out.

Anyhow, I've removed as it wasn't needed for this PR.

caseydavenport · 2017-07-05T23:16:06Z

lib/backend/k8s/resources/nodebgppeer_test.go

+		key, err := converter.NodeAndNameToKey("nodeA", "1-2-3-4")
+		Expect(err).To(BeNil())
+		Expect(key).To(Equal(model.NodeBGPPeerKey{
+			Nodename: "nodeS",


huh, shouldn't this be nodeA? Why are the tests passing?

Well that's just embarrassing - no test suite defined, so not actually running any of these tests. Will fix (and fix up broken tests).

caseydavenport · 2017-07-05T23:29:56Z

lib/backend/k8s/resources/customnoderesource.go

+	})
+	logContext.Debug("Update per-Node resource")
+
+	// Get the resource name and the latest Node settings associated with the Key.


What are "Node settings"? Does this mean "Node resource"?

caseydavenport · 2017-07-05T23:30:23Z

lib/backend/k8s/resources/customnoderesource.go

+	}
+	ak := c.nameToAnnotationKey(resName)
+
+	// There should be no existing entry for a Create.


stale comment

caseydavenport · 2017-07-05T23:34:48Z

lib/backend/k8s/resources/customnoderesource.go

+		nodeList, err := c.ClientSet.Nodes().List(metav1.ListOptions{})
+		if err != nil {
+			logContext.WithError(err).Info("Failed to list resources: unable to list Nodes")
+			err = K8sErrorToCalico(err, nodeName)


Could we combine this line into the return below?

caseydavenport · 2017-07-05T23:36:41Z

lib/backend/k8s/resources/customnoderesource.go

+}
+
+// getNameAndNodeFromKey extracts the resource name from the Key
+// and gets the current Node resource config from the Kubernetes API.


"Node resource config" I think is just a complicated way of saying "Node" :)

caseydavenport · 2017-07-05T23:37:02Z

lib/backend/k8s/resources/customnoderesource.go

+
+// getNameAndNodeFromKey extracts the resource name from the Key
+// and gets the current Node resource config from the Kubernetes API.
+// Returns: the Resource name, The Node resource.


weird capitalization.

robbrockbank · 2017-07-06T04:07:18Z

@caseydavenport hopefully last iteration

caseydavenport · 2017-07-06T17:50:43Z

@robbrockbank feel free to merge after squashing 🤞

addig L7LogsURLCharLimit config setting

robbrockbank force-pushed the per-node-config branch 5 times, most recently from 154c3db to d45d94b Compare June 28, 2017 07:09

robbrockbank changed the title ~~[WIP] Add per-node config~~ Add per-node config infrastructure and per-node BGP peering Jun 29, 2017

robbrockbank requested a review from caseydavenport June 29, 2017 16:04

robbrockbank assigned caseydavenport Jun 29, 2017

caseydavenport requested changes Jun 29, 2017

View reviewed changes

This was referenced Jul 3, 2017

Add per-node and global BGP config to KDD #468

Merged

Expose per-node listing as external interfaces #469

Merged

robbrockbank force-pushed the per-node-config branch from 4e7c6fb to bbbeb8a Compare July 5, 2017 19:20

caseydavenport requested changes Jul 5, 2017

View reviewed changes

caseydavenport approved these changes Jul 6, 2017

View reviewed changes

Add per-node config infrastructure and add per-node BGP peers

29df6fd

robbrockbank force-pushed the per-node-config branch from e9afc76 to 29df6fd Compare July 6, 2017 17:52

robbrockbank merged commit 2990e44 into projectcalico:master Jul 6, 2017

robbrockbank deleted the per-node-config branch July 6, 2017 17:59

song-jiang pushed a commit to song-jiang/libcalico-go that referenced this pull request Jul 19, 2021

Merge pull request projectcalico#464 from penkeysuresh/l7-char-limit

27c8530

addig L7LogsURLCharLimit config setting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per-node config infrastructure and per-node BGP peering #464

Add per-node config infrastructure and per-node BGP peering #464

robbrockbank commented Jun 28, 2017 •

edited

Loading

caseydavenport left a comment

caseydavenport Jun 29, 2017

caseydavenport Jun 29, 2017

caseydavenport Jun 29, 2017

robbrockbank Jun 29, 2017

caseydavenport Jun 29, 2017

robbrockbank Jun 30, 2017

robbrockbank Jun 30, 2017

caseydavenport Jun 29, 2017

caseydavenport Jun 29, 2017

caseydavenport Jun 29, 2017

robbrockbank Jun 29, 2017

caseydavenport Jun 30, 2017

caseydavenport Jun 29, 2017

robbrockbank Jun 29, 2017

caseydavenport Jun 29, 2017

robbrockbank Jun 29, 2017

robbrockbank commented Jun 30, 2017 •

edited

Loading

caseydavenport left a comment

caseydavenport Jul 5, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 5, 2017

caseydavenport Jul 5, 2017

caseydavenport Jul 5, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 5, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 5, 2017

caseydavenport Jul 5, 2017

caseydavenport Jul 5, 2017

caseydavenport Jul 5, 2017

caseydavenport Jul 5, 2017

robbrockbank commented Jul 6, 2017

caseydavenport commented Jul 6, 2017

Add per-node config infrastructure and per-node BGP peering #464

Add per-node config infrastructure and per-node BGP peering #464

Conversation

robbrockbank commented Jun 28, 2017 • edited Loading

caseydavenport 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

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

robbrockbank commented Jun 30, 2017 • edited Loading

caseydavenport 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

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

robbrockbank commented Jul 6, 2017

caseydavenport commented Jul 6, 2017

robbrockbank commented Jun 28, 2017 •

edited

Loading

robbrockbank commented Jun 30, 2017 •

edited

Loading