-
Notifications
You must be signed in to change notification settings - Fork 23
task(GcpVpcPeering): watching operations #1559
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,6 +148,13 @@ type VpcPeeringStatus struct { | |
| // +listType=map | ||
| // +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 | ||
| //One example is the quota exceeded error that can happen on either side if one of the vpcs is close to the quota limit | ||
| // +optional | ||
| Operation string `json:"operation,omitempty"` | ||
| // +optional | ||
| RemoteOperation string `json:"remoteOperation,omitempty"` | ||
|
Comment on lines
+155
to
+157
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is kind of vague naming. In my opinion, it is unclear what is Operation, and what is RemoteOperation (arent both operations remote?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a stable term
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Peering requires two API calls. First initalize peering connection, second accepts it. I assume that these are two different operations in GCP.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I prefer very descriptive naming |
||
| } | ||
|
|
||
| //+kubebuilder:object:root=true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ type GcpClients struct { | |
|
|
||
| // The VpcPeeringClients uses a different service account than the other clients and has different permissions as well. | ||
| type VpcPeeringClients struct { | ||
| ComputeGlobalOperations *compute.GlobalOperationsClient | ||
| ComputeNetworks *compute.NetworksClient | ||
| ResourceManagerTagBindings *resourcemanager.TagBindingsClient | ||
| } | ||
|
|
@@ -126,8 +127,11 @@ func NewGcpClients(ctx context.Context, saJsonKeyPath string, vpcPeeringSaJsonKe | |
| if err != nil { | ||
| return nil, fmt.Errorf("error creating vpc peering compute networks client: %w", err) | ||
| } | ||
| vpcPeeringComputeGlobalOperations, err := compute.NewGlobalOperationsRESTClient(ctx, option.WithTokenSource(vpcPeeringComputeNetworksTokenSource)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating vpc peering compute operations client: %w", err) | ||
| } | ||
|
Comment on lines
+130
to
+133
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if I followed you but as Milos said, VPC Peering uses a different principal to work.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand |
||
| // resource manager client for VPC peering, uses a different service account---------------- | ||
|
|
||
| vpcPeeringResourceManagerTokenProvider, err := vpcPeeringClientBuilder.WithScopes(resourcemanager.DefaultAuthScopes()).BuildTokenProvider() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build vpc peering resource manager token provider: %w", err) | ||
|
|
@@ -148,6 +152,7 @@ func NewGcpClients(ctx context.Context, saJsonKeyPath string, vpcPeeringSaJsonKe | |
| RedisCluster: redisCluster, | ||
| RedisInstance: redisInstance, | ||
| VpcPeeringClients: &VpcPeeringClients{ | ||
| ComputeGlobalOperations: vpcPeeringComputeGlobalOperations, | ||
| ComputeNetworks: vpcPeeringComputeNetworks, | ||
| ResourceManagerTagBindings: vpcPeeringresourceManagerTagBindings, | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package meta | |
|
|
||
| import ( | ||
| "errors" | ||
| "strings" | ||
|
|
||
| "github.com/googleapis/gax-go/v2/apierror" | ||
| "google.golang.org/api/googleapi" | ||
|
|
@@ -74,3 +75,21 @@ func IsTooManyRequests(err error) bool { | |
|
|
||
| return false | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
Comment on lines
+79
to
+95
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this func is in a shared metadata file,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree and will make the change, unfortunately the GCP API returns a 400 and the subcode is well badRequest.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Function Naming: The function name Please provide feedback on the review comment by checking the appropriate box:
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,10 +13,11 @@ type vpcPeeringEntry struct { | |
| peering *pb.NetworkPeering | ||
| } | ||
| type vpcPeeringStore struct { | ||
| m sync.Mutex | ||
| items map[string]*vpcPeeringEntry | ||
| errorMap map[string]error | ||
| tags map[string][]string | ||
| m sync.Mutex | ||
| items map[string]*vpcPeeringEntry | ||
| operations map[string]*pb.Operation | ||
| errorMap map[string]error | ||
| tags map[string][]string | ||
| } | ||
|
|
||
| type VpcPeeringMockClientUtils interface { | ||
|
|
@@ -30,7 +31,7 @@ func getFullNetworkUrl(project, vpc string) string { | |
| return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/global/networks/%s", project, vpc) | ||
| } | ||
|
|
||
| func (s *vpcPeeringStore) CreateRemoteVpcPeering(ctx context.Context, remotePeeringName string, remoteVpc string, remoteProject string, customRoutes bool, kymaProject string, kymaVpc string) error { | ||
| func (s *vpcPeeringStore) CreateRemoteVpcPeering(ctx context.Context, remotePeeringName string, remoteVpc string, remoteProject string, customRoutes bool, kymaProject string, kymaVpc string) (*pb.Operation, error) { | ||
| s.m.Lock() | ||
| defer s.m.Unlock() | ||
| remoteNetwork := getFullNetworkUrl(remoteProject, remoteVpc) | ||
|
|
@@ -42,9 +43,21 @@ func (s *vpcPeeringStore) CreateRemoteVpcPeering(ctx context.Context, remotePeer | |
| exportCustomRoutes = true | ||
| } | ||
|
|
||
| if s.operations == nil { | ||
| s.operations = make(map[string]*pb.Operation) | ||
| } | ||
|
|
||
| if s.operations[remoteVpc] == nil { | ||
| operationpb := &pb.Operation{ | ||
| Status: ptr.To(pb.Operation_DONE), | ||
| Name: ptr.To(remoteVpc), | ||
| } | ||
| s.operations[remoteVpc] = operationpb | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it otherwise return operation in progress error? |
||
|
|
||
| _, peeringExists := s.items[remoteNetwork] | ||
| if peeringExists { | ||
| return nil | ||
| return s.operations[remoteVpc], nil | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
| } | ||
|
|
||
| item := &vpcPeeringEntry{ | ||
|
|
@@ -59,10 +72,10 @@ func (s *vpcPeeringStore) CreateRemoteVpcPeering(ctx context.Context, remotePeer | |
| } | ||
| s.items[remoteNetwork] = item | ||
|
|
||
| return nil | ||
| return s.operations[remoteVpc], nil | ||
| } | ||
|
|
||
| func (s *vpcPeeringStore) CreateKymaVpcPeering(ctx context.Context, remotePeeringName string, remoteVpc string, remoteProject string, customRoutes bool, kymaProject string, kymaVpc string) error { | ||
| func (s *vpcPeeringStore) CreateLocalVpcPeering(ctx context.Context, remotePeeringName string, remoteVpc string, remoteProject string, customRoutes bool, kymaProject string, kymaVpc string) (*pb.Operation, error) { | ||
| s.m.Lock() | ||
| defer s.m.Unlock() | ||
|
|
||
|
|
@@ -72,9 +85,21 @@ func (s *vpcPeeringStore) CreateKymaVpcPeering(ctx context.Context, remotePeerin | |
| exportCustomRoutes := false | ||
| importCustomRoutes := customRoutes | ||
|
|
||
| if s.operations == nil { | ||
| s.operations = make(map[string]*pb.Operation) | ||
| } | ||
|
|
||
| if s.operations[kymaVpc] == nil { | ||
| operationpb := &pb.Operation{ | ||
| Status: ptr.To(pb.Operation_DONE), | ||
| Name: ptr.To(kymaVpc), | ||
| } | ||
| s.operations[kymaVpc] = operationpb | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it otherwise return operation in progress error? |
||
|
|
||
| _, peeringExists := s.items[kymaNetwork] | ||
| if peeringExists { | ||
| return nil | ||
| return s.operations[kymaVpc], nil | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
| } | ||
|
|
||
| item := &vpcPeeringEntry{ | ||
|
|
@@ -90,7 +115,7 @@ func (s *vpcPeeringStore) CreateKymaVpcPeering(ctx context.Context, remotePeerin | |
|
|
||
| s.items[kymaNetwork] = item | ||
|
|
||
| return nil | ||
| return s.operations[kymaVpc], nil | ||
| } | ||
|
|
||
| func (s *vpcPeeringStore) GetRemoteNetworkTags(_ context.Context, vpc string, project string) ([]string, error) { | ||
|
|
@@ -122,6 +147,10 @@ func (s *vpcPeeringStore) GetVpcPeering(ctx context.Context, remotePeeringName s | |
| s.tags = make(map[string][]string) | ||
| } | ||
|
|
||
| if s.operations == nil { | ||
| s.operations = make(map[string]*pb.Operation) | ||
| } | ||
|
|
||
| _, peeringExists := s.items[network] | ||
| if !peeringExists { | ||
| return nil, nil | ||
|
|
@@ -143,6 +172,17 @@ func (s *vpcPeeringStore) DeleteVpcPeering(ctx context.Context, remotePeeringNam | |
| return nil | ||
| } | ||
|
|
||
| func (s *vpcPeeringStore) GetOperation(ctx context.Context, project string, operationId string) (*pb.Operation, error) { | ||
| s.m.Lock() | ||
| defer s.m.Unlock() | ||
|
|
||
| _, operationExists := s.operations[operationId] | ||
|
Comment on lines
+175
to
+179
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
| if !operationExists { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
| return nil, fmt.Errorf("operation %s not found", operationId) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's try to stub the error GCP API would return |
||
| } | ||
| return s.operations[operationId], nil | ||
| } | ||
|
|
||
| // Fake client implementations to mimic google API calls | ||
| func (s *vpcPeeringStore) SetMockVpcPeeringLifeCycleState(project string, vpc string, state pb.NetworkPeering_State) { | ||
| stateString := state.String() | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,83 @@ | ||||||||||||||||
| package vpcpeering | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "context" | ||||||||||||||||
| "strings" | ||||||||||||||||
|
|
||||||||||||||||
| pb "cloud.google.com/go/compute/apiv1/computepb" | ||||||||||||||||
| cloudcontrolv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-control/v1beta1" | ||||||||||||||||
| "github.com/kyma-project/cloud-manager/api/cloud-resources/v1beta1" | ||||||||||||||||
| "github.com/kyma-project/cloud-manager/pkg/util" | ||||||||||||||||
| "k8s.io/apimachinery/pkg/api/meta" | ||||||||||||||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||||||
| "k8s.io/utils/ptr" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/kyma-project/cloud-manager/pkg/composed" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| func checkLocalOperation(ctx context.Context, st composed.State) (error, context.Context) { | ||||||||||||||||
| state := st.(*State) | ||||||||||||||||
| logger := composed.LoggerFromCtx(ctx) | ||||||||||||||||
|
|
||||||||||||||||
| if composed.MarkedForDeletionPredicate(ctx, state) || (state.localOperation != nil && state.localOperation.GetStatus() != pb.Operation_PENDING) { | ||||||||||||||||
| return nil, ctx | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if state.ObjAsVpcPeering().Status.Operation != "" { | ||||||||||||||||
| op, err := state.client.GetOperation(ctx, state.LocalNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.Operation) | ||||||||||||||||
|
Comment on lines
+26
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code Structure: Consider using early returns to reduce nesting. Invert the condition and return early to improve readability.
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||||||||||||
| if err != nil { | ||||||||||||||||
| logger.Error(err, "[KCP GCP VpcPeering checkLocalOperation] Error getting local operation") | ||||||||||||||||
|
Comment on lines
+26
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some if's can be inverted here to reduce nesting by using early exit. |
||||||||||||||||
| meta.SetStatusCondition(state.ObjAsVpcPeering().Conditions(), metav1.Condition{ | ||||||||||||||||
| Type: v1beta1.ConditionTypeError, | ||||||||||||||||
| Status: "True", | ||||||||||||||||
| Reason: v1beta1.ConditionReasonError, | ||||||||||||||||
| Message: "Error loading Local Vpc Peering Operation: " + state.ObjAsVpcPeering().Status.Operation, | ||||||||||||||||
| }) | ||||||||||||||||
| err = state.PatchObjStatus(ctx) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return composed.LogErrorAndReturn(err, | ||||||||||||||||
| "Error updating status since it was not possible to load the local Vpc Peering operation.", | ||||||||||||||||
| composed.StopWithRequeueDelay(util.Timing.T10000ms()), | ||||||||||||||||
| ctx, | ||||||||||||||||
| ) | ||||||||||||||||
| } | ||||||||||||||||
| return composed.StopWithRequeueDelay(util.Timing.T60000ms()), nil | ||||||||||||||||
| } | ||||||||||||||||
| state.localOperation = op | ||||||||||||||||
| if op != nil { | ||||||||||||||||
| if op.GetStatus() != pb.Operation_DONE { | ||||||||||||||||
| logger.Info("[KCP GCP VpcPeering checkLocalOperation] Local operation still in progress", "localOperation", ptr.Deref(op.Name, "OperationUnknown")) | ||||||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log Message Inconsistency: Line 53 says "checkRemoteOperation" but this is in the checkLocalOperation function. Should be "checkLocalOperation".
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||||||||||||
| state.ObjAsVpcPeering().Status.State = cloudcontrolv1beta1.VirtualNetworkPeeringStateDisconnected | ||||||||||||||||
| if strings.Contains(op.GetError().String(), "QUOTA_EXCEEDED") { | ||||||||||||||||
| return composed.UpdateStatus(state.ObjAsVpcPeering()).SetExclusiveConditions(metav1.Condition{ | ||||||||||||||||
| Type: v1beta1.ConditionTypeQuotaExceeded, | ||||||||||||||||
| Status: "True", | ||||||||||||||||
| Reason: v1beta1.ConditionTypeQuotaExceeded, | ||||||||||||||||
| Message: "Error creating Local Vpc Peering due to quota limits, please check if your vpc quota limits are not exceeded.", | ||||||||||||||||
| }). | ||||||||||||||||
| ErrorLogMessage("Error creating Local VpcPeering due to quota exceeded"). | ||||||||||||||||
| FailedError(composed.StopWithRequeue). | ||||||||||||||||
| SuccessError(composed.StopWithRequeueDelay(util.Timing.T300000ms())). | ||||||||||||||||
| Run(ctx, state) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return composed.UpdateStatus(state.ObjAsVpcPeering()).SetExclusiveConditions(metav1.Condition{ | ||||||||||||||||
| Type: v1beta1.ConditionTypeError, | ||||||||||||||||
| Status: "True", | ||||||||||||||||
| Reason: v1beta1.ConditionTypeError, | ||||||||||||||||
| Message: "The cloud provider had an error while creating Local Vpc Peering", | ||||||||||||||||
| }). | ||||||||||||||||
| ErrorLogMessage("The cloud provider had an error while creating Local Vpc Peering"). | ||||||||||||||||
| FailedError(composed.StopWithRequeue). | ||||||||||||||||
| SuccessError(composed.StopWithRequeueDelay(util.Timing.T300000ms())). | ||||||||||||||||
| Run(ctx, state) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return nil, nil | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||||||||||||
| package vpcpeering | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "context" | ||||||||||||||||
|
|
||||||||||||||||
| pb "cloud.google.com/go/compute/apiv1/computepb" | ||||||||||||||||
| "github.com/kyma-project/cloud-manager/api/cloud-resources/v1beta1" | ||||||||||||||||
| "github.com/kyma-project/cloud-manager/pkg/util" | ||||||||||||||||
| "k8s.io/apimachinery/pkg/api/meta" | ||||||||||||||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||||||
| "k8s.io/utils/ptr" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/kyma-project/cloud-manager/pkg/composed" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| func checkRemoteOperation(ctx context.Context, st composed.State) (error, context.Context) { | ||||||||||||||||
| state := st.(*State) | ||||||||||||||||
| logger := composed.LoggerFromCtx(ctx) | ||||||||||||||||
|
|
||||||||||||||||
| if composed.MarkedForDeletionPredicate(ctx, state) || (state.remoteOperation != nil && state.remoteOperation.GetStatus() != pb.Operation_PENDING) { | ||||||||||||||||
| return nil, ctx | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if state.ObjAsVpcPeering().Status.RemoteOperation != "" { | ||||||||||||||||
| op, err := state.client.GetOperation(ctx, state.RemoteNetwork().Spec.Network.Reference.Gcp.GcpProject, state.ObjAsVpcPeering().Status.RemoteOperation) | ||||||||||||||||
|
Comment on lines
+24
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code Structure: Use early return to reduce nesting and improve readability.
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||||||||||||
| if err != nil { | ||||||||||||||||
|
Comment on lines
+24
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer early exit to reduce nesting |
||||||||||||||||
| logger.Error(err, "[KCP GCP VpcPeering checkRemoteOperation] Error getting remote operation") | ||||||||||||||||
| meta.SetStatusCondition(state.ObjAsVpcPeering().Conditions(), metav1.Condition{ | ||||||||||||||||
| Type: v1beta1.ConditionTypeError, | ||||||||||||||||
| Status: "True", | ||||||||||||||||
| Reason: v1beta1.ConditionReasonError, | ||||||||||||||||
| Message: "Error loading Remote Vpc Peering Operation: " + state.ObjAsVpcPeering().Status.RemoteOperation, | ||||||||||||||||
| }) | ||||||||||||||||
| err = state.PatchObjStatus(ctx) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return composed.LogErrorAndReturn(err, | ||||||||||||||||
| "Error updating status since it was not possible to load the remote Vpc Peering operation.", | ||||||||||||||||
| composed.StopWithRequeueDelay(util.Timing.T10000ms()), | ||||||||||||||||
| ctx, | ||||||||||||||||
| ) | ||||||||||||||||
| } | ||||||||||||||||
| return composed.StopWithRequeueDelay(util.Timing.T60000ms()), nil | ||||||||||||||||
| } | ||||||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Please provide feedback on the review comment by checking the appropriate box:
|
||||||||||||||||
| return composed.StopWithRequeueDelay(util.Timing.T60000ms()), nil | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||
| } | ||||||||||||||||
| return nil, ctx | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return nil, nil | ||||||||||||||||
| } | ||||||||||||||||
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.
Comment Formatting: The comment should start with a space after
//according to Go conventions.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box: