task(GcpVpcPeering): watching operations #1559
Conversation
vandjelk
changed the title
| Operation string `json:"operation,omitempty"` | ||
| // +optional | ||
| RemoteOperation string `json:"remoteOperation,omitempty"` |
There was a problem hiding this comment.
This is kind of vague naming. In my opinion, it is unclear what is Operation, and what is RemoteOperation (arent both operations remote?)
There was a problem hiding this comment.
There's a stable term remote in the peering relating to the subscriptions not it our control - the other side we're peering with. It's a string distinguishment since all ops on the remote side require other credentials and "remote" client . I guess this is same as Operation field, but for that remote side. I would rather see if that can fit into the existing Operation field, but if not, guess the field and the term itself is fine.
There was a problem hiding this comment.
Actually aren't all peering ops all done with the single peering principal, both on our and remote side? Then why is a new field required? Why not use the old Operation field?
There was a problem hiding this comment.
Peering requires two API calls. First initalize peering connection, second accepts it. I assume that these are two different operations in GCP.
There was a problem hiding this comment.
Exactly as Vladimir said, Operation is the local peering operation and RemoteOperation is the remote peering operation, they are different from each other. What I can do is renaming Operation to LocalOperation ? What are your thoughts ?
There was a problem hiding this comment.
Personally, I prefer very descriptive naming LocalPeeringOperation RemotePeeringOperation
| func IsOperationInProgressError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
|
|
||
| var googleapierror *googleapi.Error | ||
| if ok := errors.As(err, &googleapierror); ok { | ||
| if googleapierror.Code == 400 { | ||
| for _, e := range googleapierror.Errors { | ||
| if strings.Contains(e.Message, "peering operation in progress") { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Since this func is in a shared metadata file, IsOperationInProgressError name is misleading.
It is specifically tied to peering operation in progress.
There was a problem hiding this comment.
I agree and would put an effort to recognize if it's any operation in progress not just the peering one. Ideally it should not be done with string recognition but more strictly trough some code/subcode, but if GCP API doesn't provide that, at least search for operation in progress - aka remove the peering part
There was a problem hiding this comment.
I agree and will make the change, unfortunately the GCP API returns a 400 and the subcode is well badRequest....
| if state.ObjAsVpcPeering().Status.Operation != "" { | ||
| op, err := state.client.GetOperation(ctx, state.LocalNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.Operation) | ||
| if err != nil { | ||
| logger.Error(err, "[KCP GCP VpcPeering checkLocalOperation] Error getting local operation") |
There was a problem hiding this comment.
Some if's can be inverted here to reduce nesting by using early exit.
| if state.ObjAsVpcPeering().Status.RemoteOperation != "" { | ||
| op, err := state.client.GetOperation(ctx, state.RemoteNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.RemoteOperation) | ||
| if err != nil { |
There was a problem hiding this comment.
Prefer early exit to reduce nesting
| vpcPeeringComputeGlobalOperations, err := compute.NewGlobalOperationsRESTClient(ctx, option.WithTokenSource(vpcPeeringComputeNetworksTokenSource)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating vpc peering compute operations client: %w", err) | ||
| } |
There was a problem hiding this comment.
In my opinion, it would be better if we didnt couple ComputeGlobalOperations client with VpcPeering.
I recommend instantiating it as part of compute, and give it compute token source.
There was a problem hiding this comment.
That client is used to check the create/delete operation status that was started with the peering principal, so it also must be used peering principal. The peering here refers to the peering principal not to the peering operation per se. I agree, the terminology is a bit conflicting with that remote from before, but it definitely can not be switched to use"compute token source"
There was a problem hiding this comment.
I am not sure if I followed you but as Milos said, VPC Peering uses a different principal to work.
| if state.kymaVpcPeering != nil { | ||
| if composed.MarkedForDeletionPredicate(ctx, state) || (state.localVpcPeering != nil || (state.localOperation != nil)) { |
There was a problem hiding this comment.
Why are you checking MarkedForDelete?
The flow is already directed in the reconciler (new.go).
|
|
||
| if state.remoteVpcPeering != nil { | ||
| if composed.MarkedForDeletionPredicate(ctx, state) || state.remoteVpcPeering != nil || (state.remoteOperation != nil) { |
There was a problem hiding this comment.
MarkedForDelete is already handled in new.go
| //kcpCondErr := meta.FindStatusCondition(state.KcpVpcPeering.Status.Conditions, cloudcontrolv1beta1.ConditionTypeError) | ||
| //kcpCondReady := meta.FindStatusCondition(state.KcpVpcPeering.Status.Conditions, cloudcontrolv1beta1.ConditionTypeReady) | ||
| // | ||
| //skrCondErr := meta.FindStatusCondition(state.ObjAsGcpVpcPeering().Status.Conditions, cloudresourcesv1beta1.ConditionTypeError) | ||
| //skrCondReady := meta.FindStatusCondition(state.ObjAsGcpVpcPeering().Status.Conditions, cloudresourcesv1beta1.ConditionTypeReady) | ||
| // | ||
| //state.ObjAsGcpVpcPeering().Status.State = state.KcpVpcPeering.Status.State | ||
| // | ||
| //if kcpCondErr != nil && skrCondErr == nil { | ||
| // return composed.UpdateStatus(state.ObjAsGcpVpcPeering()). | ||
| // SetExclusiveConditions(metav1.Condition{ | ||
| // Type: cloudresourcesv1beta1.ConditionTypeError, |
| Operation string `json:"operation,omitempty"` | ||
| // +optional | ||
| RemoteOperation string `json:"remoteOperation,omitempty"` |
There was a problem hiding this comment.
There's a stable term remote in the peering relating to the subscriptions not it our control - the other side we're peering with. It's a string distinguishment since all ops on the remote side require other credentials and "remote" client . I guess this is same as Operation field, but for that remote side. I would rather see if that can fit into the existing Operation field, but if not, guess the field and the term itself is fine.
| Operation string `json:"operation,omitempty"` | ||
| // +optional | ||
| RemoteOperation string `json:"remoteOperation,omitempty"` |
There was a problem hiding this comment.
Actually aren't all peering ops all done with the single peering principal, both on our and remote side? Then why is a new field required? Why not use the old Operation field?
| vpcPeeringComputeGlobalOperations, err := compute.NewGlobalOperationsRESTClient(ctx, option.WithTokenSource(vpcPeeringComputeNetworksTokenSource)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating vpc peering compute operations client: %w", err) | ||
| } |
There was a problem hiding this comment.
That client is used to check the create/delete operation status that was started with the peering principal, so it also must be used peering principal. The peering here refers to the peering principal not to the peering operation per se. I agree, the terminology is a bit conflicting with that remote from before, but it definitely can not be switched to use"compute token source"
| } | ||
| if gcpmeta.IsOperationInProgressError(err) { | ||
| logger.Info("There is a peering operation in progress on the local or peer network.") | ||
| if state.ObjAsVpcPeering().Spec.Details.DeleteRemotePeering { |
There was a problem hiding this comment.
There's no purpose to check DeleteRemotePeering arg here when it's checked at the top of the file. If operation is in progress it alway must requeue with enough time to let the old op finish before it tries again.
| func IsOperationInProgressError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
|
|
||
| var googleapierror *googleapi.Error | ||
| if ok := errors.As(err, &googleapierror); ok { | ||
| if googleapierror.Code == 400 { | ||
| for _, e := range googleapierror.Errors { | ||
| if strings.Contains(e.Message, "peering operation in progress") { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
I agree and would put an effort to recognize if it's any operation in progress not just the peering one. Ideally it should not be done with string recognition but more strictly trough some code/subcode, but if GCP API doesn't provide that, at least search for operation in progress - aka remove the peering part
| Name: ptr.To(remoteVpc), | ||
| } | ||
| s.operations[remoteVpc] = operationpb | ||
| } |
There was a problem hiding this comment.
Should it otherwise return operation in progress error?
| Name: ptr.To(kymaVpc), | ||
| } | ||
| s.operations[kymaVpc] = operationpb | ||
| } |
There was a problem hiding this comment.
Should it otherwise return operation in progress error?
|
|
||
| _, operationExists := s.operations[operationId] | ||
| if !operationExists { | ||
| return nil, fmt.Errorf("operation %s not found", operationId) |
There was a problem hiding this comment.
Let's try to stub the error GCP API would return
| if op != nil { | ||
| if op.GetStatus() != pb.Operation_DONE { | ||
| logger.Info("[KCP GCP VpcPeering checkRemoteOperation] Remote operation still in progress", "remoteOperation", ptr.Deref(op.Name, "OperationUnknown")) | ||
| return composed.StopWithRequeueDelay(util.Timing.T60000ms()), nil |
There was a problem hiding this comment.
I suggest using requeue delay near the avg peering op duration. How must time it takes for a peering operation to complete? On Azure it in average couple of seconds. Do we really need 1min polling wait time here? And if made shorter, let's not log every time. There's util.ExpiringSwitch() that you can use to make logging less frequent.
There was a problem hiding this comment.
From my experience it varies, sometimes it takes seconds, sometimes it takes a few seconds sometimes it can take up to 2 minutes. For quota exceeded errors I have seen it doing 3 cycles until its cleared.
Thanks for the ExpiringSwitch I will have a look!
| localVpcPeering *pb.NetworkPeering | ||
|
|
||
| //Operations | ||
| remoteOperation *pb.Operation |
There was a problem hiding this comment.
What's the different between remoteOperation and localOperation? What's the general logic of the reconciler? I would assume if there's an op id in the status what until it's completed, otherwise just continue and eventually stat the op, save it to the status and requeue. If so, don't see a reason to have them distinguished to remote and local.
There was a problem hiding this comment.
logic is:
First we create the remote peering which returns the remote operation.
Then we wait for the remote operation to be done and then we create the local peering which returns the local peering operation.
I think it is better to keep both operation for diagnosing issues, but it is possible to use only one operation saving first the remote and then overriding with the local, is that your suggestion ?
|
|
||
| if err != nil { | ||
| return composed.LogErrorAndReturn(err, "[SKR GCP VPCPeering deleteKcpVpcPeering] Error deleting KCP VpcPeering", composed.StopWithRequeue, ctx) | ||
| return composed.LogErrorAndReturn(err, "[SKR GCP VPCPeering deleteKcpVpcPeering] Error deleting KCP VpcPeering "+state.KcpVpcPeering.Name, composed.StopWithRequeue, ctx) |
There was a problem hiding this comment.
We should do logging uniformly.
- we normally don't use square brackets in the log message.
SKRis already present in the log attributeplaneset by usVpcPeeringis already present in the log attributekindset by controller-runtimestate.KcpVpcPeering.Nameor any other value should be injected into the logger withlogger.WithValues()and not contacted to the message- the name of the reconciled object is already present in the log attribute
nameand set by controller-runtime, don't see a need to repeat it
|
|
||
| if kcpCondErr != nil && skrCondErr == nil { | ||
| // Initial status update when SKR status conditions are empty | ||
| if len(state.ObjAsGcpVpcPeering().Status.Conditions) == 0 { |
There was a problem hiding this comment.
GcpVpcPeering should just copy status from VpcPeering perhaps something like this
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This pull request adds operation tracking for GCP VPC Peering connections to better handle asynchronous operations. While the overall approach is sound, there are several issues that need to be addressed:
Critical Issues:
- Potential nil pointer dereferences in
updateStatus.gowhen accessing operation errors - Potential panic in
waitSkrStatusReady.goandupdateStatus.gowhen accessing conditions arrays without proper bounds checking - Logic inconsistency in checking error types (using
state.remoteOperationinstead ofopvariable)
Code Quality Issues:
- Large blocks of commented-out code should be removed
- Redundant
MarkedForDeletionchecks in create functions that are already handled by the flow control - Missing spaces in concatenated error messages
- Inconsistent logging patterns (should use structured logging with
WithValues()) - Function naming in shared metadata file is too specific to peering operations
Performance Considerations:
- The 60-second requeue delay for operation polling seems excessive and could be reduced to 10-15 seconds for better responsiveness
Please address these issues, particularly the potential runtime panics from nil pointer dereferences and array bounds violations, before merging.
PR Bot Information
Version: 1.15.8
- Event Trigger:
issue_comment.created - Correlation ID:
262db5c0-cbbe-11f0-904f-5cc52db59f97
| // +listMapKey=type | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
|
|
||
| //GCP creates operations when creating peerings, since we are creating two peerings (one in each direction), we store the operation names here to track them if needed |
There was a problem hiding this comment.
Comment Formatting: The comment should start with a space after // according to Go conventions.
| //GCP creates operations when creating peerings, since we are creating two peerings (one in each direction), we store the operation names here to track them if needed | |
| // GCP creates operations when creating peerings, since we are creating two peerings (one in each direction), we store the operation names here to track them if needed | |
| // One example is the quota exceeded error that can happen on either side if one of the vpcs is close to the quota limit |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| if state.ObjAsVpcPeering().Status.Operation != "" { | ||
| op, err := state.client.GetOperation(ctx, state.LocalNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.Operation) |
There was a problem hiding this comment.
Code Structure: Consider using early returns to reduce nesting. Invert the condition and return early to improve readability.
| if state.ObjAsVpcPeering().Status.Operation != "" { | |
| op, err := state.client.GetOperation(ctx, state.LocalNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.Operation) | |
| if state.ObjAsVpcPeering().Status.Operation == "" { | |
| return nil, ctx | |
| } | |
| op, err := state.client.GetOperation(ctx, state.LocalNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.Operation) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| return composed.StopWithRequeueDelay(util.Timing.T60000ms()), nil | ||
| } | ||
| if op.GetError() != nil { | ||
| logger.Error(err, "[KCP GCP VpcPeering checkRemoteOperation] Local operation error "+op.GetName()) |
There was a problem hiding this comment.
Log Message Inconsistency: Line 53 says "checkRemoteOperation" but this is in the checkLocalOperation function. Should be "checkLocalOperation".
| logger.Error(err, "[KCP GCP VpcPeering checkRemoteOperation] Local operation error "+op.GetName()) | |
| logger.Error(err, "[KCP GCP VpcPeering checkLocalOperation] Local operation error "+op.GetName()) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| if state.ObjAsVpcPeering().Status.RemoteOperation != "" { | ||
| op, err := state.client.GetOperation(ctx, state.RemoteNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.RemoteOperation) |
There was a problem hiding this comment.
Code Structure: Use early return to reduce nesting and improve readability.
| if state.ObjAsVpcPeering().Status.RemoteOperation != "" { | |
| op, err := state.client.GetOperation(ctx, state.RemoteNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.RemoteOperation) | |
| if state.ObjAsVpcPeering().Status.RemoteOperation == "" { | |
| return nil, ctx | |
| } | |
| op, err := state.client.GetOperation(ctx, state.RemoteNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.RemoteOperation) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| state.remoteOperation = op | ||
| if op != nil { | ||
| if op.GetStatus() != pb.Operation_DONE { | ||
| logger.Info("[KCP GCP VpcPeering checkRemoteOperation] Remote operation still in progress", "remoteOperation", ptr.Deref(op.Name, "OperationUnknown")) |
There was a problem hiding this comment.
Performance: The requeue delay of 60 seconds seems excessive. GCP peering operations typically complete within seconds. Consider reducing this to a shorter interval (e.g., 10-15 seconds) to improve responsiveness. Also, consider using util.ExpiringSwitch() to reduce log frequency during the polling period.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| _, peeringExists := s.items[remoteNetwork] | ||
| if peeringExists { | ||
| return nil | ||
| return s.operations[remoteVpc], nil |
There was a problem hiding this comment.
Mock Logic: When a peering already exists, should this return an "operation in progress" error instead of just returning the operation? This would better simulate real GCP behavior.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| _, peeringExists := s.items[kymaNetwork] | ||
| if peeringExists { | ||
| return nil | ||
| return s.operations[kymaVpc], nil |
There was a problem hiding this comment.
Mock Logic: Similar to CreateRemoteVpcPeering, when a peering already exists, should this return an "operation in progress" error to better simulate real GCP behavior?
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| defer s.m.Unlock() | ||
|
|
||
| _, operationExists := s.operations[operationId] | ||
| if !operationExists { |
There was a problem hiding this comment.
Mock Error Handling: Instead of returning a generic error message, consider stubbing the actual error that GCP API would return. This would make testing more realistic.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| // ErrorLogMessage("Error updating KCP GcpVpcPeering status with ready condition"). | ||
| // SuccessError(composed.StopWithRequeue). | ||
| // Run(ctx, state) | ||
| //} |
There was a problem hiding this comment.
Code Quality: There's a large block of commented-out code (lines 37-74). This should either be removed or if it's needed for reference, consider adding a clear comment explaining why it's being kept.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| func updateStatus(ctx context.Context, st composed.State) (error, context.Context) { | ||
| state := st.(*State) | ||
| logger := composed.LoggerFromCtx(ctx) | ||
| //logger := composed.LoggerFromCtx(ctx) |
There was a problem hiding this comment.
Commented Debug Code: The logger variable is commented out but not used. Either use it or remove the commented line.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
4 participants
Description
Changes proposed in this pull request:
Related issue(s)
Issues#1557