feat: drift detection + takeover (full draft) #1001
Conversation
…ift-detection-takeover
…ift-detection-takeover
|
|
||
| if !utils.IsDriftedResourcePlacementEqual(oldBinding.Status.DriftedPlacements, newBinding.Status.DriftedPlacements) { | ||
| klog.V(2).InfoS("The binding drifted placement has changed, need to update the corresponding CRP", "oldBinding", klog.KObj(oldBinding), "newBinding", klog.KObj(newBinding)) | ||
| return true | ||
| } | ||
|
|
||
| if !utils.IsDiffedResourcePlacementEqual(oldBinding.Status.DiffedPlacements, newBinding.Status.DiffedPlacements) { | ||
| klog.V(2).InfoS("The binding diffed placement has changed, need to update the corresponding CRP", "oldBinding", klog.KObj(oldBinding), "newBinding", klog.KObj(newBinding)) | ||
| return true | ||
| } |
There was a problem hiding this comment.
is there any e2e cover this case?
| status.DriftedPlacements = binding.Status.DriftedPlacements | ||
| } | ||
| } | ||
| // Drifts are reported regardless of the status of the Applied condition. |
There was a problem hiding this comment.
is there any e2e cover this case?
| func (r *Reconciler) processApplyStrategyUpdates( | ||
| ctx context.Context, | ||
| crp *fleetv1beta1.ClusterResourcePlacement, | ||
| allBindings []*fleetv1beta1.ClusterResourceBinding, | ||
| ) error { |
There was a problem hiding this comment.
nit: the parameters are mostly on one line the rest of the file
| // Update the CRP with a new apply strategy. | ||
| rolloutCRP.Spec.Strategy.ApplyStrategy = &fleetv1beta1.ApplyStrategy{ | ||
| ComparisonOption: fleetv1beta1.ComparisonOptionTypeFullComparison, | ||
| WhenToApply: fleetv1beta1.WhenToApplyTypeIfNotDrifted, | ||
| WhenToTakeOver: fleetv1beta1.WhenToTakeOverTypeIfNoDiff, | ||
| Type: fleetv1beta1.ApplyStrategyTypeServerSideApply, | ||
| ServerSideApplyConfig: &fleetv1beta1.ServerSideApplyConfig{ | ||
| ForceConflicts: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
how about add a case the CRP changed but no applyStrategy related change?
| 5, | ||
| 4, |
There was a problem hiding this comment.
where do those magic number come from?
| ignoreFieldTypeMetaInNamespace = cmpopts.IgnoreFields(corev1.Namespace{}, "TypeMeta") | ||
| ) | ||
|
|
||
| func TestMain(m *testing.M) { |
There was a problem hiding this comment.
where is this function called?
|
|
||
| fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" | ||
| ) | ||
|
|
There was a problem hiding this comment.
how many old test cases do we have to make sure whatever we have is backward compatible?
| appliedResourceMeta := []fleetv1beta1.AppliedResourceMeta{ | ||
| { | ||
| WorkResourceIdentifier: fleetv1beta1.WorkResourceIdentifier{ | ||
| Version: "v1", | ||
| Kind: "Namespace", | ||
| Resource: "namespaces", | ||
| Name: nsName, | ||
| }, | ||
| UID: regularNS.UID, | ||
| }, |
There was a problem hiding this comment.
why is that NS is applied when the option is "reportDiffOnly"?
| { | ||
| Type: fleetv1beta1.WorkConditionTypeApplied, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(ManifestProcessingApplyResultTypeNoDiffFound), | ||
| ObservedGeneration: 0, | ||
| }, | ||
| { | ||
| Type: fleetv1beta1.WorkConditionTypeAvailable, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(ManifestProcessingAvailabilityResultTypeAvailable), | ||
| ObservedGeneration: 0, |
There was a problem hiding this comment.
why is the namespace applied/available?
| Message: ManifestProcessingApplyResultTypeAppliedWithFailedDriftDetectionDescription, | ||
| ObservedGeneration: inMemberClusterObjGeneration, | ||
| } | ||
| case appliedResTyp == ManifestProcessingApplyResultTypeNoDiffFound: |
There was a problem hiding this comment.
this does not seem to be right when the apply Type is ApplyStrategyTypeReportDiff. No diff does not mean applied == true
|
Closing this PR as the main part has been checked in. |
2 participants
Description of your changes
Full draft for new feature.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer