-
Notifications
You must be signed in to change notification settings - Fork 299
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.
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
syncer writes neg desc on initialization #1187
syncer writes neg desc on initialization #1187
Conversation
swetharepakula
commented
Jul 18, 2020
- updates status on NEG CRs when neg crd is enabled
- errors during sync and initializations are exposed in NEG CR
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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
0c2bd3a
to
2685750
Compare
2685750
to
008e5d8
Compare
pkg/neg/syncers/transaction.go
Outdated
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{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
419fc39
to
d2aaa3d
Compare
pkg/neg/syncers/transaction.go
Outdated
|
||
// 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{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused. You are calculating StrategicMergePatch but the patch type is MergePatchType.
Does it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
pkg/neg/syncers/transaction.go
Outdated
|
||
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems a bit redundant since the ensureNetworkEndpointGroups
has done this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d2aaa3d
to
ff8c894
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
pkg/neg/syncers/transaction.go
Outdated
|
||
condition.LastTransitionTime = ts | ||
if err != nil { | ||
condition.Status = core.ConditionUnknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConditionFalse
cc0cb66
to
8e6709b
Compare
- 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
8e6709b
to
130802f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[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:
Approvers can indicate their approval by writing |