Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ type InstallerController struct {

installerPodMutationFns []InstallerPodMutationFunc

// installerPrecondition, when set, is consulted immediately before an installer pod is created for a node.
// When it returns false the installer controller emits an InstallerPreconditionNotMet event and requeues
// instead of creating the installer pod (which would restart the operand static pod on that node).
installerPrecondition InstallerPreconditionFunc

startupMonitorEnabled func() (bool, error)

factory *factory.Factory
Expand Down Expand Up @@ -133,6 +138,24 @@ func (c *InstallerController) WithMinReadyDuration(minReadyDuration time.Duratio
return c
}

// installerPreconditionRequeueDuration is how long the installer controller waits before rechecking
// an unmet installer precondition.
const installerPreconditionRequeueDuration = 15 * time.Second

// InstallerPreconditionFunc returns true when it is safe to create an installer pod (which will
// restart the operand static pod) on the given node. When it returns false with a reason, the
// installer controller requeues and retries later. An error makes the sync fail.
type InstallerPreconditionFunc func(ctx context.Context, nodeName string) (safe bool, reason string, err error)

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.

I think that it would be more flexible to return the backoff duration as a return value, so we could turn safe into a time.Duration and wait when that duration is > 0. We can then remove installerPreconditionRequeueDuration, which is not configurable.


// WithInstallerPrecondition sets a precondition consulted immediately before creating an installer
// pod for a node. Operators can use it to delay operand restarts when doing so would be unsafe,
// for example when restarting an etcd member while another control plane node is being rebooted
// would lose quorum.
func (c *InstallerController) WithInstallerPrecondition(precondition InstallerPreconditionFunc) *InstallerController {
c.installerPrecondition = precondition
return c
}

func (c *InstallerController) WithCerts(certDir string, certConfigMaps, certSecrets []UnrevisionedResource) *InstallerController {
c.certDir = certDir
c.certConfigMaps = certConfigMaps
Expand Down Expand Up @@ -513,6 +536,18 @@ func (c *InstallerController) manageInstallationPods(ctx context.Context, operat
}
}

if c.installerPrecondition != nil {
safe, reason, err := c.installerPrecondition(ctx, currNodeState.NodeName)
if err != nil {
return true, 0, nil, nil, err
}
if !safe {
c.eventRecorder.Warningf("InstallerPreconditionNotMet", "Delaying installer pod for revision %d on node %q: %s",
currNodeState.TargetRevision, currNodeState.NodeName, reason)
return true, installerPreconditionRequeueDuration, nil, nil, nil
}
}

if err := c.ensureInstallerPod(ctx, operatorSpec, currNodeState); err != nil {
c.eventRecorder.Warningf("InstallerPodFailed", "Failed to create installer pod for revision %d count %d on node %q: %v",
currNodeState.TargetRevision, currNodeState.LastFailedCount, currNodeState.NodeName, err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2769,3 +2769,97 @@ func TestWaitToObserveWrites(t *testing.T) {
t.Fatalf("expected %d status apply, got %d", want, nApplies)
}
}

func TestCreateInstallerPodPrecondition(t *testing.T) {
newController := func(precondition InstallerPreconditionFunc) (*InstallerController, *events.Recorder, func() *corev1.Pod) {
kubeClient := fake.NewSimpleClientset(
&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "test-config"}},
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "test-secret"}},
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: fmt.Sprintf("%s-%d", "test-secret", 1)}},
&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: fmt.Sprintf("%s-%d", "test-config", 1)}},
)
var installerPod *corev1.Pod
kubeClient.PrependReactor("create", "pods", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
installerPod = action.(ktesting.CreateAction).GetObject().(*corev1.Pod)
return false, nil, nil
})
kubeInformers := informers.NewSharedInformerFactoryWithOptions(kubeClient, 1*time.Minute, informers.WithNamespace("test"))
fakeStaticPodOperatorClient := v1helpers.NewFakeStaticPodOperatorClient(
&operatorv1.StaticPodOperatorSpec{OperatorSpec: operatorv1.OperatorSpec{ManagementState: operatorv1.Managed}},
&operatorv1.StaticPodOperatorStatus{
OperatorStatus: operatorv1.OperatorStatus{LatestAvailableRevision: 1},
NodeStatuses: []operatorv1.NodeStatus{{NodeName: "test-node-1"}},
},
nil, nil,
)
eventRecorder := events.NewRecorder(kubeClient.CoreV1().Events("test"), "test-operator", &corev1.ObjectReference{}, clocktesting.NewFakePassiveClock(time.Now()))
c := NewInstallerController(
"unit-test", "test", "test-pod",
[]revision.RevisionResource{{Name: "test-config"}},
[]revision.RevisionResource{{Name: "test-secret"}},
[]string{"/bin/true"},
kubeInformers,
fakeStaticPodOperatorClient,
kubeClient.CoreV1(),
kubeClient.CoreV1(),
kubeClient.CoreV1(),
eventRecorder,
).WithInstallerPrecondition(precondition)
c.ownerRefsFn = func(ctx context.Context, revision int32) ([]metav1.OwnerReference, error) {
return []metav1.OwnerReference{}, nil
}
c.installerPodImageFn = func() string { return "docker.io/foo/bar" }
return c, &eventRecorder, func() *corev1.Pod { return installerPod }
}

