syncer writes neg desc on initialization #1187

swetharepakula · 2020-07-18T01:25:40Z

updates status on NEG CRs when neg crd is enabled
errors during sync and initializations are exposed in NEG CR

k8s-ci-robot · 2020-07-18T01:25:48Z

Hi @swetharepakula. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rramkumar1 · 2020-07-19T21:38:25Z

/ok-to-test

pkg/neg/manager.go

pkg/neg/syncers/transaction.go

pkg/neg/types/interfaces.go

pkg/neg/syncers/utils.go

pkg/neg/syncers/transaction.go

pkg/apis/svcneg/v1beta1/types.go

pkg/neg/syncers/transaction.go

freehan · 2020-07-24T18:01:20Z

pkg/neg/syncers/transaction.go

+	negCR.Status.Conditions = ensureCondition(negCR.Status.Conditions, negv1beta1.Synced, types.NegSyncSuccessful, types.NegSyncFailed, ts, syncErr)
+	negCR.Status.LastSyncTime = ts
+
+	_, err = s.svcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(s.Namespace).Update(context.Background(), negCR, metav1.UpdateOptions{})


I think it might make sense to use PATCH instead of Update to update the condition. This will save one GET API call. Double check if you can make it work. If not, we can sync up.

This is an example https://github.com/kubernetes/ingress-gce/blob/master/pkg/neg/readiness/reflector.go#L268

freehan · 2020-07-28T17:27:14Z

pkg/neg/syncers/transaction.go

+
+// patchNegStatus patches the specified NegCR status with the provided new status
+func patchNegStatus(svcNegClient svcnegclient.Interface, oldStatus, newStatus negv1beta1.ServiceNetworkEndpointGroupStatus, namespace, negName string) (*negv1beta1.ServiceNetworkEndpointGroup, error) {
+	patchBytes, err := utils.StrategicMergePatchBytes(negv1beta1.ServiceNetworkEndpointGroup{Status: oldStatus}, negv1beta1.ServiceNetworkEndpointGroup{Status: newStatus}, negv1beta1.ServiceNetworkEndpointGroup{})


I am a bit confused. You are calculating StrategicMergePatch but the patch type is MergePatchType.

Does it work?

I did take a look at the MergePatchType. I tested both out and both merge patches worked with the MergePatchType in this situation. I was avoiding the MergePatch just to reduce the amount of code introduced in this PR. But I can switch to using a MergePatch

pkg/neg/syncers/transaction.go

freehan · 2020-07-28T17:40:20Z

pkg/neg/syncers/transaction.go

+		return nil, fmt.Errorf("failed to prepare patch bytes: %s", err)
+	}
+
+	return svcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(namespace).Patch(context.Background(), negName, types.MergePatchType, patchBytes, metav1.PatchOptions{})


I may have mislead you in the previous comment.

Some background: https://erosb.github.io/post/json-patch-vs-merge-patch/
Strategic Merge patch is different than these two.

I have some code example for calculating json patch

import ( "encoding/json" "fmt" "github.com/mattbaird/jsonpatch" ) // CreatePatch creates a json patch to convert the old object to the new object. func CreatePatch(old []byte, newObj interface{}) ([]byte, error) { newJson, err := json.Marshal(newObj) if err != nil { return nil, fmt.Errorf("failed to marshal object %v into json: %v", newObj, err) } patch, err := jsonpatch.CreatePatch(old, newJson) if err != nil { return nil, fmt.Errorf("failed to creat patch: %v", err) } return json.Marshal(patch) }

I have not found handy example for json merge patch. If you find it you might use it as well. No preference for me.

There is a parallel function for calculating the json merge patch in apimachinery (https://github.com/kubernetes/apimachinery/blob/master/pkg/util/jsonmergepatch/patch.go#L31). It will be similar to how the StrategicMergePatch is calculated. I can use that and avoid bringing in another dependency.

pkg/neg/syncers/transaction.go

freehan · 2020-07-28T17:54:05Z

pkg/neg/syncers/transaction.go

+		negCR.Status.Conditions = ensureCondition(negCR.Status.Conditions, negv1beta1.Initialized, negtypes.NegInitializationSuccessful, negtypes.NegInitializationFailed, ts, syncErr)
+	}
+
+	negCR.Status.Conditions = ensureCondition(negCR.Status.Conditions, negv1beta1.Synced, negtypes.NegSyncSuccessful, negtypes.NegSyncFailed, ts, syncErr)


recommend modify ensureCondition to directly ensure the conditions on the object directly.

