OADP-5158: Do not allow restic config use in OADP 1.5+; Deprecate restic for file system backups #1235

mateusoliveira43 · 2023-11-22T13:11:07Z

Description

This PR removes logic of using old restic field (it should use nodeAgent field instead) from 1.5+ code. But it does not remove old restic field from DPA (which should only be removed when DPA version is updated), for a safe upgrade and better user experience for the users.

Also, log a Deprecation Warning when using restic in NodeAgent config.

How to test

OADP 1.5

run make deploy-olm and test that

DPA is Reconciled false if old restic field (spec.configuration.restic) is present
OADP manager pod has 1 warning log and OADP namespace has 1 warning event if DPA spec.configuration.nodeAgent.uploaderType is set to restic

Simulate a upgrade

Simulate a upgrade from OADP 1.4 to 1.5 (this PR branch), by

running make catalog-test-upgrade
installing OADP 1.4
creating a DPA with old restic field (wait until it is reconciled true)
and update to OADP 1.5

More info

Update should finish successfully and DPA should:

be reconciled false, with the error message that restic field is not allowed
restic fields should be still in DPA

Makefile

mateusoliveira43 · 2024-09-26T19:06:40Z

controllers/nodeagent.go

@@ -62,10 +60,6 @@ func getFsPvHostPath(platformType string) string {
 		return envFs
 	}

