Skip to content

Commit 61821f3

Browse files
committed
change api CreateOrUpdateMantleBackup to CreateMantleBackup
After created MB, there is no need to update it. And We Found a bug caused by overwriting existing annotations Signed-off-by: Yuji Ito <llamerada.jp@gmail.com>
1 parent 9114318 commit 61821f3

8 files changed

Lines changed: 140 additions & 142 deletions

File tree

docs/controller-protocol.md

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44
## Table of Contents
55

66
- [pkg/controller/proto/controller.proto](#pkg_controller_proto_controller-proto)
7-
- [CreateOrUpdateMantleBackupRequest](#proto-CreateOrUpdateMantleBackupRequest)
8-
- [CreateOrUpdateMantleBackupResponse](#proto-CreateOrUpdateMantleBackupResponse)
7+
- [CreateMantleBackupRequest](#proto-CreateMantleBackupRequest)
8+
- [CreateMantleBackupResponse](#proto-CreateMantleBackupResponse)
99
- [CreateOrUpdatePVCRequest](#proto-CreateOrUpdatePVCRequest)
1010
- [CreateOrUpdatePVCResponse](#proto-CreateOrUpdatePVCResponse)
1111
- [ListMantleBackupRequest](#proto-ListMantleBackupRequest)
@@ -26,10 +26,10 @@
2626

2727

2828

29-
<a name="proto-CreateOrUpdateMantleBackupRequest"></a>
29+
<a name="proto-CreateMantleBackupRequest"></a>
3030

31-
### CreateOrUpdateMantleBackupRequest
32-
CreateOrUpdateMantleBackupRequest is a request message for CreateOrUpdateMantleBackup RPC.
31+
### CreateMantleBackupRequest
32+
CreateMantleBackupRequest is a request message for CreateMantleBackup RPC.
3333

3434

3535
| Field | Type | Label | Description |
@@ -41,10 +41,10 @@ CreateOrUpdateMantleBackupRequest is a request message for CreateOrUpdateMantleB
4141

4242

4343

44-
<a name="proto-CreateOrUpdateMantleBackupResponse"></a>
44+
<a name="proto-CreateMantleBackupResponse"></a>
4545

46-
### CreateOrUpdateMantleBackupResponse
47-
CreateOrUpdateMantleBackupResponse is a response message for CreateOrUpdateMantleBackup RPC.
46+
### CreateMantleBackupResponse
47+
CreateMantleBackupResponse is a response message for CreateMantleBackup RPC.
4848

4949
nothing.
5050

@@ -155,7 +155,7 @@ SetSynchronizingResponse is a response message for SetSynchronize RPC.
155155
| Method Name | Request Type | Response Type | Description |
156156
| ----------- | ------------ | ------------- | ------------|
157157
| CreateOrUpdatePVC | [CreateOrUpdatePVCRequest](#proto-CreateOrUpdatePVCRequest) | [CreateOrUpdatePVCResponse](#proto-CreateOrUpdatePVCResponse) | |
158-
| CreateOrUpdateMantleBackup | [CreateOrUpdateMantleBackupRequest](#proto-CreateOrUpdateMantleBackupRequest) | [CreateOrUpdateMantleBackupResponse](#proto-CreateOrUpdateMantleBackupResponse) | |
158+
| CreateMantleBackup | [CreateMantleBackupRequest](#proto-CreateMantleBackupRequest) | [CreateMantleBackupResponse](#proto-CreateMantleBackupResponse) | |
159159
| ListMantleBackup | [ListMantleBackupRequest](#proto-ListMantleBackupRequest) | [ListMantleBackupResponse](#proto-ListMantleBackupResponse) | |
160160
| SetSynchronizing | [SetSynchronizingRequest](#proto-SetSynchronizingRequest) | [SetSynchronizingResponse](#proto-SetSynchronizingResponse) | |
161161

internal/controller/mantlebackup_controller.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -617,10 +617,10 @@ func (r *MantleBackupReconciler) replicateManifests(
617617
return ctrl.Result{}, err
618618
}
619619

620-
// Call CreateOrUpdateMantleBackup.
621-
if _, err := client.CreateOrUpdateMantleBackup(
620+
// Call CreateMantleBackup.
621+
if _, err := client.CreateMantleBackup(
622622
ctx,
623-
&proto.CreateOrUpdateMantleBackupRequest{
623+
&proto.CreateMantleBackupRequest{
624624
MantleBackup: backupSentJson,
625625
},
626626
); err != nil {

internal/controller/mantlebackup_controller_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -494,21 +494,21 @@ var _ = Describe("MantleBackup controller", func() {
494494
Uid: "a7c9d5e2-4b8f-4e2a-9d3f-1b6a7c8e9f2b",
495495
}, nil)
496496
secondaryBackups := []*mantlev1.MantleBackup{}
497-
grpcClient.EXPECT().CreateOrUpdateMantleBackup(gomock.Any(), gomock.Any()).
497+
grpcClient.EXPECT().CreateMantleBackup(gomock.Any(), gomock.Any()).
498498
MinTimes(1).
499499
DoAndReturn(
500500
func(
501501
ctx context.Context,
502-
req *proto.CreateOrUpdateMantleBackupRequest,
502+
req *proto.CreateMantleBackupRequest,
503503
opts ...grpc.CallOption,
504-
) (*proto.CreateOrUpdateMantleBackupResponse, error) {
504+
) (*proto.CreateMantleBackupResponse, error) {
505505
var secondaryBackup mantlev1.MantleBackup
506506
err := json.Unmarshal(req.GetMantleBackup(), &secondaryBackup)
507507
if err != nil {
508508
panic(err)
509509
}
510510
secondaryBackups = append(secondaryBackups, &secondaryBackup)
511-
return &proto.CreateOrUpdateMantleBackupResponse{}, nil
511+
return &proto.CreateMantleBackupResponse{}, nil
512512
})
513513
grpcClient.EXPECT().ListMantleBackup(gomock.Any(), gomock.Any()).
514514
MinTimes(1).

internal/controller/replication.go

Lines changed: 51 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/cybozu-go/mantle/pkg/controller/proto"
1111
"google.golang.org/grpc"
1212
corev1 "k8s.io/api/core/v1"
13+
aerrors "k8s.io/apimachinery/pkg/api/errors"
1314
"k8s.io/apimachinery/pkg/labels"
1415
"k8s.io/apimachinery/pkg/types"
1516
ctrl "sigs.k8s.io/controller-runtime"
@@ -109,10 +110,10 @@ func (s *SecondaryServer) CreateOrUpdatePVC(
109110
return &proto.CreateOrUpdatePVCResponse{Uid: string(pvc.GetUID())}, nil
110111
}
111112

112-
func (s *SecondaryServer) CreateOrUpdateMantleBackup(
113+
func (s *SecondaryServer) CreateMantleBackup(
113114
ctx context.Context,
114-
req *proto.CreateOrUpdateMantleBackupRequest,
115-
) (*proto.CreateOrUpdateMantleBackupResponse, error) {
115+
req *proto.CreateMantleBackupRequest,
116+
) (*proto.CreateMantleBackupResponse, error) {
116117
var backupReceived mantlev1.MantleBackup
117118
if err := json.Unmarshal(req.MantleBackup, &backupReceived); err != nil {
118119
return nil, err
@@ -139,62 +140,59 @@ func (s *SecondaryServer) CreateOrUpdateMantleBackup(
139140
annotRemoteUID, backupReceived.GetName(), backupReceived.GetNamespace())
140141
}
141142

142-
var backup mantlev1.MantleBackup
143-
backup.SetName(backupReceived.GetName())
144-
backup.SetNamespace(backupReceived.GetNamespace())
145-
if _, err := ctrl.CreateOrUpdate(ctx, s.client, &backup, func() error {
146-
if !backup.CreationTimestamp.IsZero() {
147-
errMsg := ""
148-
if backup.Labels == nil {
149-
errMsg = "labels field is nil in backup"
150-
} else {
151-
pvcUID, ok := backup.Labels[labelLocalBackupTargetPVCUID]
152-
if !ok {
153-
errMsg = "label not found"
154-
} else if pvcUID != pvcUIDReceived {
155-
errMsg = "label not matched"
156-
}
157-
}
158-
if errMsg != "" {
159-
return fmt.Errorf("%s: %s: %s: %s",
160-
errMsg, labelLocalBackupTargetPVCUID, backupReceived.GetName(), backupReceived.GetNamespace())
161-
}
162-
163-
if backup.Annotations == nil {
164-
errMsg = "annotation field is nil in backup"
165-
} else {
166-
remoteUID, ok := backup.Annotations[annotRemoteUID]
167-
if !ok {
168-
errMsg = "annotation not found in backup"
169-
} else if remoteUID != remoteUIDReceived {
170-
errMsg = "annotation not matched"
171-
}
172-
}
173-
if errMsg != "" {
174-
return fmt.Errorf("%s: %s: %s: %s",
175-
errMsg, annotRemoteUID, backupReceived.GetName(), backupReceived.GetNamespace())
176-
}
143+
var backupExists mantlev1.MantleBackup
144+
if err := s.client.Get(ctx, types.NamespacedName{
145+
Namespace: backupReceived.GetNamespace(),
146+
Name: backupReceived.GetName(),
147+
}, &backupExists); err != nil {
148+
if !aerrors.IsNotFound(err) {
149+
return nil, fmt.Errorf("failed to get MantleBackup: %w", err)
150+
}
151+
// Not found, create a new one.
152+
if err := s.client.Create(ctx, &backupReceived); err != nil {
153+
return nil, fmt.Errorf("failed to create MantleBackup: %w", err)
177154
}
155+
if err := s.client.Status().Update(ctx, &backupReceived); err != nil {
156+
return nil, fmt.Errorf("failed to update status of MantleBackup: %w", err)
157+
}
158+
return &proto.CreateMantleBackupResponse{}, nil
159+
}
178160

179-
backup.Finalizers = backupReceived.Finalizers
180-
backup.Annotations = backupReceived.Annotations
181-
backup.Labels = backupReceived.Labels
182-
backup.Spec = backupReceived.Spec
183-
return nil
184-
}); err != nil {
185-
return nil, fmt.Errorf("CreateOrUpdate failed: %w", err)
161+
// Found, check it.
162+
if backupExists.Labels == nil || backupExists.Annotations == nil {
163+
return nil, fmt.Errorf("both labels and annotations must not be nil in the existing MantleBackup: %s/%s",
164+
backupReceived.GetNamespace(), backupReceived.GetName())
165+
}
166+
167+
if pvcUID, ok := backupExists.Labels[labelLocalBackupTargetPVCUID]; !ok {
168+
return nil, fmt.Errorf("label %s not found in the existing MantleBackup: %s/%s",
169+
labelLocalBackupTargetPVCUID, backupReceived.GetNamespace(), backupReceived.GetName())
170+
} else if pvcUID != pvcUIDReceived {
171+
return nil, fmt.Errorf("label %s not matched in the existing MantleBackup: %s/%s",
172+
labelLocalBackupTargetPVCUID, backupReceived.GetNamespace(), backupReceived.GetName())
186173
}
187174

188-
// Use Patch here because updateStatus is likely to fail due to "the object has been modified" error.
189-
newBackup := backup.DeepCopy()
190-
newBackup.Status.CreatedAt = backupReceived.Status.CreatedAt
191-
newBackup.Status.SnapSize = backupReceived.Status.SnapSize
192-
newBackup.Status.TransferPartSize = backupReceived.Status.TransferPartSize
193-
if err := s.client.Status().Patch(ctx, newBackup, client.MergeFrom(&backup)); err != nil {
194-
return nil, fmt.Errorf("status patch failed: %w", err)
175+
if remoteUID, ok := backupExists.Annotations[annotRemoteUID]; !ok {
176+
return nil, fmt.Errorf("annotation %s not found in the existing MantleBackup: %s/%s",
177+
annotRemoteUID, backupReceived.GetNamespace(), backupReceived.GetName())
178+
} else if remoteUID != remoteUIDReceived {
179+
return nil, fmt.Errorf("annotation %s not matched in the existing MantleBackup: %s/%s",
180+
annotRemoteUID, backupReceived.GetNamespace(), backupReceived.GetName())
181+
}
182+
183+
// Update status if not set.
184+
if backupExists.Status.CreatedAt.IsZero() {
185+
newBackup := backupExists.DeepCopy()
186+
newBackup.Status.CreatedAt = backupReceived.Status.CreatedAt
187+
newBackup.Status.SnapSize = backupReceived.Status.SnapSize
188+
newBackup.Status.TransferPartSize = backupReceived.Status.TransferPartSize
189+
if err := s.client.Status().Patch(ctx, newBackup, client.MergeFrom(&backupExists)); err != nil {
190+
return nil, fmt.Errorf("status patch failed: %w", err)
191+
}
195192
}
196193

197-
return &proto.CreateOrUpdateMantleBackupResponse{}, nil
194+
// Already exists with matching pvc-uid and remote-uid, do nothing.
195+
return &proto.CreateMantleBackupResponse{}, nil
198196
}
199197

200198
func (s *SecondaryServer) ListMantleBackup(

pkg/controller/proto/controller.pb.go

Lines changed: 36 additions & 36 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)