for example,
ensureCondition(svcNeg *negv1beta1.ServiceNetworkEndpointGroup, expectedCondition condition)
ensureCondition(svcNeg *negv1beta1.ServiceNetworkEndpointGroup, type, reason, error, message, timestamp)

freehan · 2020-07-28T17:55:04Z

pkg/neg/syncers/transaction.go

+
+// ensureNegRefs verfies that the negRefs provided are not empty. If empty, queries the cloud for the neg and returns a
+// list of NegObjectReferences to be set in the Neg CR status
+func ensureNegRefs(cloud negtypes.NetworkEndpointGroupCloud, zones []string, negRefs []negv1beta1.NegObjectReference, negName string, version meta.Version) []negv1beta1.NegObjectReference {


seems a bit redundant since the ensureNetworkEndpointGroups has done this.

If needInit is false, the NegRefs never gets updated if something goes wrong the first time in updating the cr or if a neg cr is deleted and recreated before the syncer and neg is not GCed yet. But with the suggestion above in regards to populating the initialization condition, that would cause this function to become unnecessary so I will remove this.

pkg/neg/syncers/utils.go

freehan

LGTM overall

freehan · 2020-07-28T23:29:12Z

pkg/neg/syncers/transaction.go

+
+	condition.LastTransitionTime = ts
+	if err != nil {
+		condition.Status = core.ConditionUnknown


ConditionFalse

- update initialized status on NEG CRs when neg is initialized - update synced condition after sync completes - expose errors during sync and initializations in NEG CR - set needInit to false if no errors during neg initialization

freehan

/lgtm
/approve

k8s-ci-robot · 2020-07-29T17:53:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, swetharepakula

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [freehan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 18, 2020

k8s-ci-robot requested review from freehan and rramkumar1 July 18, 2020 01:25

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 19, 2020

freehan reviewed Jul 22, 2020

View reviewed changes

freehan self-assigned this Jul 22, 2020

swetharepakula force-pushed the neg-crd-syncer branch 3 times, most recently from 0c2bd3a to 2685750 Compare July 24, 2020 01:42

Change LastSyncTime json tag to lastSyncTime

5d0f02f

swetharepakula force-pushed the neg-crd-syncer branch from 2685750 to 008e5d8 Compare July 24, 2020 02:07

freehan reviewed Jul 24, 2020

View reviewed changes

swetharepakula force-pushed the neg-crd-syncer branch 3 times, most recently from 419fc39 to d2aaa3d Compare July 25, 2020 06:05

freehan reviewed Jul 28, 2020

View reviewed changes

Create JSON Merge Patch Type patches

7f1bf5b

swetharepakula force-pushed the neg-crd-syncer branch from d2aaa3d to ff8c894 Compare July 28, 2020 21:39

freehan reviewed Jul 28, 2020

View reviewed changes

swetharepakula force-pushed the neg-crd-syncer branch 2 times, most recently from cc0cb66 to 8e6709b Compare July 29, 2020 01:29

syncer writes neg desc on initialization

130802f

- update initialized status on NEG CRs when neg is initialized - update synced condition after sync completes - expose errors during sync and initializations in NEG CR - set needInit to false if no errors during neg initialization

swetharepakula force-pushed the neg-crd-syncer branch from 8e6709b to 130802f Compare July 29, 2020 01:30

freehan approved these changes Jul 29, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2020

k8s-ci-robot merged commit d14b9c9 into kubernetes:master Jul 29, 2020

swetharepakula deleted the neg-crd-syncer branch July 29, 2020 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syncer writes neg desc on initialization #1187

syncer writes neg desc on initialization #1187

swetharepakula commented Jul 18, 2020

k8s-ci-robot commented Jul 18, 2020

rramkumar1 commented Jul 19, 2020

freehan Jul 24, 2020

freehan Jul 28, 2020

swetharepakula Jul 28, 2020

freehan Jul 28, 2020

freehan Jul 28, 2020

swetharepakula Jul 28, 2020

freehan Jul 28, 2020

freehan Jul 28, 2020

swetharepakula Jul 28, 2020

freehan left a comment

freehan Jul 28, 2020

freehan left a comment

k8s-ci-robot commented Jul 29, 2020

syncer writes neg desc on initialization #1187

syncer writes neg desc on initialization #1187

Conversation

swetharepakula commented Jul 18, 2020

k8s-ci-robot commented Jul 18, 2020

rramkumar1 commented Jul 19, 2020

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

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 29, 2020