Introduce ExtendedObjectMetadata, holding Writes #87

MikeSpreitzer · 2020-02-08T05:14:53Z

Introduced an additional metadata section to Subnet and
NetworkEndpoint, holding a server-side timestamped record of the
latest write to each section of the object. The timestamps are full
precision, NOT truncated to a whole number of seconds.

Updated ipam and connection agent to use those write stamps instead of
the truncated creation timestamp in the relevant histograms. Also
introduced a histogram of latency from (a) setting of status by local
connection agent to (b) creation of remote ifc by remote connection
agent.

This may be viewed as a preview of the "managed fields" metadata that is coming along with server-side apply, to become pervasive in Kubernetes release 1.18. We are not ready to advance to release 1.18 yet; when we do, we may replace this with a use of the managed fields metadata.

matte21

I have to pause the review and I'm not done yet, but there's already some stuff.

matte21 · 2020-02-10T12:39:58Z

staging/kos/pkg/registry/network/subnet/strategy.go

+	newSubnet := obj.(*network.Subnet)
+	newSubnet.ExtendedObjectMeta = oldSubnet.ExtendedObjectMeta
+	now := network.Now()
+	if newSubnet.Spec != oldSubnet.Spec {


This check is unneeded.
Currently spec is immutable for subnets (see *subnetStragey::ValidateUpdate), and *subnetStragey::PrepareForUpdate executes only for real updates, not for deletes followed by re-creates.

matte21 · 2020-02-10T12:52:48Z

staging/kos/pkg/registry/network/networkattachment/strategy.go

+	newNA := obj.(*network.NetworkAttachment)
+	newNA.ExtendedObjectMeta = oldNA.ExtendedObjectMeta
+	now := network.Now()
+	if oldNA.Spec.Node != newNA.Spec.Node || oldNA.Spec.Subnet != newNA.Spec.Subnet || !SliceOfStringEqual(oldNA.Spec.PostCreateExec, newNA.Spec.PostCreateExec) || !SliceOfStringEqual(oldNA.Spec.PostDeleteExec, newNA.Spec.PostDeleteExec) {


All the checks on the spec fields but the one on postDeleteExec are unneeded.
Currently all spec fields but postDeleteExec are immutable for NetworkAttachments (see networkAttachmentStrategy::ValidateUpdate), and networkAttachmentStrategy::PrepareForUpdate executes only for real updates, not for deletes followed by re-creates.

Actually, the CA "samples" the postDeleteExec of an attachment when it creates a network interface for the attachment, and always ignores postDeleteExec thereafter. We could therefore drop the check on NetworkAttachment's spec altogether.

Good, less to check is better.
I do not see why the cited behavior means the PostDelectExec field does not need to be examined for changes.

I do not see why the cited behavior means the PostDelectExec field does not need to be examined for changes.

You mean examined for changes in networkAttachmentStragey::PrepareForUpdate?
Actually, you're right.

matte21 · 2020-02-10T12:55:42Z

staging/kos/pkg/apis/network/v1alpha1/types.go

+
+// GetWrite returns the write to the given section, or zero if there
+// was none.
+func (writes WriteSet) GetWrite(section string) ObjectWrite {


It's a nit, but how about this function returns a got bool field as well, which is set to false when no write is found? The same change would also happen in the internal API version types file.

matte21 · 2020-02-10T13:12:31Z

staging/kos/pkg/apis/network/v1alpha1/types.go

+
+// UnionLeft produces the union of the receiver and the other writes
+// that do not overlap with the receiver
+func (writes WriteSet) UnionLeft(others WriteSet) WriteSet {


This method and the other Union methods are never used (same observation for internal API version types). Do you plan other commits where they are used?

Yes. I have some vague plans that use various of these.

matte21 · 2020-02-10T14:57:46Z

staging/kos/pkg/apis/network/v1alpha1/types.go

+func (writes WriteSet) UnionMin(others WriteSet) WriteSet {
+	ans := others.Diff(writes)
+	for _, wr := range writes {
+		owr := others.GetWrite(wr.Section)


If others does not contain a write for the given section, GetWrite returns an empty ObjectWrite, which will be added to ans with an empty Section field. How about adding owr.Section = wr.Section?
Same applies to WriteSet::UnionMax and the equivalent functions in the internal API types file.

Yes, this logic is wrong for Min. For Max, it is guaranteed that the other will not be taken if it is the zero value.

For Max, it is guaranteed that the other will not be taken if it is the zero value

Mmm... how is that guaranteed? AFAIU both UnionMin and UnionMax set ServerTime even if owr is the zero value, but Section could be left unset.

The expectation is that real server times are positive, so will win over zero in Max.
But I am rewriting both to use the extra bool.

The expectation is that real server times are positive, so will win over zero in Max.

But this affects in no way handling of Section.

matte21 · 2020-02-10T15:38:24Z

staging/kos/pkg/registry/network/networkattachment/strategy.go

+	}
+}
+
+func SliceOfStringEqual(x, y []string) bool {


This function is duplicated with the one in subnet strategy. Also, it is the negation of differ, one of the two could be therefore removed (I vote for differ). We should place this function in the util package (not necessarily now, even in the future).

Removed differ. We can think about the right place to put a utitlity like SliceOfStringEqual. In k/k, there is a lot of careful factoring of sources.

Right, let's do that on another occasion though. I would also expect k/k to already have a function like this in some util pkg.

matte21 · 2020-02-10T15:50:51Z

staging/kos/pkg/registry/network/networkattachment/strategy.go

+	newNA := obj.(*network.NetworkAttachment)
+	newNA.ExtendedObjectMeta = oldNA.ExtendedObjectMeta
+	now := network.Now()
+	if oldNA.Spec.Node != newNA.Spec.Node || oldNA.Spec.Subnet != newNA.Spec.Subnet || !SliceOfStringEqual(oldNA.Spec.PostCreateExec, newNA.Spec.PostCreateExec) || !SliceOfStringEqual(oldNA.Spec.PostDeleteExec, newNA.Spec.PostDeleteExec) {


Actually, the CA "samples" the postDeleteExec of an attachment when it creates a network interface for the attachment, and always ignores postDeleteExec thereafter. We could therefore drop the check on NetworkAttachment's spec altogether.

matte21 · 2020-02-10T16:00:11Z

staging/kos/pkg/controllers/connectionagent/connection_agent.go

@@ -1057,7 +1070,7 @@ func (ca *ConnectionAgent) updateLocalAttachmentStatus(att *netv1a1.NetworkAttac
 		updatedAtt.Status.PostCreateExecReport)

 	if att.Status.HostIP == "" {
-		ca.attachmentCreateToStatusHistogram.Observe(tAfterUpdate.Truncate(time.Second).Sub(att.CreationTimestamp.Time).Seconds())
+		ca.attachmentCreateToStatusHistogram.Observe(updatedAtt.Writes.GetServerWriteTime(netv1a1.NASectionAddr).Sub(att.Writes.GetServerWriteTime(netv1a1.NASectionSpec)).Seconds())


Shouldn't s/netv1a1.NASectionAddr/netv1a1.NASectionImpl/?

Introduced an additional metadata section to Subnet and NetworkEndpoint, holding a server-side timestamped record of the latest write to each section of the object. The timestamps are full precision, NOT truncated to a whole number of seconds. Updated ipam and connection agent to use those write stamps instead of the truncated creation timestamp in the relevant histograms. Also introduced a histogram of latency from (a) setting of status by local connection agent to (b) creation of remote ifc by remote connection agent.

MikeSpreitzer · 2020-02-10T16:38:04Z

The force-push to 3edb973 is a rebase onto add-kos.

matte21 · 2020-02-10T17:39:05Z

staging/kos/pkg/apis/network/types.go

@@ -83,7 +84,7 @@ func (writes WriteSet) SetWrite(section string, serverTime Timestamp) WriteSet {
 func (writes WriteSet) Diff(others WriteSet) WriteSet {
 	ans := make(WriteSet, 0, len(writes))
 	for _, wr := range writes {
-		owr := others.GetWrite(wr.Section)
+		owr, _ := others.GetWrite(wr.Section)


Other nit, but the following check could use the bool variable.

MikeSpreitzer · 2020-02-10T23:26:43Z

BTW, I am starting to get the idea that server-side apply is intended to have comprehensive coverage regardless of operation. That is being worked in master right now. I have not yet been able to get a successful test of that.

MikeSpreitzer · 2020-02-11T16:37:04Z

There is also this: kubernetes/community#4218 (comment)

Overlooked fixup

matte21 reviewed Feb 10, 2020

View reviewed changes

MikeSpreitzer force-pushed the numeric-timestamps branch from 3a6cb2a to 3edb973 Compare February 10, 2020 16:37

Fixes in rsponse to review

d5a572c

matte21 reviewed Feb 10, 2020

View reviewed changes

One more improvement from review

fb1d177

Overlooked fixup

MikeSpreitzer force-pushed the numeric-timestamps branch from df4b789 to fb1d177 Compare February 12, 2020 00:09

MikeSpreitzer merged commit d0b271b into add-kos Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ExtendedObjectMetadata, holding Writes #87

Introduce ExtendedObjectMetadata, holding Writes #87

MikeSpreitzer commented Feb 8, 2020 •

edited

Loading

matte21 left a comment

matte21 Feb 10, 2020

MikeSpreitzer Feb 10, 2020

matte21 Feb 10, 2020

matte21 Feb 10, 2020

MikeSpreitzer Feb 10, 2020 •

edited

Loading

matte21 Feb 10, 2020

matte21 Feb 10, 2020

MikeSpreitzer Feb 10, 2020

matte21 Feb 10, 2020

MikeSpreitzer Feb 10, 2020

matte21 Feb 10, 2020 •

edited

Loading

MikeSpreitzer Feb 10, 2020

matte21 Feb 10, 2020 •

edited

Loading

MikeSpreitzer Feb 10, 2020

matte21 Feb 10, 2020

matte21 Feb 10, 2020

MikeSpreitzer Feb 10, 2020

matte21 Feb 10, 2020 •

edited

Loading

matte21 Feb 10, 2020

matte21 Feb 10, 2020

MikeSpreitzer Feb 10, 2020

MikeSpreitzer commented Feb 10, 2020

matte21 Feb 10, 2020

MikeSpreitzer commented Feb 10, 2020 •

edited

Loading

MikeSpreitzer commented Feb 11, 2020

Introduce ExtendedObjectMetadata, holding Writes #87

Introduce ExtendedObjectMetadata, holding Writes #87

Conversation

MikeSpreitzer commented Feb 8, 2020 • edited Loading

matte21 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

MikeSpreitzer Feb 10, 2020 • edited Loading

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

matte21 Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matte21 Feb 10, 2020 • edited Loading

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

matte21 Feb 10, 2020 • edited Loading

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

MikeSpreitzer commented Feb 10, 2020

Choose a reason for hiding this comment

MikeSpreitzer commented Feb 10, 2020 • edited Loading

MikeSpreitzer commented Feb 11, 2020

MikeSpreitzer commented Feb 8, 2020 •

edited

Loading

MikeSpreitzer Feb 10, 2020 •

edited

Loading

matte21 Feb 10, 2020 •

edited

Loading

matte21 Feb 10, 2020 •

edited

Loading

matte21 Feb 10, 2020 •

edited

Loading

MikeSpreitzer commented Feb 10, 2020 •

edited

Loading