Issue 7174046: code review 7174046: misc/dashboard/builder: automatic update functionality
Descriptionmisc/dashboard/builder: automatic update functionality
Patch Set 1 #Patch Set 2 : diff -r 7dc8d66efb6d https://code.google.com/p/go #
Total comments: 9
Patch Set 3 : diff -r 7929ab4ff133 https://code.google.com/p/go #Patch Set 4 : diff -r 7929ab4ff133 https://code.google.com/p/go #Patch Set 5 : diff -r f50a112bfe3b https://code.google.com/p/go #Patch Set 6 : diff -r f50a112bfe3b https://code.google.com/p/go #Patch Set 7 : diff -r 2becc54569bf https://code.google.com/p/go #Patch Set 8 : diff -r 2becc54569bf https://code.google.com/p/go #Patch Set 9 : diff -r bfb45be43e2b https://code.google.com/p/go #Patch Set 10 : diff -r bfb45be43e2b https://code.google.com/p/go #Patch Set 11 : diff -r bfb45be43e2b https://code.google.com/p/go #Patch Set 12 : diff -r bfb45be43e2b https://code.google.com/p/go #
MessagesTotal messages: 20
https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/main.go File misc/dashboard/builder/main.go (left): https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/main.... misc/dashboard/builder/main.go:51: commitFlag = flag.Bool("commit", false, "upload information about new commits") so essentially every builder will be a commit builder? Will it pose extra load on slower builders? https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... File misc/dashboard/builder/update.go (right): https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... misc/dashboard/builder/update.go:25: const builderVersion = "TBA" // TODO(adg): set after this code is committed take a look at -ldflags -X? we can embed the version in the binary itself (but please write it to log when the builder starts) https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... misc/dashboard/builder/update.go:55: binary := filepath.Join(*buildroot, "builder-"+rev) proper clean up of old builder binary? (but we need a fallback mechanism when the new binary doesn't start correctly) i think every system supports renaming a binary even when a process is executing it. https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... misc/dashboard/builder/update.go:65: if err := syscall.Exec(binary, os.Args[1:], os.Environ()); err != nil { syscall.Exec is not supported on windows, you'd better just use os/exec and a pipe for notification.
Sign in to reply to this message.
https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/main.go File misc/dashboard/builder/main.go (left): https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/main.... misc/dashboard/builder/main.go:51: commitFlag = flag.Bool("commit", false, "upload information about new commits") On 2013/01/22 13:28:13, minux wrote: > so essentially every builder will be a commit builder? > Will it pose extra load on slower builders? See http://golang.org/cl/7178046
Sign in to reply to this message.
PTAL https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... File misc/dashboard/builder/update.go (right): https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... misc/dashboard/builder/update.go:25: const builderVersion = "TBA" // TODO(adg): set after this code is committed On 2013/01/22 13:28:13, minux wrote: > take a look at -ldflags -X? > we can embed the version in the binary itself > (but please write it to log when the builder > starts) We don't want this. This const value should be set manually by us when we want the builders to update to a newer version. https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... misc/dashboard/builder/update.go:55: binary := filepath.Join(*buildroot, "builder-"+rev) On 2013/01/22 13:28:13, minux wrote: > proper clean up of old builder binary? > (but we need a fallback mechanism when the new > binary doesn't start correctly) > > i think every system supports renaming a binary even > when a process is executing it. I don't mind leaving the old binary around. We may not even have write access to it. https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... misc/dashboard/builder/update.go:65: if err := syscall.Exec(binary, os.Args[1:], os.Environ()); err != nil { On 2013/01/22 13:28:13, minux wrote: > syscall.Exec is not supported on windows, > you'd better just use os/exec and a pipe for notification. Done. I don't think a pipe is necessary. If the update failed, we're screwed by this point anyway. I'm not sure it's worth updating cautiously. If there are problems there'll need to be manual intervention anyway.
Sign in to reply to this message.
https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... File misc/dashboard/builder/update.go (right): https://codereview.appspot.com/7174046/diff/2001/misc/dashboard/builder/updat... misc/dashboard/builder/update.go:55: binary := filepath.Join(*buildroot, "builder-"+rev) On 2013/01/23 05:25:50, adg wrote: > On 2013/01/22 13:28:13, minux wrote: > > proper clean up of old builder binary? > > (but we need a fallback mechanism when the new > > binary doesn't start correctly) > > > > i think every system supports renaming a binary even > > when a process is executing it. > > I don't mind leaving the old binary around. We may not even have write access to > it. I'm just slightly concerned about old binaries filling the space as some builders run on tmpfs.
Sign in to reply to this message.
Hello bradfitz@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
On 2013/01/23 11:15:24, minux wrote: > I'm just slightly concerned about old binaries filling the > space as some builders run on tmpfs. Can you suggest a strategy for disposing of the old binaries? Should the running binary rm itself after executing the new builder?
Sign in to reply to this message.
On Wed, Jan 30, 2013 at 8:50 AM, <adg@golang.org> wrote: > On 2013/01/23 11:15:24, minux wrote: > >> I'm just slightly concerned about old binaries filling the >> space as some builders run on tmpfs. >> > > Can you suggest a strategy for disposing of the old binaries? Should the > running binary rm itself after executing the new builder? > On windows at least you can't remove the binary when the program is current executing (not without very very tricky hacks). But we can rename the binary to another name (for example, gobuilder.old(.exe) and overwrite that name, so that we always keep at most two builder binaries: the current one and the previous one) PS: please consider display the builder version at startup or provide a way to query it.
Sign in to reply to this message.
On 30 January 2013 19:21, minux <minux.ma@gmail.com> wrote: > > On Wed, Jan 30, 2013 at 8:50 AM, <adg@golang.org> wrote: > >> On 2013/01/23 11:15:24, minux wrote: >> >>> I'm just slightly concerned about old binaries filling the >>> space as some builders run on tmpfs. >>> >> >> Can you suggest a strategy for disposing of the old binaries? Should the >> running binary rm itself after executing the new builder? >> > On windows at least you can't remove the binary when the program is current > executing (not without very very tricky hacks). But we can rename the > binary > to another name (for example, gobuilder.old(.exe) and overwrite that name, > so > that we always keep at most two builder binaries: the current one and the > previous > one) > OK, done. PTAL. PS: please consider display the builder version at startup or provide a way > to > query it. > Done.
Sign in to reply to this message.
Hello, In general I am wary of a binary replacing itself, I think it is fraught with dangers and edge cases. However if the builder was able fork a child builder which actually did the building, and it was that child that was replaced, I would be more comfortable with that approach. This would mean that on startup, the builder could do the following steps 1. checks $WORKSPACE/goroot exists and is current 2. determines the builder version it needs to run 3. checks if an existing builder exists in $WORKSPACE, say $WORKSPACE/builder-$REV, and builds it if it does not exist 4. builder calls $WORKSPACE/builder-$REV will os.Args[1:] + '-child' 5. the child builder does the build loop check, and has some logic to tell if its rev does not match the required rev, and if so, quits and the logic returns to point 3 in the parent. I can't speak for how others run their builders, but I generally run them under daemontools in an account that does not have go in its $PATH. Obviously this would have to change to implement this feature, but it sounds like a reasonable compromise.
Sign in to reply to this message.
That's a lot more complex than what I have here. The way I figure it: if a builder replaces itself and then the new one is broken, that's something we need to fix anyway, right? I don't necessarily think your proposal is a bad idea, but I don't see myself implementing it any time soon. On 24 February 2013 09:33, <dave@cheney.net> wrote: > Hello, > > In general I am wary of a binary replacing itself, I think it is fraught > with dangers and edge cases. However if the builder was able fork a > child builder which actually did the building, and it was that child > that was replaced, I would be more comfortable with that approach. > > This would mean that on startup, the builder could do the following > steps > > 1. checks $WORKSPACE/goroot exists and is current > 2. determines the builder version it needs to run > 3. checks if an existing builder exists in $WORKSPACE, say > $WORKSPACE/builder-$REV, and builds it if it does not exist > 4. builder calls $WORKSPACE/builder-$REV will os.Args[1:] + '-child' > 5. the child builder does the build loop check, and has some logic to > tell if its rev does not match the required rev, and if so, quits and > the logic returns to point 3 in the parent. > > I can't speak for how others run their builders, but I generally run > them under daemontools in an account that does not have go in its $PATH. > Obviously this would have to change to implement this feature, but it > sounds like a reasonable compromise. > > https://codereview.appspot.**com/7174046/<https://codereview.appspot.com/7174... >
Sign in to reply to this message.
That is a fair statement, this doesn't sound like the most important thing for you at the moment. If there is general agreement in my proposal then I can tackle that next week. On Tue, Feb 26, 2013 at 11:08 AM, Andrew Gerrand <adg@golang.org> wrote: > That's a lot more complex than what I have here. > > The way I figure it: if a builder replaces itself and then the new one is > broken, that's something we need to fix anyway, right? > > I don't necessarily think your proposal is a bad idea, but I don't see > myself implementing it any time soon. > > > > On 24 February 2013 09:33, <dave@cheney.net> wrote: >> >> Hello, >> >> In general I am wary of a binary replacing itself, I think it is fraught >> with dangers and edge cases. However if the builder was able fork a >> child builder which actually did the building, and it was that child >> that was replaced, I would be more comfortable with that approach. >> >> This would mean that on startup, the builder could do the following >> steps >> >> 1. checks $WORKSPACE/goroot exists and is current >> 2. determines the builder version it needs to run >> 3. checks if an existing builder exists in $WORKSPACE, say >> $WORKSPACE/builder-$REV, and builds it if it does not exist >> 4. builder calls $WORKSPACE/builder-$REV will os.Args[1:] + '-child' >> 5. the child builder does the build loop check, and has some logic to >> tell if its rev does not match the required rev, and if so, quits and >> the logic returns to point 3 in the parent. >> >> I can't speak for how others run their builders, but I generally run >> them under daemontools in an account that does not have go in its $PATH. >> Obviously this would have to change to implement this feature, but it >> sounds like a reasonable compromise. >> >> https://codereview.appspot.com/7174046/ > >
Sign in to reply to this message.
On 24 February 2013 09:33, <dave@cheney.net> wrote: > 2. determines the builder version it needs to run How does it do this?
Sign in to reply to this message.
I hadn't really thought about it too much, probably by inspecting the source in $WORKSPACE/goroot/... On Tue, Feb 26, 2013 at 11:14 AM, Andrew Gerrand <adg@golang.org> wrote: > > On 24 February 2013 09:33, <dave@cheney.net> wrote: >> >> 2. determines the builder version it needs to run > > > How does it do this?
Sign in to reply to this message.
On 26 February 2013 11:17, Dave Cheney <dave@cheney.net> wrote: > I hadn't really thought about it too much, probably by inspecting the > source in $WORKSPACE/goroot/... > So which running binary is responsible for keeping goroot up to date?
Sign in to reply to this message.
Here is the logic that I had in mind 1. parent starts, creates or updates $WORKSPACE/goroot 2. parent builds child from source in $WORKSPACE/goroot (if required), using some tag inside the source itself (waves hands) 3. parent executes child with an additional flag to avoid the child thinking it is the parent. 4. the child executes the commit watcher and builder until it notices that the source of $WORKSPACE/goroot says that it is not the current revision then it returns to the parent 5. the parent goes to step 2. On Tue, Feb 26, 2013 at 11:19 AM, Andrew Gerrand <adg@golang.org> wrote: > > On 26 February 2013 11:17, Dave Cheney <dave@cheney.net> wrote: >> >> I hadn't really thought about it too much, probably by inspecting the >> source in $WORKSPACE/goroot/... > > > So which running binary is responsible for keeping goroot up to date?
Sign in to reply to this message.
Seems reasonable. I guess you can re-use the parts of this CL that relate to step 4. On 26 February 2013 11:24, Dave Cheney <dave@cheney.net> wrote: > Here is the logic that I had in mind > > 1. parent starts, creates or updates $WORKSPACE/goroot > 2. parent builds child from source in $WORKSPACE/goroot (if required), > using some tag inside the source itself (waves hands) > 3. parent executes child with an additional flag to avoid the child > thinking it is the parent. > 4. the child executes the commit watcher and builder until it notices > that the source of $WORKSPACE/goroot says that it is not the current > revision then it returns to the parent > 5. the parent goes to step 2. > > > > On Tue, Feb 26, 2013 at 11:19 AM, Andrew Gerrand <adg@golang.org> wrote: > > > > On 26 February 2013 11:17, Dave Cheney <dave@cheney.net> wrote: > >> > >> I hadn't really thought about it too much, probably by inspecting the > >> source in $WORKSPACE/goroot/... > > > > > > So which running binary is responsible for keeping goroot up to date? >
Sign in to reply to this message.
Cool. It is on my todo list for next week. On Thu, Feb 28, 2013 at 1:21 PM, Andrew Gerrand <adg@golang.org> wrote: > Seems reasonable. I guess you can re-use the parts of this CL that relate to > step 4. > > > On 26 February 2013 11:24, Dave Cheney <dave@cheney.net> wrote: >> >> Here is the logic that I had in mind >> >> 1. parent starts, creates or updates $WORKSPACE/goroot >> 2. parent builds child from source in $WORKSPACE/goroot (if required), >> using some tag inside the source itself (waves hands) >> 3. parent executes child with an additional flag to avoid the child >> thinking it is the parent. >> 4. the child executes the commit watcher and builder until it notices >> that the source of $WORKSPACE/goroot says that it is not the current >> revision then it returns to the parent >> 5. the parent goes to step 2. >> >> >> >> On Tue, Feb 26, 2013 at 11:19 AM, Andrew Gerrand <adg@golang.org> wrote: >> > >> > On 26 February 2013 11:17, Dave Cheney <dave@cheney.net> wrote: >> >> >> >> I hadn't really thought about it too much, probably by inspecting the >> >> source in $WORKSPACE/goroot/... >> > >> > >> > So which running binary is responsible for keeping goroot up to date? > >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
