Skip to content

Commit

Permalink
Use sync/atomic to keep tab on go routines
Browse files Browse the repository at this point in the history
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>

As a result of the code pushed in
#5942, `odo logs --follow`
would hang if user executed it without pushing the component in either
dev or deploy modes. This was not a problem before that PR was merged.

This was caused because below code would not get executed in business
layer if user was using follow mode:

```
if !follow {
    doneChan <- struct{}{}
}
```

The reason we didn't execute it for follow mode was that it prematurely
exited and often didn't print the logs of all the containers.

This PR uses `sync/atomic` to keep a tab on go routines started for `odo
logs --follow` instead of using `sync.WaitGroup`, and checks if no go
routines are running before it exits. This enables us to use `doneChan
<- struct{}{}` in business layer irrespective of user running `odo logs`
with or without follow mode.

Why not use `wg.Wait()` as first statement instead in
`case <-events.Done:`? This is because `wg.Wait()` is a blocking call
and won't be called upon each time a go routine for `odo logs --follow`
is done (`wg.Done()`). This leads to same problem as that fixed by PR
5942, which is that `odo logs --follow` won't exit even if the
underlying odo component was deleted.
  • Loading branch information
dharmit committed Jul 21, 2022
1 parent a02fd6a commit d4c9206
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 14 deletions.
4 changes: 1 addition & 3 deletions pkg/logs/logs.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,7 @@ func (o *LogsClient) getLogsForMode(
}
}

if !follow {
doneChan <- struct{}{}
}
doneChan <- struct{}{}
}

// getPodsForSelector gets pods for the resources matching selector in the namespace; Pods found by this method will be
Expand Down
27 changes: 16 additions & 11 deletions pkg/odo/cli/logs/logs.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"strconv"
"strings"
"sync"
"sync/atomic"

"github.com/redhat-developer/odo/pkg/log"

Expand Down Expand Up @@ -132,9 +133,10 @@ func (o *LogsOptions) Run(_ context.Context) error {
}

uniqueContainerNames := map[string]struct{}{}
wg := sync.WaitGroup{}
var goroutines int64 // keep a track of running goroutines so that we don't exit prematurely
errChan := make(chan error) // errors are put on this channel
var mu sync.Mutex

for {
select {
case containerLogs := <-events.Logs:
Expand All @@ -144,9 +146,11 @@ func (o *LogsOptions) Run(_ context.Context) error {
logs := containerLogs.Logs

if o.follow {
wg.Add(1)
goroutines = atomic.AddInt64(&goroutines, 1)
go func(out io.Writer) {
defer wg.Done()
defer func() {
goroutines = atomic.AddInt64(&goroutines, -1)
}()
err = printLogs(uniqueName, logs, out, colour, &mu)
if err != nil {
errChan <- err
Expand All @@ -164,16 +168,17 @@ func (o *LogsOptions) Run(_ context.Context) error {
case err = <-events.Err:
return err
case <-events.Done:
if len(uniqueContainerNames) == 0 {
// This will be the case when:
// 1. user specifies --dev flag, but the component's running in Deploy mode
// 2. user specified --deploy flag, but the component's running in Dev mode
// 3. user passes no flag, but component is running in neither Dev nor Deploy mode
fmt.Fprintf(o.out, "no containers running in the specified mode for the component %q\n", o.componentName)
if goroutines == 0 {
if len(uniqueContainerNames) == 0 {
// This will be the case when:
// 1. user specifies --dev flag, but the component's running in Deploy mode
// 2. user specified --deploy flag, but the component's running in Dev mode
// 3. user passes no flag, but component is running in neither Dev nor Deploy mode
fmt.Fprintf(o.out, "no containers running in the specified mode for the component %q\n", o.componentName)
}
return nil
}
return nil
}

}
}

Expand Down
15 changes: 15 additions & 0 deletions tests/integration/cmd_logs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,21 @@ var _ = Describe("odo logs command tests", func() {
})
})

When("odo logs is executed for a component that's not running in any modes", func() {
BeforeEach(func() {
helper.CopyExample(filepath.Join("source", "nodejs"), commonVar.Context)
helper.Cmd("odo", "init", "--name", componentName, "--devfile-path", helper.GetExamplePath("source", "devfiles", "nodejs", "devfile-deploy-functional-pods.yaml")).ShouldPass()
Expect(helper.VerifyFileExists(".odo/env/env.yaml")).To(BeFalse())
})
It("should print that no containers are running", func() {
noContainersRunning := "no containers running in the specified mode for the component"
out := helper.Cmd("odo", "logs").ShouldPass().Out()
Expect(out).To(ContainSubstring(noContainersRunning))
out = helper.Cmd("odo", "logs", "--follow").ShouldPass().Out()
Expect(out).To(ContainSubstring(noContainersRunning))
})
})

When("component is created and odo logs is executed", func() {
BeforeEach(func() {
helper.CopyExample(filepath.Join("source", "nodejs"), commonVar.Context)
Expand Down

0 comments on commit d4c9206

Please sign in to comment.