Skip to content

fix pausing reconciliation for resource migrations #70

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

Merged
bwagner5 merged 2 commits into from
Aug 10, 2026
Merged

fix pausing reconciliation for resource migrations #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29bfcd3
fix pausing reconciliation for resource migrations
bwagner5 Aug 7, 2026
c56370c
change asserts to require in new tests
bwagner5 Aug 10, 2026
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
2 changes: 2 additions & 0 deletions api/v1alpha1/oxidecluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ type OxideClusterInitializationStatus struct {
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/v1beta1=v1alpha1"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/v1beta2=v1alpha1"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/provider=infrastructure-oxide"
// +kubebuilder:metadata:labels="clusterctl.cluster.x-k8s.io="
// OxideCluster is the Schema for the oxideclusters API
type OxideCluster struct {
metav1.TypeMeta `json:",inline"`
Expand Down
2 changes: 2 additions & 0 deletions api/v1alpha1/oxidemachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ type OxideMachineStatus struct {
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/v1beta1=v1alpha1"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/v1beta2=v1alpha1"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/provider=infrastructure-oxide"
// +kubebuilder:metadata:labels="clusterctl.cluster.x-k8s.io="
// OxideMachine is the Schema for the oxidemachines API
type OxideMachine struct {
metav1.TypeMeta `json:",inline"`
Expand Down
2 changes: 2 additions & 0 deletions api/v1alpha1/oxidemachinetemplate_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ type OxideMachineTemplateResource struct {
// +kubebuilder:subresource:status
// +kubebuilder:metadata:labels="cluster.x-k8s.io/v1beta1=v1alpha1"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/v1beta2=v1alpha1"
// +kubebuilder:metadata:labels="cluster.x-k8s.io/provider=infrastructure-oxide"
// +kubebuilder:metadata:labels="clusterctl.cluster.x-k8s.io="
// OxideMachineTemplate is the Schema for the oxidemachinetemplates API
type OxideMachineTemplate struct {
metav1.TypeMeta `json:",inline"`
Expand Down
2 changes: 2 additions & 0 deletions charts/cluster-api-provider-oxide/crds/infrastructure.cluster.x-k8s.io_oxideclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.21.0
labels:
cluster.x-k8s.io/provider: infrastructure-oxide
cluster.x-k8s.io/v1beta1: v1alpha1
cluster.x-k8s.io/v1beta2: v1alpha1
clusterctl.cluster.x-k8s.io: ""
name: oxideclusters.infrastructure.cluster.x-k8s.io
spec:
group: infrastructure.cluster.x-k8s.io
Expand Down
2 changes: 2 additions & 0 deletions charts/cluster-api-provider-oxide/crds/infrastructure.cluster.x-k8s.io_oxidemachines.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.21.0
labels:
cluster.x-k8s.io/provider: infrastructure-oxide
cluster.x-k8s.io/v1beta1: v1alpha1
cluster.x-k8s.io/v1beta2: v1alpha1
clusterctl.cluster.x-k8s.io: ""
name: oxidemachines.infrastructure.cluster.x-k8s.io
spec:
group: infrastructure.cluster.x-k8s.io
Expand Down
2 changes: 2 additions & 0 deletions ...luster-api-provider-oxide/crds/infrastructure.cluster.x-k8s.io_oxidemachinetemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.21.0
labels:
cluster.x-k8s.io/provider: infrastructure-oxide
cluster.x-k8s.io/v1beta1: v1alpha1
cluster.x-k8s.io/v1beta2: v1alpha1
clusterctl.cluster.x-k8s.io: ""
name: oxidemachinetemplates.infrastructure.cluster.x-k8s.io
spec:
group: infrastructure.cluster.x-k8s.io
Expand Down
75 changes: 58 additions & 17 deletions internal/controller/oxidecluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ import (
"sigs.k8s.io/cluster-api/util"
"sigs.k8s.io/cluster-api/util/conditions"
"sigs.k8s.io/cluster-api/util/patch"
"sigs.k8s.io/cluster-api/util/paused"
"sigs.k8s.io/cluster-api/util/predicates"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/handler"
Expand Down Expand Up @@ -81,6 +84,27 @@ func (r *OxideClusterReconciler) Reconcile(
return ctrl.Result{}, err
}

cluster, err := util.GetOwnerCluster(ctx, r.Client, oxideCluster.ObjectMeta)
if err != nil {
return ctrl.Result{}, err
}
if cluster == nil {
log.Info("missing ownerRef on OxideCluster", "name", oxideCluster.Name)
return ctrl.Result{}, nil
Comment on lines +92 to +93

@lgfa29 lgfa29 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this an expected scenario? Anything actionable we expect the user to do? If not, we may consider dropping this log to a debug?

@bwagner5 bwagner5 Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, a lot of the logging is not great right now. It's using controller-runtimes logr which has the integer verbosity levels (kinda hate that). Rather than mapping those, I want to take a pass at switching to zap as the backend or something else. Let me put an issue up to fix logging more holistically.

@bwagner5 bwagner5 Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#72

}

// Set the Paused condition and return early if paused, e.g. during clusterctl move.
if isPaused, requeue, err := paused.EnsurePausedCondition(
Comment thread
sudomateo marked this conversation as resolved.
ctx,
r.Client,
cluster,
oxideCluster,
); err != nil ||
isPaused ||
requeue {
return ctrl.Result{}, err
}

patchHelper, err := patch.NewHelper(oxideCluster, r.Client)
if err != nil {
return ctrl.Result{}, fmt.Errorf("building patch helper: %w", err)
Expand All @@ -91,15 +115,6 @@ func (r *OxideClusterReconciler) Reconcile(
}
}()

cluster, err := util.GetOwnerCluster(ctx, r.Client, oxideCluster.ObjectMeta)
if err != nil {
return ctrl.Result{}, err
}
if cluster == nil {
log.Info("missing ownerRef on OxideCluster", "name", oxideCluster.Name)
return ctrl.Result{}, nil
}

oxideClient, err := r.OxideClientFactory(ctx, r.Client, oxideCluster)
if err != nil {
return ctrl.Result{}, err
Expand Down Expand Up @@ -151,8 +166,23 @@ func (r *OxideClusterReconciler) Reconcile(
Reason: infrav1.ReasonFloatingIPProvisioned,
})

// Ensure floating IP is attached to an instance. Use the 0th ready control plane machine if
// unattached.
if err := r.reconcileFloatingIPAttachment(ctx, oxideClient, oxideCluster, ip); err != nil {
return ctrl.Result{}, err
}

return ctrl.Result{}, nil
}

// reconcileFloatingIPAttachment ensures the floating IP is attached to a provisioned control plane
// instance, using the 0th ready one if unattached, and sets the FloatingIPAttached condition.
func (r *OxideClusterReconciler) reconcileFloatingIPAttachment(
ctx context.Context,
oxideClient cloud.OxideClient,
oxideCluster *infrav1.OxideCluster,
ip *oxide.FloatingIp,
) error {
log := logf.FromContext(ctx)

shouldAttach := true
var machines infrav1.OxideMachineList

Expand All @@ -167,14 +197,14 @@ func (r *OxideClusterReconciler) Reconcile(
clusterv1.MachineControlPlaneLabel: "",
},
); err != nil {
return ctrl.Result{}, fmt.Errorf("listing oxide machines: %w", err)
return fmt.Errorf("listing oxide machines: %w", err)
}
if ip.InstanceId != "" {
for _, machine := range machines.Items {
if machine.Spec.ProviderID != "" {
instanceID, err := cloud.InstanceIDFromProviderID(machine.Spec.ProviderID)
if err != nil {
return ctrl.Result{}, fmt.Errorf("parsing provider id: %w", err)
return fmt.Errorf("parsing provider id: %w", err)
Comment on lines 205 to +207

@lgfa29 lgfa29 Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to this PR, but mostly out of curiosity. This doesn't seem like we expect it to happen, but if it does happen, it may be better to ignore the node and keep looking for a match? If we find a match then we're good, if we don't find a match and get an error here, we may choose to bail.

@bwagner5 bwagner5 Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are correct. This could definitely break in a real cluster pretty easily. Good catch! Will fix in a follow-up PR.

}
if instanceID == ip.InstanceId {
shouldAttach = false
Expand All @@ -199,11 +229,12 @@ func (r *OxideClusterReconciler) Reconcile(
"instance",
ip.InstanceId,
)
var err error
ip, err = oxideClient.FloatingIpDetach(ctx, oxide.FloatingIpDetachParams{
FloatingIp: oxide.NameOrId(ip.Id),
})
if err != nil {
return ctrl.Result{}, fmt.Errorf("detaching floating ip: %w", err)
return fmt.Errorf("detaching floating ip: %w", err)
}
}

Expand Down Expand Up @@ -236,7 +267,7 @@ func (r *OxideClusterReconciler) Reconcile(
}
instanceID, err := cloud.InstanceIDFromProviderID(machine.Spec.ProviderID)
if err != nil {
return ctrl.Result{}, fmt.Errorf("parsing provider id: %w", err)
return fmt.Errorf("parsing provider id: %w", err)
}
log.Info("attaching floating IP", "ip", ip.Ip, "instance", instanceID)
ip, err = oxideClient.FloatingIpAttach(ctx, oxide.FloatingIpAttachParams{
Expand All @@ -247,7 +278,7 @@ func (r *OxideClusterReconciler) Reconcile(
},
})
if err != nil {
return ctrl.Result{}, err
return err
}
break
}
Expand All @@ -271,7 +302,7 @@ func (r *OxideClusterReconciler) Reconcile(
})
}

return ctrl.Result{}, nil
return nil
}

// floatingIPAllocator builds an oxide.Allocator to provision the floating IP address:
Expand Down Expand Up @@ -365,11 +396,21 @@ func (r *OxideClusterReconciler) ensureFloatingIPDeleted(

// SetupWithManager sets up the controller with the Manager.
func (r *OxideClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
log := mgr.GetLogger().WithValues("controller", "oxidecluster")
return ctrl.NewControllerManagedBy(mgr).
For(&infrav1.OxideCluster{}).
Watches(&infrav1.OxideMachine{}, handler.EnqueueRequestsFromMapFunc(
r.oxideMachineToOxideCluster,
)).
// Reconcile on Cluster pause transitions, e.g. to resume after a clusterctl move unpauses.
Watches(&clusterv1.Cluster{}, handler.EnqueueRequestsFromMapFunc(
util.ClusterToInfrastructureMapFunc(
context.Background(),
infrav1.GroupVersion.WithKind("OxideCluster"),
mgr.GetClient(),
&infrav1.OxideCluster{},
),
), builder.WithPredicates(predicates.ClusterPausedTransitions(mgr.GetScheme(), log))).
Named("oxidecluster").
Complete(r)
}
Expand Down
114 changes: 105 additions & 9 deletions internal/controller/oxidecluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,25 @@ package controller

import (
"context"
"errors"
"net/http"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
"sigs.k8s.io/cluster-api/util/conditions"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"

infrav1 "github.com/oxidecomputer/cluster-api-provider-oxide/api/v1alpha1"
"github.com/oxidecomputer/cluster-api-provider-oxide/internal/cloud"
"github.com/oxidecomputer/cluster-api-provider-oxide/internal/cloud/mock"
"github.com/oxidecomputer/oxide.go/oxide"
clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
)

// httpErr constructs an *oxide.HTTPError with a stub HTTPResponse so that its
Expand All @@ -39,6 +48,93 @@ func httpErr(code string) *oxide.HTTPError {
}
}

// newPauseTestScheme builds a scheme with the CAPI and Oxide types registered.
func newPauseTestScheme(t *testing.T) *runtime.Scheme {
t.Helper()
scheme := runtime.NewScheme()
require.NoError(t, clusterv1.AddToScheme(scheme))
require.NoError(t, infrav1.AddToScheme(scheme))
return scheme
}

// getPausedCondition re-fetches obj and returns its Paused condition, or nil if unset.
func getPausedCondition(t *testing.T, c client.Client, obj interface {
client.Object
conditions.Getter
}) *metav1.Condition {
t.Helper()
if err := c.Get(context.Background(), client.ObjectKeyFromObject(obj), obj); err != nil {
t.Fatalf("getting %T: %v", obj, err)
}
return conditions.Get(obj, clusterv1.PausedCondition)
}

func TestOxideClusterReconcilePaused(t *testing.T) {
scheme := newPauseTestScheme(t)
cluster := &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"},
Spec: clusterv1.ClusterSpec{Paused: new(true)},
}
oxideCluster := &infrav1.OxideCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
OwnerReferences: []metav1.OwnerReference{{
APIVersion: clusterv1.GroupVersion.String(),
Kind: "Cluster",
Name: "test",
UID: "test-uid",
}},
},
}
k8sClient := fake.NewClientBuilder().
WithScheme(scheme).
WithObjects(cluster, oxideCluster).
WithStatusSubresource(&infrav1.OxideCluster{}).
Build()

factoryCalls := 0
r := &OxideClusterReconciler{
Client: k8sClient,
Scheme: scheme,
OxideClientFactory: func(context.Context, client.Client, *infrav1.OxideCluster) (cloud.OxideClient, error) {
factoryCalls++
return nil, errors.New("halting test reconcile")
},
}
ctx := context.Background()
req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "default", Name: "test"}}

// While paused, the first reconcile sets the Paused condition and requeues, and subsequent
// reconciles skip. The Oxide client must never be constructed.
for range 2 {
result, err := r.Reconcile(ctx, req)
require.NoError(t, err)
require.Equal(t, ctrl.Result{}, result)
}
require.Equal(t, 0, factoryCalls)
cond := getPausedCondition(t, k8sClient, oxideCluster)
require.NotNil(t, cond)
require.Equal(t, metav1.ConditionTrue, cond.Status)

// Unpause the Cluster. The next reconcile only flips the Paused condition; the one after
// resumes normal reconciliation and constructs the Oxide client.
require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(cluster), cluster))
cluster.Spec.Paused = new(false)
require.NoError(t, k8sClient.Update(ctx, cluster))

