RSDK-5608 - CLI commands for managing cloud builds #3298
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
| if ctx.Err() != nil { | ||
| return ctx.Err() | ||
| } | ||
| log, err := stream.Recv() |
There was a problem hiding this comment.
What is the type of stream and can Recv() block forever? I'm surprised Recv() does not take a context if this is doing network IO.
There was a problem hiding this comment.
It is quite surprising that it doesn't take a context. After looking it up, it seems like this is a known issue and the fix is to just put it in a goroutine 🤦 grpc/grpc-go#1229 (comment)
There was a problem hiding this comment.
just put it in a goroutine
Hmm gotcha; thanks for looking into it. Curious to see what that will look like here; my intuition is that you will inevitably potentially leak that goroutine. FWIW old Golang libraries (apparently e.g. grpc-go) sometimes have network I/O functions that do not take contexts, as contexts weren't around/popular at the time of writing the library (unclear if that's the reason for lack of context here, just a hypothesis). The "workaround"s usually look like an alternative "context" version of the method like stream.RecvCtx(...) or some stream.SetTimeout(...) method to control the deadline of the Recv call.
There was a problem hiding this comment.
stream.RecvCtx(...) or some stream.SetTimeout(...) method to control the deadline of the Recv call.
That is what I was hoping to find, but I can't seem to find it.
There was a problem hiding this comment.
I've put a basic impl in the bottom of f1d120d
It is a bit inefficient because it creates a new goroutine every loop, but from what I've been reading about goroutines, that seems cheap compared to network io.
If you have a better impl, I would love to see what you come up with.
There was a problem hiding this comment.
Reverted to Recv because I /think/ it respects the ctx of the stream which is the ctx of the cli (passed to GetLogs).
I haven't tested this and it is many layers deep in grpc code, but I am ~60% confident. (also the rest of viam uses the same pattern so if it is a problem, it is a problem everywhere. so we should fix it then)
grpc/internal/transport/transport.go:321
func (s *Stream) waitOnHeader() {
if s.headerChan == nil {
// On the server headerChan is always nil since a stream originates
// only after having received headers.
return
}
select {
case <-s.ctx.Done():
// Close the stream to prevent headers/trailers from changing after
// this function returns.
s.ct.CloseStream(s, ContextErr(s.ctx.Err()))
// headerChan could possibly not be closed yet if closeStream raced
// with operateHeaders; wait until it is closed explicitly here.
<-s.headerChan
case <-s.headerChan:
}
}
// RecvCompress returns the compression algorithm applied to the inbound
// message. It is empty string if there is no compression applied.
func (s *Stream) RecvCompress() string {
s.waitOnHeader()
return s.recvCompress
}
There was a problem hiding this comment.
(also talked offline)
There was a problem hiding this comment.
Cool; after offline discussion, stream.Recv() seems totally fine for now (especially given, as you pointed out, the prevalence of no-timeout-network-IO across the CLI). Thanks for digging into this; I'll trust the 60% for now and we can reconsider if NetCode and/or fleet decide we need sensible default timeouts for the CLI.
cli/app.go
Outdated
| @@ -40,6 +40,12 @@ const ( | |||
| moduleFlagPlatform = "platform" | |||
| moduleFlagForce = "force" | |||
|
|
|||
| moduleBuildFlagNumber = "number" | |||
There was a problem hiding this comment.
Initially read as build number, something like PR number, so recommend count instead. I think it will reduce any confusion that users might have.
| moduleBuildFlagNumber = "number" | |
| moduleBuildFlagCount = "count" |
There was a problem hiding this comment.
Previously:
NAME:
viam module build list - check on the status of your cloud builds
USAGE:
viam module build list [command options] [arguments...]
OPTIONS:
--id value restrict output to just return builds that match this id
--module value path to meta.json (default: "./meta.json")
--number value, -n value number of builds to list (default: all)
Now:
NAME:
viam module build list - check on the status of your cloud builds
USAGE:
viam module build list [command options] [arguments...]
OPTIONS:
--count value, -c value number of builds to list (default: all)
--id value restrict output to just return builds that match this id
--module value path to meta.json (default: "./meta.json")
"--count" > "--number" however "-n" > "-c" in my opinion... not sure. (also not sure it really matters.)
Happy to go either way (count or number) if you still think count is better
There was a problem hiding this comment.
hmm. just do -c because usually single character flags should match the first character of the larger one.
| Name: moduleBuildFlagNumber, | ||
| Usage: "number of builds to list", | ||
| Aliases: []string{"n"}, | ||
| DefaultText: "all", |
There was a problem hiding this comment.
I tried to look this up myself but couldn't figure it out. Should DefaultText always come with a Value? Is it that DefaultText is a suggested value where as Value is an actual default?
There was a problem hiding this comment.
Yup, this surprising behavior was fixed for the other cli commands in #3162
My understanding is that:
DefaultText => shows in help msg, does not actually change the default value when you try to read the flag
Value => shows in help msg + actual default value
There was a problem hiding this comment.
Yup thanks for confirming what I found. I feel like DefaultText isn't appropriate then. The "default" is that you don't specify the flag at all, otherwise you specify it and it should be a int.
There was a problem hiding this comment.
Ok, as we discussed keep it as-is.
viambot
added
safe to test
viambot
added
safe to test
| if ctx.Err() != nil { | ||
| return ctx.Err() | ||
| } | ||
| log, err := stream.Recv() |
There was a problem hiding this comment.
Cool; after offline discussion, stream.Recv() seems totally fine for now (especially given, as you pointed out, the prevalence of no-timeout-network-IO across the CLI). Thanks for digging into this; I'll trust the 60% for now and we can reconsider if NetCode and/or fleet decide we need sensible default timeouts for the CLI.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
Code Coverage
|
Co-authored-by: Abe Winter <abe-winter@users.noreply.github.com>
Ticket: https://viam.atlassian.net/browse/RSDK-5608
This adds the cli commands for managing cloud builds. The backend changes to support these new APIs in app have not been merged yet.
The help text for these functions is still hidden, so these functions can still be considered mostly-internal.
We have been encouraged to merge sooner rather than later so probably okay to merge with some functionality unfinished.
Output:
Note:
This does not have any tests yet.
If we are happy with the functionality, I will happily write a few tests.