cmd/main: Prevent to load syncthing along with new config file (fixes #4921) #5164

bahadir60 · 2018-09-05T15:34:47Z

Purpose

Don't let syncthing running along with a future config file.

Current

In cmd/main.go checks configuration version at disk and current configuration version which defined in config.go. If they are not same archived disk version. However future configuration file which store on disk means newer configuration schema may not be compatible for Syncthing.

After Code

If config.xml version is bigger than config.CurrentVersion, don't let to run Syncthing and give information about versions and suggest to use -downgradeConfig flag. Also, if compatible config was stored in disk, suggest to use it.
If -downgradeConfig flag used, first checking needed to use flag. If no needed, info to user. Flag makes a new config based on disk config and store old one.

Documentation

Adding -downgradeConfig flag to 2.1.1. Synopsis and 2.1.3. Options on page "https://docs.syncthing.net/users/syncthing.html?highlight=flag" will be suitable.

bahadir60 · 2018-09-05T15:38:36Z

I also check the Sycnthing version wheather downgrade or not and if downgrade logged it.

calmh

LGTM in principle, but I still think we should have something like --allow-configuration-downgrade that will allow Syncthing to make a "best effort" start in this case.

calmh · 2018-09-05T15:57:40Z

cmd/syncthing/main.go

@@ -741,6 +741,10 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 	curParts := strings.Split(Version, "-")
 	if prevParts[0] != curParts[0] {
 		if prevVersion != "" {
+			//Check Syncthing version is wheather downgrading
+			if upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.Older || upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.MajorOlder {
+				l.Infof("Current Syncthing Version: %s, Loading Syncthing Version: %s: (Syncthing Version Mismatch)", prevVersion, Version)


So this is a valid check, I'd go with making it a warning and tweak the wording. How about

l.Warnf("Downgrade detected: %s to %s", prevVersion, Version)

Making it a warning will mean it's also popped up in the GUI.

Definitely change the wording, I didn't understand what the original text tried to say (also agree on warning).

calmh · 2018-09-05T15:59:36Z

cmd/syncthing/main.go

@@ -965,6 +969,12 @@ func loadConfigAtStartup() *config.Wrapper {
 	}

 	if cfg.RawCopy().OriginalVersion != config.CurrentVersion {
+		//check config version
+		if cfg.RawCopy().OriginalVersion > config.CurrentVersion {
+			l.Fatalf("Current Configuration Version: %d, Loading Configuration Version: %d (Configuration Version Mismatch)", cfg.RawCopy().OriginalVersion, config.CurrentVersion)


I'd like to adjust the wording here as well to something more sentence like.

l.Fatalf("Configuration version on disk not understood by this version of Syncthing; loaded %d > current %d", ...OriginalVersion, config.CurrentVersion)

Oh, and the os.Exit is unnecessary because a Fatalf will never return.

Also maybe mention that there may be a copy of a config with the compatible version that we created on previous update. As a deluxe variant (just proposition, not necessary), even check if it is there.

AudriusButkevicius · 2018-09-05T16:39:05Z

cmd/syncthing/main.go

@@ -741,6 +741,10 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 	curParts := strings.Split(Version, "-")
 	if prevParts[0] != curParts[0] {
 		if prevVersion != "" {
+			//Check Syncthing version is wheather downgrading
+			if upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.Older || upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.MajorOlder {
+				l.Infof("Current Syncthing Version: %s, Loading Syncthing Version: %s: (Syncthing Version Mismatch)", prevVersion, Version)


Also the line below should probably go i to an else

AudriusButkevicius · 2018-09-05T16:39:11Z

cmd/syncthing/main.go

@@ -741,6 +741,10 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 	curParts := strings.Split(Version, "-")
 	if prevParts[0] != curParts[0] {
 		if prevVersion != "" {
+			//Check Syncthing version is wheather downgrading


Ditto about spaces

AudriusButkevicius · 2018-09-05T16:39:13Z

cmd/syncthing/main.go

+		//check config version
+		if cfg.RawCopy().OriginalVersion > config.CurrentVersion {
+			l.Fatalf("Current Configuration Version: %d, Loading Configuration Version: %d (Configuration Version Mismatch)", cfg.RawCopy().OriginalVersion, config.CurrentVersion)
+			//Config version is not compatible with installed version


Space after slashes

imsodin · 2018-09-05T20:13:18Z

cmd/syncthing/main.go

@@ -741,6 +741,10 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 	curParts := strings.Split(Version, "-")
 	if prevParts[0] != curParts[0] {
 		if prevVersion != "" {
+			//Check Syncthing version is wheather downgrading
+			if upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.Older || upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.MajorOlder {


Totally not necessary, just noticed: I'd add a upgrade.Relation.IsOlder convenience function (there is some other place where essentially this condition is used).

I did not want to change upgrade package like adding methods. But upgrade.Relation.IsOlder is a good suggestion I will try that thanx

imsodin · 2018-09-05T20:14:50Z

cmd/syncthing/main.go

@@ -741,6 +741,10 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 	curParts := strings.Split(Version, "-")
 	if prevParts[0] != curParts[0] {
 		if prevVersion != "" {
+			//Check Syncthing version is wheather downgrading
+			if upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.Older || upgrade.CompareVersions(curParts[0], prevParts[0]) == upgrade.MajorOlder {
+				l.Infof("Current Syncthing Version: %s, Loading Syncthing Version: %s: (Syncthing Version Mismatch)", prevVersion, Version)


Definitely change the wording, I didn't understand what the original text tried to say (also agree on warning).

imsodin · 2018-09-05T20:16:13Z

cmd/syncthing/main.go

@@ -965,6 +969,12 @@ func loadConfigAtStartup() *config.Wrapper {
 	}

 	if cfg.RawCopy().OriginalVersion != config.CurrentVersion {
+		//check config version
+		if cfg.RawCopy().OriginalVersion > config.CurrentVersion {


Define a local variable in the if above and reuse here to avoid copying twice.

I will change it as Jacob' suggests for wording. Thanks for local variable suggestion.

This is still to be addressed.

imsodin · 2018-09-05T20:19:16Z

cmd/syncthing/main.go

@@ -965,6 +969,12 @@ func loadConfigAtStartup() *config.Wrapper {
 	}

 	if cfg.RawCopy().OriginalVersion != config.CurrentVersion {
+		//check config version
+		if cfg.RawCopy().OriginalVersion > config.CurrentVersion {
+			l.Fatalf("Current Configuration Version: %d, Loading Configuration Version: %d (Configuration Version Mismatch)", cfg.RawCopy().OriginalVersion, config.CurrentVersion)


Also maybe mention that there may be a copy of a config with the compatible version that we created on previous update. As a deluxe variant (just proposition, not necessary), even check if it is there.

bahadir60 · 2018-09-05T21:09:30Z

@imsodin As you know old config files are stored for a month time. My worry is about some part of the configuration will may be changed during that time, like devices or folders. But may be I will merge @calmh ' suggestion (for adding --allow-configuration-downgrade that will allow Syncthing to make a "best effort" start) and yours' when working on --allow-configuration-downgrade part. Thanx for your comment.

bahadir60 · 2018-10-09T18:57:16Z

Based on comments, added -allow-configuration-downgrade flag.

imsodin · 2018-10-10T17:19:57Z

I don't understand the point of a separate downgradeConfig function. I'd just add a allowConfigDowngrade parameter to loadConfigAtStartup.

bahadir60 · 2018-10-30T13:25:55Z

Imsodin, thanks your comment. I didn't want to a heavy function so that I separated it. Now I've got it together.

cmd/syncthing/main.go

imsodin · 2018-10-30T17:16:04Z

cmd/syncthing/main.go

+		// Check config version
+		if cfg.RawCopy().OriginalVersion > config.CurrentVersion {
+			// Config version is not compatible with installed version
+			if allowDowngrade {


Invert the condition so you can fail early and then the entire following block doesn't need to be indented.

imsodin · 2018-10-30T17:20:22Z

cmd/syncthing/main.go

+			// Config version is not compatible with installed version
+			if allowDowngrade {
+				l.Infof("Current Configuration Version: %d, Loading Configuration Version: %d (Configuration Version Mismatch)", configVersion, config.CurrentVersion)
+				l.Infof("Configuration downgrading...")


This isn't a very slow operation, so I wouldn't log anything in advance, but at the end, when it is clear whether the downgrade worked or not.

cmd/syncthing/main.go

…yncthing#4921)

bahadir60 · 2018-11-05T08:47:00Z

@imsodin based on your comments I made changes thanx

imsodin

Just to be clear: I am deep in nit-pick territory with my current comments ;)

imsodin · 2018-11-05T08:54:55Z

cmd/syncthing/main.go

+				// Read configuration second time, Unmarshal to configuration struct and Change Version
+				var cfgConfig config.Configuration
+				var buf bytes.Buffer
+				err = json.NewEncoder(&buf).Encode(cfgXML)


Just replace cfgXML with cfg.RawCopy() and remove the line with cfgXML := cfg.RawCopy() above.

imsodin · 2018-11-05T08:57:09Z

cmd/syncthing/main.go

+				}
+				l.Infof("Use `-downgradeConfig' flag to downgrade your config.")
+				l.Fatalf("Current Configuration Version: %d, Loading Configuration Version: %d (Configuration Version Mismatch)", configVersion, config.CurrentVersion)
+			} else {


No need for the else. If the conditoin above is true, l.Fatalf aborts execution anyway.

imsodin · 2018-11-05T08:58:29Z

cmd/syncthing/main.go

+				cfgXML := cfg.RawCopy()
+
+				// Read configuration second time, Unmarshal to configuration struct and Change Version
+				var cfgConfig config.Configuration


Propositions: newConfig, downgradedConfig, ...

imsodin · 2018-11-05T09:01:07Z

cmd/syncthing/main.go

+
+				cfgConfig.Version = config.CurrentVersion
+				cfgConfig.OriginalVersion = config.CurrentVersion
+				cfgConfigWrapper := config.Wrap(cfgFile, cfgConfig)


Do you need a new wrapper instead of cfg.Replace(cfgConfig)?

imsodin · 2018-11-05T09:08:52Z

cmd/syncthing/main.go

-			l.Infoln("Detected upgrade from", prevVersion, "to", Version)
+			// Check Syncthing version
+			checkRelation := upgrade.CompareVersions(curParts[0], prevParts[0])
+			if checkRelation == upgrade.Older || checkRelation == upgrade.MajorOlder {


Inline the definition and I'd use <:
if relation := upgrade.CompareVersions(curParts[0], prevParts[0]); relation < 0

imsodin · 2018-11-05T09:09:47Z

cmd/syncthing/main.go

@@ -965,6 +969,12 @@ func loadConfigAtStartup() *config.Wrapper {
 	}

 	if cfg.RawCopy().OriginalVersion != config.CurrentVersion {
+		//check config version
+		if cfg.RawCopy().OriginalVersion > config.CurrentVersion {


This is still to be addressed.

st-review · 2019-02-04T07:02:22Z

🤖 beep boop

I'm going to close this pull request as it has been idle for more than 90 days.

This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved.

calmh reviewed Sep 5, 2018

View reviewed changes

AudriusButkevicius reviewed Sep 5, 2018

View reviewed changes

imsodin reviewed Sep 5, 2018

View reviewed changes

bahadir60 force-pushed the bh-4921 branch 3 times, most recently from 22f449a to da3c340 Compare October 9, 2018 18:51

bahadir60 force-pushed the bh-4921 branch from da3c340 to c5eb4c5 Compare October 9, 2018 18:59

bahadir60 force-pushed the bh-4921 branch from c5eb4c5 to 54e2a56 Compare October 30, 2018 13:21

imsodin requested changes Oct 30, 2018

View reviewed changes

bahadir60 force-pushed the bh-4921 branch from 54e2a56 to f7abc66 Compare November 5, 2018 08:36

cmd/main: Prevent to load syncthing along with new config file (fixes s…

24ba082

…yncthing#4921)

bahadir60 force-pushed the bh-4921 branch from f7abc66 to 24ba082 Compare November 5, 2018 08:42

imsodin requested changes Nov 5, 2018

View reviewed changes

st-review closed this Feb 4, 2019

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Feb 4, 2020

syncthing locked and limited conversation to collaborators Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/main: Prevent to load syncthing along with new config file (fixes #4921) #5164

cmd/main: Prevent to load syncthing along with new config file (fixes #4921) #5164

bahadir60 commented Sep 5, 2018 •

edited

bahadir60 commented Sep 5, 2018

calmh left a comment

calmh Sep 5, 2018 •

edited

imsodin Sep 5, 2018

calmh Sep 5, 2018 •

edited

imsodin Sep 5, 2018

AudriusButkevicius Sep 5, 2018

AudriusButkevicius Sep 5, 2018

AudriusButkevicius Sep 5, 2018

imsodin Sep 5, 2018

bahadir60 Sep 5, 2018

imsodin Sep 5, 2018

imsodin Sep 5, 2018

bahadir60 Sep 5, 2018 •

edited

imsodin Nov 5, 2018

imsodin Sep 5, 2018

bahadir60 commented Sep 5, 2018 •

edited

bahadir60 commented Oct 9, 2018

imsodin commented Oct 10, 2018

bahadir60 commented Oct 30, 2018

imsodin Oct 30, 2018

imsodin Oct 30, 2018

bahadir60 commented Nov 5, 2018

imsodin left a comment

imsodin Nov 5, 2018

imsodin Nov 5, 2018

imsodin Nov 5, 2018

imsodin Nov 5, 2018

imsodin Nov 5, 2018

imsodin Nov 5, 2018

st-review commented Feb 4, 2019

cmd/main: Prevent to load syncthing along with new config file (fixes #4921) #5164

cmd/main: Prevent to load syncthing along with new config file (fixes #4921) #5164

Conversation

bahadir60 commented Sep 5, 2018 • edited

Purpose

Documentation

bahadir60 commented Sep 5, 2018

calmh left a comment

Choose a reason for hiding this comment

calmh Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calmh Sep 5, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahadir60 Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahadir60 commented Sep 5, 2018 • edited

bahadir60 commented Oct 9, 2018

imsodin commented Oct 10, 2018

bahadir60 commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahadir60 commented Nov 5, 2018

imsodin 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

st-review commented Feb 4, 2019

bahadir60 commented Sep 5, 2018 •

edited

calmh Sep 5, 2018 •

edited

calmh Sep 5, 2018 •

edited

bahadir60 Sep 5, 2018 •

edited

bahadir60 commented Sep 5, 2018 •

edited