-	if env := os.Getenv(ResticPVHostPathEnvVar); env != "" {


still not sure if I should delete this

controllers/validator.go

shubham-pampattiwar · 2024-09-30T22:21:40Z

api/v1alpha1/oadp_types.go

-	// restic is for backwards compatibility and is replaced by the nodeAgent
-	// restic will be removed in the future
-	// +kubebuilder:deprecatedversion:warning=1.3
+	// (do not use warning) restic field is for backwards compatibility and


What does (do not use warning) mean here ?

I think the intend here should be that "If a user uses restic config then the operator should do a No-Op and should add a warning on DPA that use node-agent config instead of restic"

So, the current idea is that user can only use restic through node-agent config, this is a way to tell user not to use restic config

If user tries to use, DPA want reconcile telling user to use node-agent config instead

config/samples/kustomization.yaml

controllers/validator.go

openshift-ci-robot · 2025-01-02T18:49:44Z

@mateusoliveira43: This pull request references OADP-5158 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.19.0" version, but no target version was set.

In response to this:

Description

This PR removes logic of using old restic field (it should use nodeAgent field instead) from 1.5+ code. But it does not remove old restic field from DPA (which should only be removed when DPA version is updated), for a safe upgrade and better user experience for the users.

Also, log a Deprecation Warning when using restic in NodeAgent config.

How to test

Simulate a upgrade from OADP 1.4 to 1.5 (this PR branch), by

running make catalog-test-upgrade

installing OADP 1.4

creating a DPA with old restic field (wait until it is reconciled true)

and update to OADP 1.5

More info

Update should finish successfully and DPA should:

be reconciled false, with the error message that restic field is not allowed

restic fields should be still in DPA

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-01-02T19:36:15Z

@mateusoliveira43: This pull request references OADP-5158 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.19.0" version, but no target version was set.

In response to this:

Description

This PR removes logic of using old restic field (it should use nodeAgent field instead) from 1.5+ code. But it does not remove old restic field from DPA (which should only be removed when DPA version is updated), for a safe upgrade and better user experience for the users.

Also, log a Deprecation Warning when using restic in NodeAgent config.

How to test

OADP 1.5

run make deploy-olm and test that

DPA is Reconciled false if old restic field (spec.configuration.restic) is present

OADP manager pod has 1 warning log and OADP namespace has 1 warning event if DPA spec.configuration.nodeAgent.uploaderType is set to restic

Simulate a upgrade

Simulate a upgrade from OADP 1.4 to 1.5 (this PR branch), by

running make catalog-test-upgrade

installing OADP 1.4

creating a DPA with old restic field (wait until it is reconciled true)

and update to OADP 1.5

More info

Update should finish successfully and DPA should:

be reconciled false, with the error message that restic field is not allowed

restic fields should be still in DPA

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 openshift-eng/jira-lifecycle-plugin repository.

Signed-off-by: Mateus Oliveira <[email protected]>

openshift-ci · 2025-01-21T16:03:42Z

@mateusoliveira43: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.12-operator-e2e-aws	`1587d2c`	link	true	`/test 4.12-operator-e2e-aws`
ci/prow/4.12-operator-unit-test	`1587d2c`	link	true	`/test 4.12-operator-unit-test`
ci/prow/4.14-operator-e2e-azure	`1587d2c`	link	true	`/test 4.14-operator-e2e-azure`
ci/prow/4.12-operator-e2e-azure	`1587d2c`	link	true	`/test 4.12-operator-e2e-azure`
ci/prow/4.12-e2e-test-azure	`8299442`	link	true	`/test 4.12-e2e-test-azure`
ci/prow/4.15-e2e-test-aws	`cbd019d`	link	true	`/test 4.15-e2e-test-aws`
ci/prow/4.17-e2e-test-aws	`cbd019d`	link	true	`/test 4.17-e2e-test-aws`

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-sigs/prow repository. I understand the commands that are listed here.

shubham-pampattiwar · 2025-01-23T19:14:27Z

api/v1alpha1/dataprotectionapplication_types.go

-	// restic is for backwards compatibility and is replaced by the nodeAgent
-	// restic will be removed in the future
-	// +kubebuilder:deprecatedversion:warning=1.3
+	// (do not use warning) restic field is for backwards compatibility and


s/field/spec at both places

shubham-pampattiwar · 2025-01-23T19:20:13Z

internal/controller/nodeagent.go

@@ -37,7 +35,6 @@ const (
 	IBMCloudPVHostPath      = "/var/data/kubelet/pods"
 	GenericPluginsHostPath  = "/var/lib/kubelet/plugins"
 	IBMCloudPluginsHostPath = "/var/data/kubelet/plugins"
-	ResticPVHostPathEnvVar  = "RESTIC_PV_HOSTPATH"


I think you will also need to remove RESTIC_PV_HOSTPATH from manager.yaml and operator csv

shubham-pampattiwar · 2025-01-23T19:29:17Z

internal/controller/validator.go

 	// check for VSM/Volsync DataMover (OADP 1.2 or below) syntax
 	if r.dpa.Spec.Features != nil && r.dpa.Spec.Features.DataMover != nil {
 		return false, errors.New("Delete vsm from spec.configuration.velero.defaultPlugins and dataMover object from spec.features. Use Velero Built-in Data Mover instead")
 	}

+	// check for ResticConfig (OADP 1.4 or below) syntax
+	if r.dpa.Spec.Configuration.Restic != nil {
+		return false, errors.New("Delete restic object from spec.configuration, use spec.configuration.nodeAgent instead")


s/object/spec

shubham-pampattiwar · 2025-01-23T19:47:57Z

internal/controller/validator.go

@@ -16,6 +17,8 @@ import (
 	"github.com/openshift/oadp-operator/pkg/credentials"
 )

+var wasRestic bool


Should we explicitly set this as false ? (I think default is false but still)

If we do, it would fail https://revive.run/r#var-declaration linter that was recently merged https://github.com/vmware-tanzu/velero/pull/8636/files#diff-9917ddc9f1c3304218f7269265b746d997c5c0615478177b5fceecd33ef47cb5
specifically because we are declaring "zeroLiteral"

var zeroLiteral = map[string]bool{ "false": true, // bool // runes `'\x00'`: true, `'\000'`: true, // strings `""`: true, "``": true, // numerics "0": true, "0.": true, "0.0": true, "0i": true, }

im ok with it either way.

openshift-ci bot requested review from eemcmullan and shubham-pampattiwar November 22, 2023 13:13

kaovilai reviewed Nov 27, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

mateusoliveira43 force-pushed the fix/remove-restic-config-master branch from 1587d2c to 7f9b1e9 Compare November 30, 2023 16:22

This comment was marked as outdated.

Sign in to view

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2023

This comment was marked as outdated.

Sign in to view

mateusoliveira43 changed the title ~~fix: Remove restic config from master branch~~ fix: Do not allow restic config use for master branch (OADP 1.4) Jan 4, 2024

This comment was marked as outdated.

Sign in to view

mateusoliveira43 changed the title ~~fix: Do not allow restic config use for master branch (OADP 1.4)~~ fix: Do not allow restic config use in OADP 1.4+ Jan 16, 2024

mateusoliveira43 mentioned this pull request Jan 20, 2024

Remove unused fields from DPA when API version updates #1276

Open

This comment was marked as outdated.

Sign in to view

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2024

This comment was marked as outdated.

Sign in to view

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2024

This comment was marked as outdated.

Sign in to view

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2024

This comment was marked as outdated.

Sign in to view

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2024

mateusoliveira43 commented Sep 26, 2024

View reviewed changes

controllers/validator.go Outdated Show resolved Hide resolved

mateusoliveira43 commented Sep 30, 2024

View reviewed changes

controllers/validator.go Outdated Show resolved Hide resolved

mateusoliveira43 force-pushed the fix/remove-restic-config-master branch from f88e2d2 to 085c01c Compare September 30, 2024 14:06

mateusoliveira43 marked this pull request as ready for review September 30, 2024 14:06

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2024

openshift-ci bot requested a review from mpryc September 30, 2024 14:06

This comment was marked as outdated.

Sign in to view

shubham-pampattiwar reviewed Sep 30, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 1, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 1, 2025

mateusoliveira43 marked this pull request as draft January 2, 2025 18:19

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 2, 2025

mateusoliveira43 force-pushed the fix/remove-restic-config-master branch from 085c01c to 5ac138b Compare January 2, 2025 18:46

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 2, 2025

mateusoliveira43 changed the title ~~fix: Do not allow restic config use in OADP 1.5+~~ OADP-5158: Do not allow restic config use in OADP 1.5+ Jan 2, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 2, 2025

This comment was marked as resolved.

Sign in to view

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 2, 2025

mateusoliveira43 changed the title ~~OADP-5158: Do not allow restic config use in OADP 1.5+~~ OADP-5158: Do not allow restic config use in OADP 1.5+; Deprecate restic for file system backups Jan 2, 2025

mateusoliveira43 marked this pull request as ready for review January 3, 2025 13:42

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 3, 2025

openshift-ci bot requested a review from weshayutin January 3, 2025 13:42

fix: Remove restic config from master branch

cbd019d

Signed-off-by: Mateus Oliveira <[email protected]>

mateusoliveira43 force-pushed the fix/remove-restic-config-master branch from 6c5473d to cbd019d Compare January 21, 2025 12:27

shubham-pampattiwar reviewed Jan 23, 2025

View reviewed changes

OADP-5158: Do not allow restic config use in OADP 1.5+; Deprecate restic for file system backups #1235

Are you sure you want to change the base?

OADP-5158: Do not allow restic config use in OADP 1.5+; Deprecate restic for file system backups #1235

Conversation

mateusoliveira43 commented Nov 22, 2023 • edited Loading

Description

How to test

OADP 1.5

Simulate a upgrade

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

openshift-ci-robot commented Jan 2, 2025 • edited by openshift-ci bot Loading

Description

How to test

This comment was marked as resolved.

openshift-ci-robot commented Jan 2, 2025 • edited by openshift-ci bot Loading

Description

How to test

OADP 1.5

Simulate a upgrade

openshift-ci bot commented Jan 21, 2025

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

mateusoliveira43 commented Nov 22, 2023 •

edited

Loading

openshift-ci-robot commented Jan 2, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 2, 2025 •

edited by openshift-ci bot

Loading