Skip to content

Implement offline backup#229

Merged
tamalsaha merged 26 commits intomasterfrom
offline-backup
Dec 4, 2017
Merged

Implement offline backup#229
tamalsaha merged 26 commits intomasterfrom
offline-backup

Conversation

@diptadas
Copy link
Contributor

@diptadas diptadas commented Nov 16, 2017

Fixes #225 #216

Changes:

  • Rename package schedule to backup and add offline flag
  • Use init container for offline backup instead of sidecar
  • Add check command
  • Create check job from init container
  • Create cronjob from restic watcher to delete workload pods, set restic as cronjob's owner ref, set label app=stash, set annotation restic-name, stash-operation
  • Update cronjob accordingly when restic updated
  • Watch job with label app=stash, delete completed jobs/pods for backup/check/recovery

Issues:

  • Event writing
  • Wait for workload ready in kutil

main.go Outdated
defer logs.FlushLogs()

if err := cmds.NewCmdStash(Version).Execute(); err != nil {
log.Infoln("Error in Stash Main:", err)
Copy link
Member

Choose a reason for hiding this comment

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

Use log.Fatalln() instead of 2 commands

type Controller struct {
k8sClient kubernetes.Interface
stashClient cs.StashV1alpha1Interface
namespace string
Copy link
Member

Choose a reason for hiding this comment

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

Create an Options struct for the string variables

return
}

defer func() {
Copy link
Member

Choose a reason for hiding this comment

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

Where is the delay?

ctrl.ElectLeader(stopBackup)
default:
ctrl.SetupAndRun(stopBackup)
if opt.RunOffline {
Copy link
Member

Choose a reason for hiding this comment

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

Use an option called runOnce

func (c *StashController) initJobWatcher() {
lw := &cache.ListWatch{ // TODO @ Dipta: only watch stash jobs
ListFunc: func(options metav1.ListOptions) (rt.Object, error) {
return c.k8sClient.BatchV1().Jobs(core.NamespaceAll).List(options)
Copy link
Member

Choose a reason for hiding this comment

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

job := obj.(*batch.Job)
fmt.Printf("Sync/Add/Update for Job %s\n", job.GetName())

if job.Labels["app"] == util.AppLabelStash && job.Status.Succeeded > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

These should be annotations

d := obj.(*api.Restic)
fmt.Printf("Sync/Add/Update for Restic %s\n", d.GetName())

if d.Spec.Type == api.BackupOffline {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be done at the start of operator and we should check that the image exists and exit if the image is missing. To check image existence use,
https://github.com/k8sdb/apimachinery/blob/3f92c150ed3bedd0d2d19bcc3856360937d06033/pkg/docker/checks.go#L13

if c.Name == StashContainer {
found = true
break
if offline {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of offline bool, I think we should pass the backupType

return container
}

func CreateSidecarContainer(r *api.Restic, tag string, workload api.LocalTypedReference) core.Container {
Copy link
Member

Choose a reason for hiding this comment

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

Where is serviceAccountName created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

InitContainer and Sidecar use the same service account as the workload?


err = c.resticCLI.Check()
if err != nil {
c.recorder.Eventf(resource.ObjectReference(), core.EventTypeWarning, eventer.EventReasonFailedToCheck, "Repository check failed for workload %s %s/%s. Reason: %v", c.opt.Workload.Kind, c.opt.Namespace, c.opt.Workload.Name, err)
Copy link
Member

Choose a reason for hiding this comment

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

When the line is this long, move each parameter to a separate line. Makes it more readable.

@tamalsaha tamalsaha merged commit 7241c6d into master Dec 4, 2017
@tamalsaha tamalsaha deleted the offline-backup branch December 4, 2017 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants