Skip to content
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.

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

Add Conditions to the Status information #204

Merged
merged 2 commits into from
Dec 22, 2020
Merged

Conversation

cimnine
Copy link
Contributor

@cimnine cimnine commented Dec 14, 2020

The goal of this PR is to provide more information about various error and success-conditions to the average K8up user. Such users are mainly interacting with K8up through the relevant custom resources. Therefore, when querying the status of these resources (e.g. kubectl describe backups/X), Conditions shall provide additional information about the resource's individual status.

Closes #138

@cimnine cimnine marked this pull request as draft December 14, 2020 10:27
@cimnine cimnine force-pushed the ExtendStatusWithConditions branch 2 times, most recently from fffa912 to 3ac6b29 Compare December 15, 2020 10:51
Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

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

Overall looking good.

I commented some minor things here and there.

I miss a condition for when the job finished. This might not be that easy to achieve though as this would require to what jobs and have a reconciliation loop for when they change. Maybe create a feature request for this.

executor/archive.go Outdated Show resolved Hide resolved
executor/backup.go Show resolved Hide resolved
executor/restore.go Outdated Show resolved Hide resolved
executor/restore.go Outdated Show resolved Hide resolved
executor/worker.go Show resolved Hide resolved
@cimnine
Copy link
Contributor Author

cimnine commented Dec 15, 2020

I miss a condition for when the job finished. This might not be that easy to achieve though as this would require to what jobs and have a reconciliation loop for when they change. Maybe create a feature request for this.

I'll quickly look into it on Thursday, but it'll probably lead to a feature request.

@cimnine cimnine marked this pull request as ready for review December 15, 2020 19:36
@ccremer
Copy link
Contributor

ccremer commented Dec 17, 2020

I miss a condition for when the job finished. This might not be that easy to achieve though as this would require to what jobs and have a reconciliation loop for when they change. Maybe create a feature request for this.

Not sure if I understand it correctly. We already have a reconciler for batch.jobs: https://github.com/vshn/k8up/blob/master/controllers/job_controller.go#L50, so we are watching those.

executor/prune.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
prebackup/prebackup.go Outdated Show resolved Hide resolved
prebackup/prebackup.go Show resolved Hide resolved
prebackup/prebackup.go Outdated Show resolved Hide resolved
@corvus-ch
Copy link
Contributor

We already have a reconciler for batch.jobs: https://github.com/vshn/k8up/blob/master/controllers/job_controller.go#L50, so we are watching those.

I was not aware of that one. So then this should be simple, I guess.

@cimnine
Copy link
Contributor Author

cimnine commented Dec 17, 2020

We already have a reconciler for batch.jobs: https://github.com/vshn/k8up/blob/master/controllers/job_controller.go#L50, so we are watching those.

I was not aware of that one. So then this should be simple, I guess.

I don't see a straight-forward way to get back to the original Backup object, though. This is what I've come up with so far:

// Handle extracts some information from the batchv1.job that make observations
// easier.
func (j *JobHandler) Handle() error {

	jobEvent := observer.Create

	if _, exists := j.job.GetLabels()[job.K8uplabel]; !exists {
		return nil
	}

	if j.job.Status.CompletionTime != nil {
		references := j.job.GetObjectMeta().GetOwnerReferences()
		if len(references) < 1 {
			return nil
		}

		reference := references[0]

		ownerBackupObj := &v1alpha1.Backup{}
		err := j.Client.Get(j.CTX, client.ObjectKey{Name: reference.Name, Namespace: j.job.Namespace}, ownerBackupObj)
		if err != nil {
			return err
		}
	}
  • Is that how you'd solve it?
  • Do I understand correctly that the OwnerReference could either be a Backup, Restore, Check, Prune, Snapshot, etc.? So I would have to account for all possible types that start jobs? (That seems error-prone to me.)

job/job.go Show resolved Hide resolved
prebackup/prebackup.go Outdated Show resolved Hide resolved
prebackup/prebackup.go Outdated Show resolved Hide resolved
@cimnine
Copy link
Contributor Author

cimnine commented Dec 18, 2020

Rebased onto master and stashed the commits.

Copy link
Contributor

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

I played around with it locally, though I haven't set up local backup repository so they always failed. at least the "JobCreated" condition is correctly being applied to the prune job that I set up.

Looking good so far! Thanks for the works!

Copy link
Contributor

@tobru tobru left a comment

Choose a reason for hiding this comment

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

Documentation update LGTM

@tobru tobru added this to the v1.0.0-rc2 milestone Dec 22, 2020
@cimnine cimnine merged commit 92ceeb5 into master Dec 22, 2020
@cimnine cimnine deleted the ExtendStatusWithConditions branch December 22, 2020 13:00
@ccremer ccremer added the enhancement New feature or request label Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend K8up Status resources with Conditions
4 participants