Skip to content

OCPBUGS-112721: Retry on conflict in syncMachineConfigNodes #6466

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

Closed
jira-solve-bot wants to merge 1 commit into from
+44 −2
Closed

OCPBUGS-112721: Retry on conflict in syncMachineConfigNodes #6466

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
38 changes: 38 additions & 0 deletions lib/resourceapply/machineconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ import (
"github.com/openshift/client-go/machineconfiguration/clientset/versioned/fake"
"github.com/openshift/machine-config-operator/test/helpers"
"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/diff"
clienttesting "k8s.io/client-go/testing"
)
Expand Down Expand Up @@ -296,3 +298,39 @@ func TestApplyMachineConfig(t *testing.T) {
})
}
}

func TestApplyMachineConfigNodeConflictReturnsRetriableError(t *testing.T) {

@pablintino pablintino Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are a few other reasons apart from Conflict that makes IsApplyErrorRetriable return true. Can we cover them? I'd prefer to see consistency when testing and cover as much as possible of the API being tested. A test with a few test cases suffice.

existing := &mcfgv1.MachineConfigNode{
ObjectMeta: metav1.ObjectMeta{Name: "node-1", ResourceVersion: "100"},
Spec: mcfgv1.MachineConfigNodeSpec{
Pool: mcfgv1.MCOObjectReference{Name: "worker"},
},
}
required := &mcfgv1.MachineConfigNode{
ObjectMeta: metav1.ObjectMeta{Name: "node-1"},
Spec: mcfgv1.MachineConfigNodeSpec{
Pool: mcfgv1.MCOObjectReference{Name: "master"},
},
}

client := fake.NewSimpleClientset(existing)
conflictErr := apierrors.NewConflict(
schema.GroupResource{Group: "machineconfiguration.openshift.io", Resource: "machineconfignodes"},
"node-1",
fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again"),
)
client.PrependReactor("update", "machineconfignodes", func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, nil, conflictErr
})

_, _, err := ApplyMachineConfigNode(client.MachineconfigurationV1(), required)
if err == nil {
t.Fatal("expected conflict error, got nil")
}
if !apierrors.IsConflict(err) {
t.Fatalf("expected conflict error, got: %v", err)
}
if !IsApplyErrorRetriable(err) {
t.Fatal("conflict error should be retriable via IsApplyErrorRetriable")
}
}
8 changes: 6 additions & 2 deletions pkg/operator/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,8 +894,12 @@ func (optr *Operator) syncMachineConfigNodes(_ *renderConfig, _ *configv1.Cluste
return err
}
p := mcoResourceRead.ReadMachineConfigNodeV1OrDie(mcsBytes)
mcn, _, err := mcoResourceApply.ApplyMachineConfigNode(optr.client.MachineconfigurationV1(), p)
if err != nil {
var mcn *mcfgv1.MachineConfigNode

@pablintino pablintino Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't a max retries make sense here? I know a few applies should do the job to make it apply succesfully in case of failure, but wouldn't it be more robuts to upper limit the retries?

if err := retry.OnError(retry.DefaultRetry, mcoResourceApply.IsApplyErrorRetriable, func() error {
var applyErr error
mcn, _, applyErr = mcoResourceApply.ApplyMachineConfigNode(optr.client.MachineconfigurationV1(), p)
return applyErr
}); err != nil {
return err
}
// if this is the first time we are applying the MCN and the node is ready, set the config version probably
Expand Down