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.

Sign up for GitHub

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

Jump to bottom

[OADP-460] Don't mount cloud provider secret volumes with NoDefaultBackupLocation #662

Merged
merged 8 commits into from
May 11, 2022
Merged

[OADP-460] Don't mount cloud provider secret volumes with NoDefaultBackupLocation #662

merged 8 commits into from
May 11, 2022

Conversation

kaovilai
Copy link
Member

@kaovilai kaovilai commented May 2, 2022 •
edited
Loading

  • Add/fix e2e test

Fixes OADP-460

This is a follow up to #420, #607, #349, #380, #373, and #12

@openshift-ci openshift-ci bot requested review from alaypatel07 and jmontleon May 2, 2022 14:49
@kaovilai kaovilai changed the title Initial commit of fix for OADP-460. Don't mount cloud provider secret volumes with noDefaultLocation May 2, 2022
@kaovilai
Copy link
Member Author

kaovilai commented May 2, 2022 •
edited
Loading

azure related (partiallyFailed backups) errors can be ignored until #654 is merged.

@kaovilai
Copy link
Member Author

kaovilai commented May 2, 2022

/retest

@kaovilai
Copy link
Member Author

kaovilai commented May 2, 2022

/hold we found some issue with restic pods

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2022
@kaovilai kaovilai changed the title Don't mount cloud provider secret volumes with noDefaultLocation Don't mount cloud provider secret volumes with NoDefaultBackupLocation May 3, 2022
@kaovilai
Copy link
Member Author

kaovilai commented May 4, 2022

/retest

@kaovilai kaovilai changed the title Don't mount cloud provider secret volumes with NoDefaultBackupLocation [OADP-460] Don't mount cloud provider secret volumes with NoDefaultBackupLocation May 6, 2022
dymurray
dymurray reviewed May 11, 2022
View reviewed changes
controllers/validator.go
if len(dpa.Spec.BackupLocations) == 0 && !dpa.Spec.Configuration.Velero.NoDefaultBackupLocation {
return false, errors.New("no backupstoragelocations configured, ensure a backupstoragelocation has been configured or use the noDefaultLocationBackupLocation flag")
if dpa.Spec.Configuration.Velero.NoDefaultBackupLocation {
if len(dpa.Spec.BackupLocations) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is true, we really are just saying if we have any BSLs defined in the DPA spec that none of them have default: true right?

Copy link
Member

Choose a reason for hiding this comment

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

after digging, velero install takes the same approach https://github.com/vmware-tanzu/velero/blob/77be862b9c8d4d0b9187a8239059e6ed9b54cd1b/pkg/cmd/cli/install/install.go#L307-L309

so its fine

Copy link
Member

Choose a reason for hiding this comment

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

We only use install lib for the deployment, so we really can choose our own behavior here. Awaiting merge for more input

Copy link
Member

Choose a reason for hiding this comment

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

@dymurray @kaovilai I think we should be consistent with how upstream Velero deals with this, as the users will expect similar behavior. So shouldn't we be passing this install flag value to Velero deployment ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@shubham-pampattiwar this flag is only used in func AllResources which we skipped entirely from using.

Copy link
Member Author

Choose a reason for hiding this comment

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

We consumed individually install.Deployments and install.Daemonsets. We may refactor to use AllResources in the future after required restic pod changes are included upstream.

dymurray
dymurray previously approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@dymurray dymurray left a comment

Choose a reason for hiding this comment

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

VISIACK, changes look sane

@dymurray dymurray dismissed their stale review May 11, 2022 02:27

awaiting more feedback

dymurray
dymurray approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@dymurray dymurray left a comment

Choose a reason for hiding this comment

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

VISIACK

@dymurray
Copy link
Member

/retest

2 similar comments
@kaovilai
Copy link
Member Author

/retest

@kaovilai
Copy link
Member Author

/retest

kaovilai added a commit to kaovilai/oadp-operator that referenced this pull request May 11, 2022
@kaovilai
Squashed commit of the following from openshift#662:
c5f79f1 
commit 91fbcd6
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Wed May 11 10:34:18 2022 -0400

    added TestDPAReconciler_noDefaultCredentials

commit e175757
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 23:58:59 2022 -0400

    go fmt

commit 7b49d25
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 23:58:48 2022 -0400

    restic only mount secretVolume if NoDefaultBackupLocation is found.

commit c70f85d
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:51:49 2022 -0400

    spec indent update

commit d14b22e
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:40:42 2022 -0400

    add usage doc

commit 2b26e48
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:37:45 2022 -0400

    if vsl specified we still need default creds

commit 30ac88d
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Mon May 2 10:50:10 2022 -0400

    error message change

commit 3ed9194
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Mon May 2 10:47:23 2022 -0400

    Initial commit of fix for OADP-460.
kaovilai added a commit to kaovilai/oadp-operator that referenced this pull request May 11, 2022
@kaovilai
Squashed commit of the following from openshift#662:
5cbc3d1 
commit 91fbcd6
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Wed May 11 10:34:18 2022 -0400

    added TestDPAReconciler_noDefaultCredentials

commit e175757
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 23:58:59 2022 -0400

    go fmt

commit 7b49d25
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 23:58:48 2022 -0400

    restic only mount secretVolume if NoDefaultBackupLocation is found.

commit c70f85d
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:51:49 2022 -0400

    spec indent update

commit d14b22e
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:40:42 2022 -0400

    add usage doc

commit 2b26e48
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:37:45 2022 -0400

    if vsl specified we still need default creds

commit 30ac88d
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Mon May 2 10:50:10 2022 -0400

    error message change

commit 3ed9194
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Mon May 2 10:47:23 2022 -0400

    Initial commit of fix for OADP-460.
@kaovilai kaovilai mentioned this pull request May 11, 2022
Merged
@openshift-ci
Copy link

openshift-ci bot commented May 11, 2022

@kaovilai: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@kaovilai kaovilai merged commit 330b156 into openshift:master May 11, 2022
dymurray pushed a commit that referenced this pull request May 11, 2022
@kaovilai
Squashed commit of the following from #662: (#689)
5e7e158 
commit 91fbcd6
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Wed May 11 10:34:18 2022 -0400

    added TestDPAReconciler_noDefaultCredentials

commit e175757
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 23:58:59 2022 -0400

    go fmt

commit 7b49d25
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 23:58:48 2022 -0400

    restic only mount secretVolume if NoDefaultBackupLocation is found.

commit c70f85d
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:51:49 2022 -0400

    spec indent update

commit d14b22e
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:40:42 2022 -0400

    add usage doc

commit 2b26e48
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Tue May 3 10:37:45 2022 -0400

    if vsl specified we still need default creds

commit 30ac88d
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Mon May 2 10:50:10 2022 -0400

    error message change

commit 3ed9194
Author: Tiger Kaovilai <tkaovila@redhat.com>
Date:   Mon May 2 10:47:23 2022 -0400

    Initial commit of fix for OADP-460.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shubham-pampattiwar shubham-pampattiwar shubham-pampattiwar approved these changes

@dymurray dymurray dymurray approved these changes

@alaypatel07 alaypatel07 Awaiting requested review from alaypatel07

@jmontleon jmontleon Awaiting requested review from jmontleon

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaovilai @dymurray @shubham-pampattiwar