t.Run("unmet precondition delays installer pod", func(t *testing.T) {
checkedNode := ""
c, eventRecorder, getPod := newController(func(ctx context.Context, nodeName string) (bool, string, error) {
checkedNode = nodeName
return false, "another master is rebooting", nil
})
for i := 0; i < 3; i++ {
if err := c.Sync(context.TODO(), factory.NewSyncContext("InstallerController", *eventRecorder)); err != nil {
t.Fatal(err)
}
}
if getPod() != nil {
t.Fatalf("expected no installer pod while the precondition is unmet")
}
if checkedNode != "test-node-1" {
t.Fatalf("expected precondition to be consulted for test-node-1, got %q", checkedNode)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
})

t.Run("met precondition allows installer pod", func(t *testing.T) {
c, eventRecorder, getPod := newController(func(ctx context.Context, nodeName string) (bool, string, error) {
return true, "", nil
})
for i := 0; i < 3; i++ {

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.

  1. If you set TargetRevision: 1 on the node status, you can just call Sync once IMO.
  2. In case you need a loop, use for range 3 {}

The same goes for the next test.

if err := c.Sync(context.TODO(), factory.NewSyncContext("InstallerController", *eventRecorder)); err != nil {
t.Fatal(err)
}
}
if getPod() == nil {
t.Fatalf("expected installer pod to be created when the precondition is met")
}
})

t.Run("precondition error fails sync", func(t *testing.T) {
c, eventRecorder, getPod := newController(func(ctx context.Context, nodeName string) (bool, string, error) {
return false, "", fmt.Errorf("boom")
})
var syncErr error
for i := 0; i < 3; i++ {
if syncErr = c.Sync(context.TODO(), factory.NewSyncContext("InstallerController", *eventRecorder)); syncErr != nil {
break
}
}
if syncErr == nil {
t.Fatalf("expected sync error from precondition")
}
if getPod() != nil {
t.Fatalf("expected no installer pod on precondition error")
}
})
}
11 changes: 11 additions & 0 deletions pkg/operator/staticpod/controllers.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type staticPodOperatorControllerBuilder struct {
installCommand []string
installerPodMutationFunc installer.InstallerPodMutationFunc
minReadyDuration time.Duration
installerPrecondition installer.InstallerPreconditionFunc
enableStartMonitor func() (bool, error)

// pruning information
Expand Down Expand Up @@ -114,6 +115,9 @@ type Builder interface {
WithUnrevisionedCerts(certDir string, certConfigMaps, certSecrets []installer.UnrevisionedResource) Builder
WithInstaller(command []string) Builder
WithMinReadyDuration(minReadyDuration time.Duration) Builder
// WithInstallerPrecondition sets a precondition consulted immediately before an installer pod is
// created for a node; see installer.InstallerPreconditionFunc.
WithInstallerPrecondition(precondition installer.InstallerPreconditionFunc) Builder
WithStartupMonitor(enabledStartupMonitor func() (bool, error)) Builder

// WithExtraNodeSelector Informs controllers to handle extra nodes as well as master nodes.
Expand Down Expand Up @@ -185,6 +189,11 @@ func (b *staticPodOperatorControllerBuilder) WithMinReadyDuration(minReadyDurati
return b
}

func (b *staticPodOperatorControllerBuilder) WithInstallerPrecondition(precondition installer.InstallerPreconditionFunc) Builder {
b.installerPrecondition = precondition
return b
}

func (b *staticPodOperatorControllerBuilder) WithStartupMonitor(enabledStartupMonitor func() (bool, error)) Builder {
b.enableStartMonitor = enabledStartupMonitor
return b
Expand Down Expand Up @@ -296,6 +305,8 @@ func (b *staticPodOperatorControllerBuilder) ToControllers() (manager.Controller
b.installerPodMutationFunc,
).WithMinReadyDuration(
b.minReadyDuration,
).WithInstallerPrecondition(
b.installerPrecondition,
), 1)

manager.WithController(installerstate.NewInstallerStateController(
Expand Down