Support config file for server configuration #208

swapnilgm · 2019-12-06T09:00:37Z

Signed-off-by: Swapnil Mhamane [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #201

Special notes for your reviewer:

Release note:

Configuring backup-restore server using config file is now supported.

Signed-off-by: Swapnil Mhamane <[email protected]>

shreyas-s-rao

@swapnilgm Thanks for the well-written PR. I have a few suggestions/changes. Please address them.

In all init.go files, bring the default values out of the New___Config method into types.go, as constants.
Why is config-file not enabled for the other subcommands (restore, snapshot, initialize)? Given that server command config already has most of the flags for the other subcommands, it would be easier to use config-file option for those subcommands as well. WDYT? Just a suggestion.

shreyas-s-rao · 2020-02-18T09:45:02Z

pkg/etcdutil/init.go

+// NewEtcdConnectionConfig returns etcd connection config.
+func NewEtcdConnectionConfig() *EtcdConnectionConfig {
+	return &EtcdConnectionConfig{
+		Endpoints:          []string{"127.0.0.1:2379"},


Define constants and then return them. That way it's easier to modify such default values in the future.

shreyas-s-rao · 2020-02-18T09:46:51Z

pkg/etcdutil/init.go

+func NewEtcdConnectionConfig() *EtcdConnectionConfig {
+	return &EtcdConnectionConfig{
+		Endpoints:          []string{"127.0.0.1:2379"},
+		ConnectionTimeout:  wrappers.Duration{Duration: 30 * time.Second},


See above comment

shreyas-s-rao · 2020-02-24T06:24:54Z

cmd/initializer.go

 			}

-			var snapstoreConfig *snapstore.Config
-			if storageProvider != "" {


Looks like this check is missed in the new code. How will etcd-events be handled in that case?

etcd-events/empty storage provider case are handled in flow. We don't have to have any additional check here. The initialise rather restore api define inside have to deal with empty store. So, the check is defined internally.

shreyas-s-rao · 2020-02-26T12:57:57Z

pkg/snapshot/snapshotter/init.go

+	return &Config{
+		FullSnapshotSchedule:     "0 */1 * * *",
+		DeltaSnapshotPeriod:      wrappers.Duration{Duration: 20 * time.Second},
+		DeltaSnapshotMemoryLimit: 10 * 1024 * 1024, //10Mib


Suggested change

DeltaSnapshotMemoryLimit: 10 * 1024 * 1024, //10Mib

DeltaSnapshotMemoryLimit: DefaultDeltaSnapMemoryLimit,

shreyas-s-rao · 2020-02-26T12:58:24Z

pkg/snapshot/snapshotter/init.go

+func NewSnapshotterConfig() *Config {
+	return &Config{
+		FullSnapshotSchedule:     "0 */1 * * *",
+		DeltaSnapshotPeriod:      wrappers.Duration{Duration: 20 * time.Second},


Suggested change

DeltaSnapshotPeriod: wrappers.Duration{Duration: 20 * time.Second},

DeltaSnapshotPeriod: wrappers.Duration{Duration: DefaultDeltaSnapshotInterval},

shreyas-s-rao · 2020-02-26T12:59:05Z

pkg/snapshot/snapshotter/init.go

+		DeltaSnapshotMemoryLimit: 10 * 1024 * 1024, //10Mib
+		GarbageCollectionPeriod:  wrappers.Duration{Duration: time.Minute},
+		GarbageCollectionPolicy:  GarbageCollectionPolicyExponential,
+		MaxBackups:               7,


Suggested change

MaxBackups: 7,

MaxBackups: DefaultMaxBackups,

shreyas-s-rao · 2020-02-27T09:54:05Z

example/00-backup-restore-server-config.yaml

+  port: 8080
+  # enableProfiling: true
+  # server-cert: "ssl/etcdbr/tls.crt"
+  # server-key: "ssl/etcdbr/tls.crt"


Suggested change

# server-key: "ssl/etcdbr/tls.crt"

# server-key: "ssl/etcdbr/tls.key"

swapnilgm

Thanks for the review. I have done the the changes.

Regarding config for sub-commands, I think I had some good reason initially to not add config file for sub-commands. it was something around, we rarely use this sub commands, And sub-commands are mostly used for one time manual operation so it won't make much sense. But anyway we could add it later. I or one of us can add it later as a part of separate PR. WDYS?

swapnilgm · 2020-03-16T10:13:53Z

cmd/initializer.go

 			}

-			var snapstoreConfig *snapstore.Config
-			if storageProvider != "" {


etcd-events/empty storage provider case are handled in flow. We don't have to have any additional check here. The initialise rather restore api define inside have to deal with empty store. So, the check is defined internally.

swapnilgm · 2020-03-18T10:55:49Z

example/00-backup-restore-server-config.yaml

+  port: 8080
+  # enableProfiling: true
+  # server-cert: "ssl/etcdbr/tls.crt"
+  # server-key: "ssl/etcdbr/tls.crt"


swapnilgm · 2020-03-18T10:58:15Z

pkg/etcdutil/init.go

+// NewEtcdConnectionConfig returns etcd connection config.
+func NewEtcdConnectionConfig() *EtcdConnectionConfig {
+	return &EtcdConnectionConfig{
+		Endpoints:          []string{"127.0.0.1:2379"},


swapnilgm · 2020-03-18T11:06:02Z

pkg/snapshot/snapshotter/init.go

+func NewSnapshotterConfig() *Config {
+	return &Config{
+		FullSnapshotSchedule:     "0 */1 * * *",
+		DeltaSnapshotPeriod:      wrappers.Duration{Duration: 20 * time.Second},


swapnilgm · 2020-03-18T11:06:09Z

pkg/snapshot/snapshotter/init.go

+	return &Config{
+		FullSnapshotSchedule:     "0 */1 * * *",
+		DeltaSnapshotPeriod:      wrappers.Duration{Duration: 20 * time.Second},
+		DeltaSnapshotMemoryLimit: 10 * 1024 * 1024, //10Mib


swapnilgm · 2020-03-18T11:06:15Z

pkg/snapshot/snapshotter/init.go

+		DeltaSnapshotMemoryLimit: 10 * 1024 * 1024, //10Mib
+		GarbageCollectionPeriod:  wrappers.Duration{Duration: time.Minute},
+		GarbageCollectionPolicy:  GarbageCollectionPolicyExponential,
+		MaxBackups:               7,


pkg/snapshot/restorer/init.go

Signed-off-by: Swapnil Mhamane <[email protected]>

swapnilgm requested review from georgekuruvillak and shreyas-s-rao as code owners December 6, 2019 09:00

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 6, 2019

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 6, 2019

swapnilgm force-pushed the feature/support-config-file branch from 022b1af to 382e58a Compare December 9, 2019 09:07

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 9, 2019

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 9, 2019

Support config file for configration

16cecba

Signed-off-by: Swapnil Mhamane <[email protected]>

swapnilgm force-pushed the feature/support-config-file branch from 382e58a to 16cecba Compare December 10, 2019 06:29

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 10, 2019

shreyas-s-rao requested changes Feb 27, 2020

View reviewed changes

swapnilgm commented Mar 18, 2020

View reviewed changes

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 18, 2020

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 18, 2020

shreyas-s-rao reviewed Mar 23, 2020

View reviewed changes

pkg/snapshot/restorer/init.go Outdated Show resolved Hide resolved

👌 Addressed the review comments

8780687

Signed-off-by: Swapnil Mhamane <[email protected]>

swapnilgm force-pushed the feature/support-config-file branch from 1026b83 to 8780687 Compare March 24, 2020 05:10

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 24, 2020

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 24, 2020

swapnilgm merged commit c6a9cf5 into gardener:master Mar 24, 2020

swapnilgm deleted the feature/support-config-file branch March 24, 2020 05:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support config file for server configuration #208

Support config file for server configuration #208

swapnilgm commented Dec 6, 2019 •

edited

Loading

shreyas-s-rao left a comment

shreyas-s-rao Feb 18, 2020

swapnilgm Mar 18, 2020

shreyas-s-rao Feb 18, 2020

shreyas-s-rao Feb 24, 2020

swapnilgm Mar 16, 2020

shreyas-s-rao Feb 26, 2020 •

edited

Loading

swapnilgm Mar 18, 2020

shreyas-s-rao Feb 26, 2020 •

edited

Loading

swapnilgm Mar 18, 2020

shreyas-s-rao Feb 26, 2020 •

edited

Loading

swapnilgm Mar 18, 2020

shreyas-s-rao Feb 27, 2020

swapnilgm Mar 18, 2020

swapnilgm left a comment

swapnilgm Mar 16, 2020

swapnilgm Mar 18, 2020

swapnilgm Mar 18, 2020

swapnilgm Mar 18, 2020

swapnilgm Mar 18, 2020

swapnilgm Mar 18, 2020

	DeltaSnapshotMemoryLimit: 10 * 1024 * 1024, //10Mib
	DeltaSnapshotMemoryLimit: DefaultDeltaSnapMemoryLimit,

	DeltaSnapshotPeriod: wrappers.Duration{Duration: 20 * time.Second},
	DeltaSnapshotPeriod: wrappers.Duration{Duration: DefaultDeltaSnapshotInterval},

	# server-key: "ssl/etcdbr/tls.crt"
	# server-key: "ssl/etcdbr/tls.key"

Support config file for server configuration #208

Support config file for server configuration #208

Conversation

swapnilgm commented Dec 6, 2019 • edited Loading

shreyas-s-rao left a 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

Choose a reason for hiding this comment

shreyas-s-rao Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-s-rao Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-s-rao Feb 26, 2020 • edited Loading

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

swapnilgm left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swapnilgm commented Dec 6, 2019 •

edited

Loading

shreyas-s-rao Feb 26, 2020 •

edited

Loading

shreyas-s-rao Feb 26, 2020 •

edited

Loading

shreyas-s-rao Feb 26, 2020 •

edited

Loading