Support MantleBackupConfig #11

ushitora-anqou · 2024-06-12T03:01:38Z

No description provided.

satoru-takeuchi · 2024-06-26T06:45:39Z

@ushitora-anqou Could you add signed-off-by in the 2nd and the 3rd commits?

ushitora-anqou · 2024-06-26T07:11:16Z

@ushitora-anqou Could you add signed-off-by in the 2nd and the 3rd commits?

I'll fix it later. That being said, we may need to introduce DCO GitHub App here, like TopoLVM.

api/v1/mantlebackupconfig_types.go

satoru-takeuchi · 2024-06-26T20:52:58Z

cmd/backupandrotate/main.go

+
+func getMBName(mbc *mantlev1.MantleBackupConfig, jobID string) string {
+	name := mbc.GetName()
+	if len(name) >= 43 {


Suggested change

if len(name) >= 43 {

if len(name) > 43 {

cmd/backupandrotate/main.go

satoru-takeuchi · 2024-06-26T21:08:37Z

cmd/backupandrotate/main.go

+		return "", fmt.Errorf("JOB_NAME not found")
+	}
+	if len(jobName) < 8 {
+		return "", fmt.Errorf("the length of JOB_NAME is smaller than 8")


Suggested change

return "", fmt.Errorf("the length of JOB_NAME is smaller than 8")

return "", fmt.Errorf("the length of JOB_NAME must be >= 8")

To let users know what they should do by reading this message clearer.

satoru-takeuchi · 2024-06-26T21:18:05Z

cmd/backupandrotate/main.go

+		return fmt.Errorf("can't get mbc: %s: %s: %w", mbcName, mbcNamespace, err)
+	}
+
+	if err := backup(ctx, cli, &mbc); err != nil {


Suggested change

if err := backup(ctx, cli, &mbc); err != nil {

if err := createMantleBackup(ctx, cli, &mbc); err != nil {

This function doesn't create backup directly. So it's better to clarify this function creates MantleBackup resource.

satoru-takeuchi · 2024-06-26T21:40:18Z

cmd/backupandrotate/main.go

+
+	// At this point we know that mb was created by the previous run of the current job and we're retrying it.
+	// Thus, it is safe to ignore this "already exists" error.
+	logger.Info("avoid creating a new MantleBackup because it already exists",


Suggested change

logger.Info("avoid creating a new MantleBackup because it already exists",

logger.Info("MantleBackup already exists",

Actualy we tried to create MantleBackup. So omitting "avoid ..." is better.

satoru-takeuchi · 2024-06-26T21:41:37Z

cmd/backupandrotate/main.go

+	mbc *mantlev1.MantleBackupConfig,
+	expireOffset time.Duration,
+) error {
+	// List all MantleBackup objects associated with mbc.


Suggested change

// List all MantleBackup objects associated with mbc.

// List all MantleBackup objects associated with the mbc.

satoru-takeuchi · 2024-06-26T23:43:04Z

cmd/backupandrotate/main.go

+	flags.StringVar(&mbcName, "name", "", "MantleBackupConfig resource's name")
+	flags.StringVar(&mbcNamespace, "namespace", "", "MantleBackupConfig resource's namespace")
+	flags.StringVar(&expireOffset, "expire-offset", "0s",
+		"An offset for expire field. Use this option for testing purposes only.")


How about writing down more strict meaning of this paramter? It would be like this: mb's expire becomes mb.spec.expire - expore-offset.

satoru-takeuchi · 2024-06-27T00:00:59Z

internal/controller/mantlebackupconfig_controller.go

+// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
+func (r *MantleBackupConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	// Get the running pod info
+	runningPod, err := getRunningPod(ctx, r.Client)


Suggested change

runningPod, err := getRunningPod(ctx, r.Client)

cronJobInfo, err := getCronJobInfo(ctx, r.Client)

How about remaning the function to exlain the role of this function clearly? In addition, it's better to define the dedicated struct for cronJobInfo because this informatino is not about the controller's pod itself. The new struct would be like this.

struct cronJobInfo { Namespace ServiceAccountName Image }

I mostly agree with you. We should probably define two separate functions: getCronJobInfo and getRunningPod. Then, getCronJobInfo can call getRunningPod. This way, getRunningPod can be reused for other purposes, even though it's currently only used for creating CronJobs.

satoru-takeuchi · 2024-06-27T00:04:47Z

internal/controller/mantlebackupconfig_controller.go

+		return ctrl.Result{}, fmt.Errorf("failed to get Ceph cluster ID: %s: %s: %w", mbc.Namespace, mbc.Spec.PVC, err)
+	}
+	if clusterID != r.managedCephClusterID {
+		logger.Info("mbc not managed", "name", req.Name, "namespace", req.Namespace, "error", err)


Suggested change

logger.Info("mbc not managed", "name", req.Name, "namespace", req.Namespace, "error", err)

logger.Info("the target pvc is not managed by this controller", "name", req.Name, "namespace", req.Namespace, "error", err)

satoru-takeuchi · 2024-06-27T00:17:56Z

@ushitora-anqou I didn't check test code since they will be changed after reflecting my comments.

satoru-takeuchi · 2024-06-27T07:33:46Z

cmd/backupandrotate/main.go

+
+	parsedExpireOffset, err := strfmt.ParseDuration(expireOffset)
+	if err != nil {
+		return fmt.Errorf("can't parse the expire offset: %w", err)


Suggested change

return fmt.Errorf("can't parse the expire offset: %w", err)

return fmt.Errorf("couldn't parse the expire offset: %w", err)

satoru-takeuchi · 2024-06-27T07:33:56Z

cmd/backupandrotate/main.go

+
+	cli, err := client.New(config.GetConfigOrDie(), client.Options{Scheme: scheme})
+	if err != nil {
+		return fmt.Errorf("can't create a new client: %w", err)


Suggested change

return fmt.Errorf("can't create a new client: %w", err)

return fmt.Errorf("couldn't create a new client: %w", err)

satoru-takeuchi · 2024-06-27T07:34:11Z

cmd/backupandrotate/main.go

+	// Get the target mbc.
+	var mbc mantlev1.MantleBackupConfig
+	if err := cli.Get(ctx, types.NamespacedName{Name: mbcName, Namespace: mbcNamespace}, &mbc); err != nil {
+		return fmt.Errorf("can't get mbc: %s: %s: %w", mbcName, mbcNamespace, err)


Suggested change

return fmt.Errorf("can't get mbc: %s: %s: %w", mbcName, mbcNamespace, err)

return fmt.Errorf("couldn't get mbc: %s: %s: %w", mbcName, mbcNamespace, err)

satoru-takeuchi · 2024-06-27T07:35:58Z

cmd/backupandrotate/main.go

+		return nil
+	}
+	if !errors.IsAlreadyExists(err) {
+		return fmt.Errorf("can't create a MantleBackup: %s: %s: %w", mbName, mbNamespace, err)


Suggested change

return fmt.Errorf("can't create a MantleBackup: %s: %s: %w", mbName, mbNamespace, err)

return fmt.Errorf("couldn't create a MantleBackup: %s: %s: %w", mbName, mbNamespace, err)

satoru-takeuchi · 2024-06-27T07:36:15Z

cmd/backupandrotate/main.go

+
+	var mb mantlev1.MantleBackup
+	if err := cli.Get(ctx, types.NamespacedName{Name: mbName, Namespace: mbNamespace}, &mb); err != nil {
+		return fmt.Errorf("can't get MantleBackup: %s: %s: %w", mbName, mbNamespace, err)


Suggested change

return fmt.Errorf("can't get MantleBackup: %s: %s: %w", mbName, mbNamespace, err)

return fmt.Errorf("couldn't get MantleBackup: %s: %s: %w", mbName, mbNamespace, err)

satoru-takeuchi · 2024-06-28T02:11:51Z

internal/controller/mantlebackupconfig_controller_test.go

+	return nil
+}
+
+// cf. https://zenn.dev/nobishii/articles/type_param_intro_2#%E5%85%AC%E5%BC%8F%E3%83%89%E3%82%AD%E3%83%A5%E3%83%A1%E3%83%B3%E3%83%88%E3%81%AB%E8%A6%8B%E3%82%8B%E5%88%B6%E7%B4%84%E5%9E%8B%E6%8E%A8%E8%AB%96%E3%81%AE%E6%B4%BB%E7%94%A8%E4%BE%8B


Could you replace this article with another one writtein in English, if possible?

satoru-takeuchi · 2024-06-28T02:19:51Z

internal/controller/mantlebackupconfig_controller_test.go

+			Expect(err).NotTo(HaveOccurred())
+		},
+		Entry("various expires 1", "0 0 * * *", "24h"),
+		Entry("various expires 1", "0 0 * * *", "1d"),


Suggested change

Entry("various expires 1", "0 0 * * *", "1d"),

Entry("various expires 1", "0 0 * * *", "1d"),

Suggested change

Entry("various expires 1", "0 0 * * *", "1d"),

Entry("various expires 2", "0 0 * * *", "1d"),

Subsequent 1st argus should also be updated.

test/e2e/backup_test.go

satoru-takeuchi · 2024-06-28T02:32:38Z

test/e2e/backup_test.go

+			return err
+		}).Should(Succeed())
+
+		By("Creating a Job from the CronJob")


It it to emulating the cretion of Job by CronJob and we don't wait for one day in this test? If so, describing it in L296 is better. Or can we introduce another command line parameter for test (like expireOffset)?

It it to emulating the cretion of Job by CronJob and we don't wait for one day in this test? If so, describing it in L296 is better.

Yes, it is. I should have added some comments on it.

Or can we introduce another command line parameter for test (like expireOffset)?

That sounds better. I'll fix the test that way.

Instead of scheduleOffset, I introduced overwrite-mbc-schedule option. When we use this option with some value (e.g., * * * * *), every .spec.schedule of a CronJob created for a MBC is replaced with the value. We can set this to * * * * * for the e2e test. Then, the backup-and-rotate subcommand will be triggered every minute, and we don't need to create a new Job from the CronJob explicitly.

satoru-takeuchi · 2024-06-28T02:37:13Z

test/e2e/testdata/values-mantle.yaml

@@ -0,0 +1,2 @@
+controller:
+  expireOffset: 100d


Can we emulate the actual behavior by changing expireOffset to several seconds or decent secods like '2w-10s'?

I'm worried that we can't predict how quickly the MantleBackup resource will be deleted after it's created. For instance, if we set expireOffset to 1w6d23h59m50s (i.e., 2w-10s), the deletion needs to happen at least 10 seconds after the creation is complete. If it happens any sooner, the test will fail. This makes the test flakier.

Maybe we can add a short sleep in the test to ensure that the 10 seconds have passed. This might be a better approach. I'll give it a try and see how it works.

Indeed, please keep this code as is.

Well, after my fix for your previous feedback, backup-and-rotate subcommand will be triggered every 60 seconds, so we can safely use 1w6d23h59m50s now.

…BackupConfig` and `make` Signed-off-by: Ryotaro Banno <[email protected]>

Signed-off-by: Ryotaro Banno <[email protected]>

satoru-takeuchi

@ushitora-anqou LGTM. Please squash your commits.

Signed-off-by: Ryotaro Banno <[email protected]>

ushitora-anqou changed the title ~~change group from "backup" to "mantle"~~ Support MantleBackupConfig Jun 12, 2024

ushitora-anqou force-pushed the support-mantlebackupconfig branch 6 times, most recently from 2e9f2ae to a0bb4ef Compare June 17, 2024 00:41

ushitora-anqou force-pushed the support-mantlebackupconfig branch 8 times, most recently from e4d2b84 to 2a6172a Compare June 26, 2024 04:29

ushitora-anqou marked this pull request as ready for review June 26, 2024 04:41

ushitora-anqou requested a review from satoru-takeuchi June 26, 2024 04:41

satoru-takeuchi requested changes Jun 27, 2024

View reviewed changes

ushitora-anqou force-pushed the support-mantlebackupconfig branch 2 times, most recently from 45e9704 to aa8e4fa Compare June 27, 2024 04:24

ushitora-anqou requested a review from satoru-takeuchi June 27, 2024 04:42

satoru-takeuchi requested changes Jun 28, 2024

View reviewed changes

ushitora-anqou force-pushed the support-mantlebackupconfig branch 5 times, most recently from 76b3e65 to 71cfd55 Compare July 4, 2024 08:35

ushitora-anqou force-pushed the support-mantlebackupconfig branch 2 times, most recently from c896312 to ceb7a2d Compare July 8, 2024 04:58

ushitora-anqou added 2 commits July 8, 2024 04:58

Run `kubebuilder create api --group mantle --version v1 --kind Mantle…

dfef094

…BackupConfig` and `make` Signed-off-by: Ryotaro Banno <[email protected]>

Makefile: copy MantleBackupConfig CRD from config/ to charts/

1309f62

Signed-off-by: Ryotaro Banno <[email protected]>

ushitora-anqou force-pushed the support-mantlebackupconfig branch 3 times, most recently from 925a975 to 8548600 Compare July 8, 2024 05:29

satoru-takeuchi approved these changes Jul 9, 2024

View reviewed changes

implement reconciler for MantleBackupConfig

Loading
Loading status checks…

739c07e

Signed-off-by: Ryotaro Banno <[email protected]>

ushitora-anqou force-pushed the support-mantlebackupconfig branch from 1298574 to 739c07e Compare July 9, 2024 03:03

ushitora-anqou merged commit 4d07d80 into main Jul 9, 2024
2 checks passed

ushitora-anqou deleted the support-mantlebackupconfig branch July 9, 2024 04:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MantleBackupConfig #11

Support MantleBackupConfig #11

ushitora-anqou commented Jun 12, 2024 •

edited

Loading

satoru-takeuchi commented Jun 26, 2024

ushitora-anqou commented Jun 26, 2024

satoru-takeuchi Jun 26, 2024

satoru-takeuchi Jun 26, 2024

satoru-takeuchi Jun 26, 2024

satoru-takeuchi Jun 26, 2024

satoru-takeuchi Jun 26, 2024

satoru-takeuchi Jun 26, 2024

satoru-takeuchi Jun 27, 2024

ushitora-anqou Jun 27, 2024

satoru-takeuchi Jun 27, 2024

satoru-takeuchi commented Jun 27, 2024

satoru-takeuchi Jun 27, 2024

satoru-takeuchi Jun 27, 2024

satoru-takeuchi Jun 27, 2024

satoru-takeuchi Jun 27, 2024

satoru-takeuchi Jun 27, 2024

satoru-takeuchi Jun 28, 2024

satoru-takeuchi Jun 28, 2024

satoru-takeuchi Jun 28, 2024

ushitora-anqou Jun 28, 2024

ushitora-anqou Jun 28, 2024 •

edited

Loading

satoru-takeuchi Jun 28, 2024

ushitora-anqou Jun 28, 2024 •

edited

Loading

satoru-takeuchi Jun 28, 2024

ushitora-anqou Jun 28, 2024 •

edited

Loading

satoru-takeuchi left a comment

	return "", fmt.Errorf("the length of JOB_NAME is smaller than 8")
	return "", fmt.Errorf("the length of JOB_NAME must be >= 8")

	if err := backup(ctx, cli, &mbc); err != nil {
	if err := createMantleBackup(ctx, cli, &mbc); err != nil {

	logger.Info("avoid creating a new MantleBackup because it already exists",
	logger.Info("MantleBackup already exists",

	// List all MantleBackup objects associated with mbc.
	// List all MantleBackup objects associated with the mbc.

	runningPod, err := getRunningPod(ctx, r.Client)
	cronJobInfo, err := getCronJobInfo(ctx, r.Client)

	logger.Info("mbc not managed", "name", req.Name, "namespace", req.Namespace, "error", err)
	logger.Info("the target pvc is not managed by this controller", "name", req.Name, "namespace", req.Namespace, "error", err)

	return fmt.Errorf("can't parse the expire offset: %w", err)
	return fmt.Errorf("couldn't parse the expire offset: %w", err)

	return fmt.Errorf("can't create a new client: %w", err)
	return fmt.Errorf("couldn't create a new client: %w", err)

	return fmt.Errorf("can't get mbc: %s: %s: %w", mbcName, mbcNamespace, err)
	return fmt.Errorf("couldn't get mbc: %s: %s: %w", mbcName, mbcNamespace, err)

	return fmt.Errorf("can't create a MantleBackup: %s: %s: %w", mbName, mbNamespace, err)
	return fmt.Errorf("couldn't create a MantleBackup: %s: %s: %w", mbName, mbNamespace, err)

	return fmt.Errorf("can't get MantleBackup: %s: %s: %w", mbName, mbNamespace, err)
	return fmt.Errorf("couldn't get MantleBackup: %s: %s: %w", mbName, mbNamespace, err)

	Entry("various expires 1", "0 0 * * *", "1d"),
	Entry("various expires 1", "0 0 * * *", "1d"),

	Entry("various expires 1", "0 0 * * *", "1d"),
	Entry("various expires 2", "0 0 * * *", "1d"),

Support MantleBackupConfig #11

Support MantleBackupConfig #11

Conversation

ushitora-anqou commented Jun 12, 2024 • edited Loading

satoru-takeuchi commented Jun 26, 2024

ushitora-anqou commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satoru-takeuchi commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ushitora-anqou Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

satoru-takeuchi left a comment

Choose a reason for hiding this comment

ushitora-anqou commented Jun 12, 2024 •

edited

Loading

ushitora-anqou Jun 28, 2024 •

edited

Loading

ushitora-anqou Jun 28, 2024 •

edited

Loading

ushitora-anqou Jun 28, 2024 •

edited

Loading