Skip to content
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.

Sign up for GitHub

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

replication: add new Validated condition #664

Merged
merged 1 commit into from
Sep 12, 2024
Merged

replication: add new Validated condition #664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/replication.storage/v1alpha1/volumereplication_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const (
ConditionCompleted = "Completed"
ConditionDegraded = "Degraded"
ConditionResyncing = "Resyncing"
ConditionValidated = "Validated"
)

// These are valid conditions.
Expand Down Expand Up @@ -60,6 +61,10 @@ const (
FailedToResync = "FailedToResync"
// NotResyncing condition represents the volume is not resyncing.
NotResyncing = "NotResyncing"
// PrerequisiteMet condition represents that the prerequisite is met.
PrerequisiteMet = "PrerequisiteMet"
// PrerequisiteNotMet condition represents that the prerequisite is not met.
PrerequisiteNotMet = "PrerequisiteNotMet"
)

// ReplicationState represents the replication operations to be performed on the volume.
Expand Down
34 changes: 34 additions & 0 deletions internal/controller/replication.storage/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,40 @@ func setFailedPromotionCondition(conditions *[]metav1.Condition, observedGenerat
ObservedGeneration: observedGeneration,
Status: metav1.ConditionFalse,
})
setStatusCondition(conditions, &metav1.Condition{
Madhu-1 marked this conversation as resolved.
Show resolved Hide resolved
Type: v1alpha1.ConditionValidated,
Reason: v1alpha1.PrerequisiteMet,
ObservedGeneration: observedGeneration,
Status: metav1.ConditionTrue,
})
}

// sets conditions when volume promotion was failed due to failed validation.
func setFailedValidationCondition(conditions *[]metav1.Condition, observedGeneration int64) {
setStatusCondition(conditions, &metav1.Condition{
Type: v1alpha1.ConditionCompleted,
Reason: v1alpha1.FailedToPromote,
ObservedGeneration: observedGeneration,
Status: metav1.ConditionFalse,
})
setStatusCondition(conditions, &metav1.Condition{
Type: v1alpha1.ConditionDegraded,
Reason: v1alpha1.Error,
ObservedGeneration: observedGeneration,
Status: metav1.ConditionTrue,
})
setStatusCondition(conditions, &metav1.Condition{
Type: v1alpha1.ConditionResyncing,
Reason: v1alpha1.NotResyncing,
ObservedGeneration: observedGeneration,
Status: metav1.ConditionFalse,
})
setStatusCondition(conditions, &metav1.Condition{
Type: v1alpha1.ConditionValidated,
Reason: v1alpha1.PrerequisiteNotMet,
ObservedGeneration: observedGeneration,
Status: metav1.ConditionFalse,
})
}

// sets conditions when volume is demoted and ready to use (resync completed).
Expand Down
16 changes: 11 additions & 5 deletions internal/controller/replication.storage/volumereplication_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const (
var (
volumePromotionKnownErrors = []codes.Code{codes.FailedPrecondition}
disableReplicationKnownErrors = []codes.Code{codes.NotFound}
enableReplicationKnownErrors = []codes.Code{codes.FailedPrecondition}
getReplicationInfoKnownErrors = []codes.Code{codes.NotFound}
)

Expand Down Expand Up @@ -322,7 +323,6 @@ func (r *VolumeReplicationReconciler) Reconcile(ctx context.Context, req ctrl.Re
// enable replication only if its not primary
if err = r.enableReplication(vr); err != nil {
logger.Error(err, "failed to enable replication")
setFailureCondition(instance)
msg := replication.GetMessageFromError(err)
uErr := r.updateReplicationStatus(instance, logger, getCurrentReplicationState(instance), msg)
if uErr != nil {
Expand Down Expand Up @@ -741,13 +741,19 @@ func (r *VolumeReplicationReconciler) enableReplication(vr *volumeReplicationIns

resp := volumeReplication.Enable()

if resp.Error != nil {
vr.logger.Error(resp.Error, "failed to enable volume replication")
if resp.Error == nil {
return nil
}

return resp.Error
vr.logger.Error(resp.Error, "failed to enable volume replication")

if resp.HasKnownGRPCError(enableReplicationKnownErrors) {
setFailedValidationCondition(&vr.instance.Status.Conditions, vr.instance.Generation)
} else {
setFailedPromotionCondition(&vr.instance.Status.Conditions, vr.instance.Generation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please use setFailureCondition(instance) as it internally takes care of setting the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/csi-addons/kubernetes-csi-addons/blob/main/internal/controller/replication.storage/status.go

I would like to use similar format of using different functions currently present in status.go than forcing another function (for just one case) just to handle if else decision which then would again call the functions present in status.go .

https://github.com/csi-addons/kubernetes-csi-addons/blob/main/internal/controller/replication.storage/volumereplication_controller.go#L805

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have a generic helper function to set the conditions i don't like to see others calling it even though we have a helper function to do it. and for this again it's a bug we should not be setting setFailedPromotionCondition condition if the enable replication fails, the condition should reflect what went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have a generic helper function to set the conditions i don't like to see others calling it even though we have a helper function to do it. and for this again it's a bug we should not be setting setFailedPromotionCondition condition if the enable replication fails, the condition should reflect what went wrong.

Enabling replication is part of promotion. I don't see a issue with that part.

Why do you want to force a helper function that is used at a lot of other places to fit into a new scenario ?
If we do that, we'll need more arguments and the function will not be so generic anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even thought its part of the condition we should reflect on what exactly went wrong #666 is the tracker for it.

Why do you want to force a helper function that is used at a lot of other places to fit into a new scenario ?
If we do that, we'll need more arguments and the function will not be so generic anymore.

i don't see any new change in the behaviour to call setFailureCondition instead of setFailedPromotionCondition

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't get why you want to call that function when you already know which function it will in turn call.

There are a lot of places already calling functions from status.go directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to remove all that one and remove unwanted functions, we should be doing call A in one function to set status and call B in one function to set status, we need have only one way to set the conditions

}

return nil
return resp.Error
}

// getVolumeReplicationInfo gets volume replication info.
Expand Down
Loading