_, err := r.Reconcile(ctx, req)
require.NoError(t, err)
require.Equal(t, 0, factoryCalls)
cond = getPausedCondition(t, k8sClient, oxideCluster)
require.NotNil(t, cond)
require.Equal(t, metav1.ConditionFalse, cond.Status)

_, err = r.Reconcile(ctx, req)
require.ErrorContains(t, err, "halting test reconcile")
require.Equal(t, 1, factoryCalls)
}

func TestEnsureFloatingIPExists(t *testing.T) {
wantIP := &oxide.FloatingIp{
Id: "ip-id",
Expand Down Expand Up @@ -96,11 +192,11 @@ func TestEnsureFloatingIPExists(t *testing.T) {
"ip-name",
)
if tc.wantErr != "" {
assert.ErrorContains(t, gotErr, tc.wantErr)
assert.Nil(t, gotIP)
require.ErrorContains(t, gotErr, tc.wantErr)
require.Nil(t, gotIP)
} else {
assert.NoError(t, gotErr)
assert.Equal(t, wantIP, gotIP)
require.NoError(t, gotErr)
require.Equal(t, wantIP, gotIP)
}
})
}
Expand Down Expand Up @@ -145,9 +241,9 @@ func TestEnsureFloatingIPDeleted(t *testing.T) {
"ip-name",
)
if tc.wantErr != "" {
assert.ErrorContains(t, gotErr, tc.wantErr)
require.ErrorContains(t, gotErr, tc.wantErr)
} else {
assert.NoError(t, gotErr)
require.NoError(t, gotErr)
}
})
}
Expand Down Expand Up @@ -224,7 +320,7 @@ func TestFloatingIPAllocator(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
got := floatingIPAllocator(tc.cluster)
assert.Equal(t, tc.want, got)
require.Equal(t, tc.want, got)
})
}
}
Loading