Skip to content

Commit

Permalink
Switch from MergePatchOrCreate to GetAndCreateOrMergePatch
Browse files Browse the repository at this point in the history
For the reasons explained in gardener#4057, MergePatchOrCreate is not safe to use
without reading objects from the API server / cache first. Ideally we would
switch to server-side apply to simplify such operations on controlled objects
(e.g. extension resources), though we cannot leverage it for now because of
several issues described in gardener#4122.
  • Loading branch information
timebertt committed May 31, 2021
1 parent 747b41c commit 3576fd6
Show file tree
Hide file tree
Showing 18 changed files with 97 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func (a *actuator) deployBackupBucketExtensionSecret(ctx context.Context) error
}

extensionSecret := a.emptyExtensionSecret()
_, err = controllerutils.MergePatchOrCreate(ctx, a.seedClient.Client(), extensionSecret, func() error {
_, err = controllerutils.GetAndCreateOrMergePatch(ctx, a.seedClient.Client(), extensionSecret, func() error {
extensionSecret.Data = coreSecret.DeepCopy().Data
return nil
})
Expand All @@ -194,7 +194,7 @@ func (a *actuator) deployBackupBucketExtension(ctx context.Context) error {
extensionSecret := a.emptyExtensionSecret()

// reconcile extension backup bucket resource in seed
_, err := controllerutils.MergePatchOrCreate(ctx, a.seedClient.Client(), a.extensionBackupBucket, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, a.seedClient.Client(), a.extensionBackupBucket, func() error {
metav1.SetMetaDataAnnotation(&a.extensionBackupBucket.ObjectMeta, v1beta1constants.GardenerOperation, v1beta1constants.GardenerOperationReconcile)
metav1.SetMetaDataAnnotation(&a.extensionBackupBucket.ObjectMeta, v1beta1constants.GardenerTimestamp, time.Now().UTC().String())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ func (a *actuator) deployBackupEntryExtensionSecret(ctx context.Context) error {

// create secret for extension BackupEntry in seed
extensionSecret := emptyExtensionSecret(a.backupEntry)
if _, err := controllerutils.MergePatchOrCreate(ctx, a.seedClient.Client(), extensionSecret, func() error {
if _, err := controllerutils.GetAndCreateOrMergePatch(ctx, a.seedClient.Client(), extensionSecret, func() error {
extensionSecret.Data = coreSecret.DeepCopy().Data
return nil
}); err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func (b *backupEntry) Deploy(ctx context.Context) error {
}

func (b *backupEntry) deploy(ctx context.Context, operation string) (extensionsv1alpha1.Object, error) {
_, err := controllerutils.MergePatchOrCreate(ctx, b.client, b.backupEntry, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, b.client, b.backupEntry, func() error {
metav1.SetMetaDataAnnotation(&b.backupEntry.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&b.backupEntry.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
gutil "github.com/gardener/gardener/pkg/utils/gardener"
"github.com/gardener/gardener/pkg/utils/test"
. "github.com/gardener/gardener/pkg/utils/test/matchers"
apierrors "k8s.io/apimachinery/pkg/api/errors"

"github.com/golang/mock/gomock"
. "github.com/onsi/ginkgo"
Expand Down Expand Up @@ -305,12 +306,19 @@ var _ = Describe("#BackupEntry", func() {
mc := mockclient.NewMockClient(ctrl)
mc.EXPECT().Status().Return(mc)

mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("backupentries"), name))

// deploy with wait-for-state annotation
obj := expected.DeepCopy()
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/operation", "wait-for-state")
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/timestamp", now.UTC().String())
obj.TypeMeta = metav1.TypeMeta{}
test.EXPECTPatch(ctx, mc, obj, empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(obj)).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(obj))
return nil
})

// restore state
expectedWithState := obj.DeepCopy()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,10 @@ func (c *containerRuntime) Deploy(ctx context.Context) error {
}

func (c *containerRuntime) deploy(ctx context.Context, cr *extensionsv1alpha1.ContainerRuntime, coreCR gardencorev1beta1.ContainerRuntime, workerName, operation string) (extensionsv1alpha1.Object, error) {
_, err := controllerutils.MergePatchOrCreate(ctx, c.client, cr, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, c.client, cr, func() error {
metav1.SetMetaDataAnnotation(&cr.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&cr.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())

cr.Spec.BinaryPath = extensionsv1alpha1.ContainerDRuntimeContainersBinFolder
cr.Spec.Type = coreCR.Type
cr.Spec.ProviderConfig = coreCR.ProviderConfig
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"github.com/golang/mock/gomock"
"github.com/sirupsen/logrus"
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"
Expand Down Expand Up @@ -370,11 +371,18 @@ var _ = Describe("#ContainerRuntime", func() {
},
}

// deploy with wait-for-state annotation
empty.SetName(expected[0].GetName())
mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("containerruntimes"), expected[0].GetName()))

// deploy with wait-for-state annotation
expected[0].Annotations[v1beta1constants.GardenerOperation] = v1beta1constants.GardenerOperationWaitForState
expected[0].Annotations[v1beta1constants.GardenerTimestamp] = now.UTC().String()
test.EXPECTPatch(ctx, mc, expected[0], empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(expected[0])).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(expected[0]))
return nil
})

// restore state
expectedWithState := expected[0].DeepCopy()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func (c *controlPlane) deploy(ctx context.Context, operation string) (extensions
providerConfig = &runtime.RawExtension{Raw: cfg.Raw}
}

_, err := controllerutils.MergePatchOrCreate(ctx, c.client, c.controlPlane, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, c.client, c.controlPlane, func() error {
metav1.SetMetaDataAnnotation(&c.controlPlane.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&c.controlPlane.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
gutil "github.com/gardener/gardener/pkg/utils/gardener"
"github.com/gardener/gardener/pkg/utils/test"
. "github.com/gardener/gardener/pkg/utils/test/matchers"
apierrors "k8s.io/apimachinery/pkg/api/errors"

"github.com/golang/mock/gomock"
. "github.com/onsi/ginkgo"
Expand Down Expand Up @@ -414,12 +415,19 @@ var _ = Describe("ControlPlane", func() {
mc := mockclient.NewMockClient(ctrl)
mc.EXPECT().Status().Return(mc)

mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("controlplanes"), name))

// deploy with wait-for-state annotation
obj := cp.DeepCopy()
obj.Spec = cpSpec
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/operation", "wait-for-state")
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/timestamp", now.UTC().String())
test.EXPECTPatch(ctx, mc, obj, empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(obj)).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(obj))
return nil
})

// restore state
expectedWithState := obj.DeepCopy()
Expand All @@ -444,16 +452,23 @@ var _ = Describe("ControlPlane", func() {
mc := mockclient.NewMockClient(ctrl)
mc.EXPECT().Status().Return(mc)

empty.Name += "-exposure"
mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("controlplanes"), name))

// deploy with wait-for-state annotation
values.Purpose = extensionsv1alpha1.Exposure
empty.Name += "-exposure"
cp.Name += "-exposure"
obj := cp.DeepCopy()
obj.Spec = cpSpec
obj.Spec.Purpose = &values.Purpose
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/operation", "wait-for-state")
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/timestamp", now.UTC().String())
test.EXPECTPatch(ctx, mc, obj, empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(obj)).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(obj))
return nil
})

// restore state
shootState.Spec.Extensions[0].Name = &obj.Name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func (e *extension) Deploy(ctx context.Context) error {
}

func (e *extension) deploy(ctx context.Context, ext *extensionsv1alpha1.Extension, extType string, providerConfig *runtime.RawExtension, operation string) (extensionsv1alpha1.Object, error) {
_, err := controllerutils.MergePatchOrCreate(ctx, e.client, ext, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, e.client, ext, func() error {
metav1.SetMetaDataAnnotation(&ext.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&ext.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())
ext.Spec.Type = extType
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
gutil "github.com/gardener/gardener/pkg/utils/gardener"
"github.com/gardener/gardener/pkg/utils/test"
. "github.com/gardener/gardener/pkg/utils/test/matchers"
apierrors "k8s.io/apimachinery/pkg/api/errors"

"github.com/golang/mock/gomock"
"github.com/hashicorp/go-multierror"
Expand Down Expand Up @@ -310,11 +311,18 @@ var _ = Describe("Extension", func() {
mc := mockclient.NewMockClient(ctrl)
mc.EXPECT().Status().Return(mc)

// deploy with wait-for-state annotation
empty.SetName(expected[0].GetName())
mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("extensions"), empty.GetName()))

// deploy with wait-for-state annotation
expected[0].Annotations[v1beta1constants.GardenerOperation] = v1beta1constants.GardenerOperationWaitForState
expected[0].Annotations[v1beta1constants.GardenerTimestamp] = now.UTC().String()
test.EXPECTPatch(ctx, mc, expected[0], empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(expected[0])).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(expected[0]))
return nil
})

// restore state
expectedWithState := expected[0].DeepCopy()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func (i *infrastructure) deploy(ctx context.Context, operation string) (extensio
}
}

_, err := controllerutils.MergePatchOrCreate(ctx, i.client, i.infrastructure, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, i.client, i.infrastructure, func() error {
if i.values.AnnotateOperation {
metav1.SetMetaDataAnnotation(&i.infrastructure.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&i.infrastructure.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
retryfake "github.com/gardener/gardener/pkg/utils/retry/fake"
"github.com/gardener/gardener/pkg/utils/test"
. "github.com/gardener/gardener/pkg/utils/test/matchers"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
Expand Down Expand Up @@ -359,12 +360,19 @@ var _ = Describe("#Interface", func() {
values.SSHPublicKey = sshPublicKey
values.AnnotateOperation = true

mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("infrastructures"), name))

// deploy with wait-for-state annotation
obj := expected.DeepCopy()
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/operation", "wait-for-state")
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/timestamp", now.UTC().String())
obj.TypeMeta = metav1.TypeMeta{}
test.EXPECTPatch(ctx, mc, obj, empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(obj)).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(obj))
return nil
})

// restore state
expectedWithState := obj.DeepCopy()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func (n *network) WaitCleanup(ctx context.Context) error {
}

func (n *network) deploy(ctx context.Context, operation string) (extensionsv1alpha1.Object, error) {
_, err := controllerutils.MergePatchOrCreate(ctx, n.client, n.network, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, n.client, n.network, func() error {
metav1.SetMetaDataAnnotation(&n.network.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&n.network.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/sirupsen/logrus"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -301,12 +302,19 @@ var _ = Describe("#Network", func() {
mc := mockclient.NewMockClient(ctrl)
mc.EXPECT().Status().Return(mc)

mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("networks"), networkName))

// deploy with wait-for-state annotation
obj := expected.DeepCopy()
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/operation", "wait-for-state")
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/timestamp", now.UTC().String())
obj.TypeMeta = metav1.TypeMeta{}
test.EXPECTPatch(ctx, mc, obj, empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(obj)).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(obj))
return nil
})

// restore state
expectedWithState := obj.DeepCopy()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ func (d *deployer) deploy(ctx context.Context, operation string) (extensionsv1al
// the arrays as a whole.
// However, this is not a problem, as no other client should write to these arrays as the OSC spec is supposed
// to be owned by gardenlet exclusively.
_, err = controllerutils.MergePatchOrCreate(ctx, d.client, d.osc, func() error {
_, err = controllerutils.GetAndCreateOrMergePatch(ctx, d.client, d.osc, func() error {
metav1.SetMetaDataAnnotation(&d.osc.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&d.osc.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import (
gutil "github.com/gardener/gardener/pkg/utils/gardener"
"github.com/gardener/gardener/pkg/utils/imagevector"
"github.com/gardener/gardener/pkg/utils/test"
. "github.com/gardener/gardener/pkg/utils/test/matchers"
apierrors "k8s.io/apimachinery/pkg/api/errors"

"github.com/Masterminds/semver"
"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -354,14 +356,21 @@ var _ = Describe("OperatingSystemConfig", func() {
state = stateOriginal
}

// deploy with wait-for-state annotation
emptyWithName := empty.DeepCopy()
emptyWithName.SetName(expected[i].GetName())
mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(emptyWithName), gomock.AssignableToTypeOf(emptyWithName)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("operatingsystemconfigs"), emptyWithName.GetName()))

// deploy with wait-for-state annotation
obj := expected[i].DeepCopy()
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/operation", "wait-for-state")
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/timestamp", now.UTC().String())
obj.TypeMeta = metav1.TypeMeta{}
test.EXPECTPatch(ctx, mc, obj, emptyWithName, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(obj)).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(obj))
return nil
})

// restore state
expectedWithState := obj.DeepCopy()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func (w *worker) deploy(ctx context.Context, operation string) (extensionsv1alph
// the arrays as a whole.
// However, this is not a problem, as no other client should write to these arrays as the Worker spec is supposed
// to be owned by gardenlet exclusively.
_, err := controllerutils.MergePatchOrCreate(ctx, w.client, w.worker, func() error {
_, err := controllerutils.GetAndCreateOrMergePatch(ctx, w.client, w.worker, func() error {
metav1.SetMetaDataAnnotation(&w.worker.ObjectMeta, v1beta1constants.GardenerOperation, operation)
metav1.SetMetaDataAnnotation(&w.worker.ObjectMeta, v1beta1constants.GardenerTimestamp, TimeNow().UTC().String())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
gutil "github.com/gardener/gardener/pkg/utils/gardener"
"github.com/gardener/gardener/pkg/utils/test"
. "github.com/gardener/gardener/pkg/utils/test/matchers"
apierrors "k8s.io/apimachinery/pkg/api/errors"

"github.com/Masterminds/semver"
"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -464,13 +465,20 @@ var _ = Describe("Worker", func() {
mc := mockclient.NewMockClient(ctrl)
mc.EXPECT().Status().Return(mc)

mc.EXPECT().Get(ctx, client.ObjectKeyFromObject(empty), gomock.AssignableToTypeOf(empty)).
Return(apierrors.NewNotFound(extensionsv1alpha1.Resource("workers"), name))

// deploy with wait-for-state annotation
obj := w.DeepCopy()
obj.Spec = wSpec
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/operation", "wait-for-state")
metav1.SetMetaDataAnnotation(&obj.ObjectMeta, "gardener.cloud/timestamp", now.UTC().String())
obj.TypeMeta = metav1.TypeMeta{}
test.EXPECTPatch(ctx, mc, obj, empty, types.MergePatchType)
mc.EXPECT().Create(ctx, test.HasObjectKeyOf(obj)).
DoAndReturn(func(ctx context.Context, actual client.Object, opts ...client.CreateOption) error {
Expect(actual).To(DeepEqual(obj))
return nil
})

// restore state
expectedWithState := obj.DeepCopy()
Expand Down

0 comments on commit 3576fd6

Please sign in to comment.