Fix update behavior when getting a zypper-related update #134

termdew · 2017-09-18T10:56:46Z

Fix the semantics of 'client.go', so that a second update process is executed, when a zypper-related update (with exitcode 103) was installed.
Change the explanation of the update process in the manpage.

Fix #110 .

Signed-off-by: Fabian Baumanis fabian.baumanis@suse.com

mssola · 2017-09-19T06:29:15Z

client.go

+			de := err.(dockerError)
+			if isZypperExitCodeSevere(de.exitCode) {
+				if de.exitCode == zypperExitInfRestartNeeded  {
+					goto run_cmd


I'm sure you can avoid using a goto statement 😉

I prefer using a goto here. Golang doesn't support do-while loops, so an alternative to it would be using a for loop and a variable indicating another run.

mssola · 2017-09-19T06:30:13Z

client.go

+	run_cmd:
+		id, err = startContainer(id, cmd, true, dst)
+		return id, err
+		switch err.(type) {


Is this going to be called at all ? (mind the previous return statement)

vrothberg

Please provide a commit message describing your changes and why they are necessary. Some comments in the code would also be great. Also, the descriptions of the changed functions should be changed to reflect the new semantics. startContainer, for instance, now expects a container ID and not an image name anymore.

vrothberg · 2017-09-19T07:21:54Z

client.go

+		de := err.(dockerError)
+		if isZypperExitCodeSevere(de.exitCode) {
+			if de.exitCode == zypperExitInfRestartNeeded {
+				goto run_cmd


The nesting of those if-branches isn't correct here. First, we need to return an error if the exit code is severe. Also the zypperExitInfRestartNeeded has the value 103, but severe ones are > 0 && < 100, so the if conditions are contradicting. What we should do instead is to return an error if the code is severe and move the run_cmd in a separate if branch.

mssola · 2017-09-19T14:06:23Z

client.go

+		return "", err
+	}
+
+run_cmd:


Use a regular for loop with a boolean value or something else. Please refrain from using a goto statement unless it's really necessary.

mssola · 2017-09-19T14:07:06Z

client.go

-		log.Println(err)
-		return "", err
-	}
+func startContainer(id string, cmd []string, wait bool, dst io.Writer) (string, error) {


Update the documentation as @vrothberg said. The semantics have changed, so the expectation has changed too, and the documentation should be in sync with the semantics.

startContainer(...) currently doesn't use the cmd argument. We need to fix this as the interface suggests that the passed cmd will actually be executed inside container id. Sorry for not catching this before.

Yeah, cmd shouldn't be passed now that we mirror the create-start API.

mssola

By the way, this will loop forever if it keeps on failing no ?

mssola · 2017-09-20T07:15:50Z

client.go

+	}
+
+	error103 := false
+	for error103 == false {


for !error103 {

I prefer a more intuitive identifier, for instance runCommand or execute or something that indicates the semantics behind.

@mssola let me explain my thoughts on this:

error103 is false, so 'startContainer' is executed the first time.
Now we assume, a 103 exitcode was returned.
So, he leaves error103 on false and startCommand will be executed again.
Now we assume, no exitcode 103 was returned.
Therefore we go in fo the default case, where error103 is set to true and startCommand won't be executed again.

If there are more zypper updates which return the exitcode 103, the for-loop runs until there is no exitcode 103 anymore.

BTW, i changed error103 to 'execute' as @vrothberg mentioned before.
Therefore, startCommand will be executed, as long as 'execute' is 'true'.

vrothberg · 2017-09-20T08:31:28Z

man/zypper-docker-up.1.md

-provided by the NEW-IMAGE argument. Note that both the IMAGE and the NEW\-IMAGE
-arguments have to follow Docker's naming format.
+with all the available updates. If there is a zypper-related update, which returns
+the exit code 103 (zypperExitInfRestartNeeded), zypper-docker goes through the 


The actual exit code of zypper is called ZYPPER_EXIT_INF_RESTART_NEEDED (see zypper's manpages). After that change, I think we're good to go 👍

cyphar · 2017-09-20T12:15:51Z

client.go

+			execute = false
+			return id, err
+		}
+		return id, err


Doesn't this line mean the loop will never run more than once?

which one? the "return id, err" ?

Line 208. To me it looks like:

for someCond { // some code return blah }

Which will always return blah (you need to use continue or similar to make the loop not hit that return statement).

cyphar · 2017-09-20T12:18:58Z

client.go

+	}
+
+	execute := true
+	for execute {


To avoid infinite loops we could do something like

for i := 0; i < 256; i++ { // snip: startContainer switch err.(type) { case dockerError: // snip: the severe check if de.exitCode == zypperExitInfRestartNeeded { continue } default: } return id, err } return "", some_error_about_too_many_retries

termdew · 2017-10-05T10:43:13Z

ping please review @cyphar @vrothberg @mssola

vrothberg · 2017-10-05T11:05:05Z

client.go

-		log.Println(err)
-		return "", err
-	}
+func startContainer(id string, cmd []string, wait bool, dst io.Writer) (string, error) {


startContainer(...) currently doesn't use the cmd argument. We need to fix this as the interface suggests that the passed cmd will actually be executed inside container id. Sorry for not catching this before.

Fix explanation of the update process in the manpage - startContainer and createContainer are now seperated - createContainer now only creates the container, which is started then by startContainer. - startContainer now only starts the container based on the container ID created by createContainer - after the severity check of the exit code another if-statement checks, if the exit code is 103 (zypperExitInfRestartNeeded) Signed-off-by: Fabian Baumanis <fabian.baumanis@suse.com>

termdew · 2017-10-05T11:48:03Z

Changes in client.go: removed the argument 'cmd' from the function 'startContainer' and from all elements which call 'startContainer'

vrothberg · 2017-10-05T12:32:27Z

@mssola Any remaining or new objections?

termdew · 2017-10-30T08:25:19Z

this PR is still open, anything still wrong with it, or can it be merged?

@mssola @cyphar @vrothberg

termdew requested review from mssola and vrothberg September 18, 2017 10:56

termdew force-pushed the UpdateFix branch from 93f35da to f92fdd6 Compare September 18, 2017 11:46

mssola suggested changes Sep 19, 2017

View reviewed changes

termdew force-pushed the UpdateFix branch from f92fdd6 to 90b593d Compare September 19, 2017 06:44

vrothberg suggested changes Sep 19, 2017

View reviewed changes

vrothberg reviewed Sep 19, 2017

View reviewed changes

termdew force-pushed the UpdateFix branch 2 times, most recently from 41cddc6 to 8aa353d Compare September 19, 2017 13:42

mssola suggested changes Sep 19, 2017

View reviewed changes

termdew force-pushed the UpdateFix branch from 8aa353d to e91d935 Compare September 20, 2017 06:46

mssola suggested changes Sep 20, 2017

View reviewed changes

termdew force-pushed the UpdateFix branch from e91d935 to b742598 Compare September 20, 2017 07:41

vrothberg reviewed Sep 20, 2017

View reviewed changes

vrothberg previously approved these changes Sep 20, 2017

View reviewed changes

termdew dismissed vrothberg’s stale review via a3edaaf September 20, 2017 08:44

termdew force-pushed the UpdateFix branch 2 times, most recently from a3edaaf to c3d0067 Compare September 20, 2017 12:19

cyphar reviewed Sep 20, 2017

View reviewed changes

termdew force-pushed the UpdateFix branch 2 times, most recently from 3c9ddcc to 8c593db Compare September 25, 2017 06:17

vrothberg suggested changes Oct 5, 2017

View reviewed changes

termdew force-pushed the UpdateFix branch from 8c593db to d21efad Compare October 5, 2017 11:47

vrothberg approved these changes Oct 5, 2017

View reviewed changes

mssola approved these changes Oct 5, 2017

View reviewed changes

vrothberg merged commit a6b7947 into SUSE:master Oct 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix update behavior when getting a zypper-related update #134

Fix update behavior when getting a zypper-related update #134

termdew commented Sep 18, 2017 •

edited

Loading

mssola Sep 19, 2017

vrothberg Sep 19, 2017 •

edited

Loading

mssola Sep 19, 2017

vrothberg left a comment

vrothberg Sep 19, 2017

mssola Sep 19, 2017

mssola Sep 19, 2017

vrothberg Oct 5, 2017 •

edited

Loading

cyphar Oct 5, 2017

mssola left a comment

mssola Sep 20, 2017

vrothberg Sep 20, 2017

termdew Sep 20, 2017

termdew Sep 20, 2017

vrothberg Sep 20, 2017

cyphar Sep 20, 2017

termdew Sep 20, 2017

cyphar Sep 20, 2017

cyphar Sep 20, 2017

termdew commented Oct 5, 2017

vrothberg Oct 5, 2017 •

edited

Loading

termdew commented Oct 5, 2017

vrothberg commented Oct 5, 2017

termdew commented Oct 30, 2017

Fix update behavior when getting a zypper-related update #134

Fix update behavior when getting a zypper-related update #134

Conversation

termdew commented Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

vrothberg Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg 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

vrothberg Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssola 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

termdew commented Oct 5, 2017

vrothberg Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

termdew commented Oct 5, 2017

vrothberg commented Oct 5, 2017

termdew commented Oct 30, 2017

termdew commented Sep 18, 2017 •

edited

Loading

vrothberg Sep 19, 2017 •

edited

Loading

vrothberg Oct 5, 2017 •

edited

Loading

vrothberg Oct 5, 2017 •

edited

Loading