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 a BSL controller to handle validation + update BSL status phase #2674

Merged
merged 18 commits into from Jul 14, 2020

Conversation

carlisia
Copy link
Contributor

@carlisia carlisia commented Jun 29, 2020

Closes: #1967.

Specs are on the ticket.

Note: I will add validation to BSL create/update in a separate PR.

Signed-off-by: Carlisia carlisia@vmware.com

@carlisia carlisia force-pushed the c-bsl-controller-II branch 5 times, most recently from 51eeb48 to 7fbbdac Compare June 30, 2020 23:39
@carlisia carlisia self-assigned this Jul 1, 2020
Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

This is looking good just added some comments.

config/rbac/role.yaml Outdated Show resolved Hide resolved
internal/velero/storagelocation.go Outdated Show resolved Hide resolved
internal/velero/storagelocation.go Outdated Show resolved Hide resolved
internal/velero/storagelocation.go Show resolved Hide resolved
internal/velero/storagelocation.go Show resolved Hide resolved
pkg/controller/backupstoragelocation_controller.go Outdated Show resolved Hide resolved
pkg/controller/backupstoragelocation_controller.go Outdated Show resolved Hide resolved
pkg/controller/backupstoragelocation_controller.go Outdated Show resolved Hide resolved
@carlisia carlisia mentioned this pull request Jul 6, 2020
@carlisia carlisia force-pushed the c-bsl-controller-II branch 2 times, most recently from e73449b to a24411d Compare July 7, 2020 00:59
Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

Did a quick pass and the code LGTM.
Going to take another look tomorrow with a fresher pair of 👀

Were you able to run this with multiple BSLs and also verify that the BSLs are correctly reconciled?

@ashish-amarnath ashish-amarnath self-requested a review July 10, 2020 02:08
@carlisia
Copy link
Contributor Author

Were you able to run this with multiple BSLs and also verify that the BSLs are correctly reconciled?

Yes. Scenarios (and shifting from one to the other):

  • 0 BSLs
  • storage down
  • multiple, all valid
  • multiple, all invalid
  • only 1, invalid
  • only 1, valid
  • multiple, none of which were the specified default

Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

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

Overall, I think this is pretty much ready! I have some questions on it, but I think that's all at this point.

internal/velero/storagelocation.go Show resolved Hide resolved
pkg/cmd/server/server.go Outdated Show resolved Hide resolved
pkg/cmd/cli/backuplocation/create.go Outdated Show resolved Hide resolved
site/docs/master/api-types/backupstoragelocation.md Outdated Show resolved Hide resolved
@nrb nrb added this to the v1.5 milestone Jul 14, 2020
Carlisia and others added 11 commits July 14, 2020 10:12
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Ashish Amarnath <ashisham@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Carlisia added 6 commits July 14, 2020 10:12
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @carlisia.
Will have faster turnaround on the next review.
I have another round of comments.

pkg/cmd/server/server.go Show resolved Hide resolved
log.WithError(err).Error("No backup storage locations found, at least one is required")
}

if r.StorageLocation.DefaultStoreValidationFrequency <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't DefaultStoreValidationFrequency == 0 a way to disable this periodic validation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, this should probably just look for < 0

Copy link
Contributor Author

@carlisia carlisia Jul 14, 2020

Choose a reason for hiding this comment

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

Good catch. And no validation should be in the controller anyway, removing.

In the business logic there's already:

	if validationFrequency < 0 {
		log.Debugf("Validation period must be non-negative, changing from %d to %d", validationFrequency, p.DefaultStoreValidationFrequency)
		validationFrequency = p.DefaultStoreValidationFrequency
	}
	

site/docs/master/api-types/backupstoragelocation.md Outdated Show resolved Hide resolved
Signed-off-by: Carlisia <carlisia@vmware.com>
Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

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

We're close! I think the question about what we do when there are 0 BSLs, whether on startup or during runtime, is an important one. I've got some comments and questions inline that I'd like to understand before deciding how we proceed there.

I think I'd like to lean towards not halting the server when there's no BSLs, in order to help folks who are trying to run Velero as part of a larger system.

Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

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.

Add a BSL controller to handle validation + update BSL status phase
3 participants