[bump_v18.09] Fix nil pointer dereference in node allocation, linter fixes #2769
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of running many small source code checkers and linters one by one, let's use gometalinter that runs them all in parallel. While at it, remove the individual make targets (fmt, vet, lint, ineffassign, and misspell) and replace with a single check target. One thing it provides is faster source validation. BEFORE: real 0m24.025s user 1m2.646s sys 0m3.860s (note these timings are without building binaries, which for some reason was a dependency of the vet target) AFTER: real 0m6.330s user 0m20.255s sys 0m1.019s In addition to this, it is now way easier to add/remove the checks, as well as to filter out some errors from linters that we consider false positives. [v2: add 2m deadline] [v3: use gometalinter --install; move configuration to .json] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Since Go 1.7, context is a standard package. Since Go 1.9, everything
that is provided by "x/net/context" is a couple of type aliases to
types in "context".
2. While at it, fix the order of imports (done by goimports and some shell
trickery, see below).
3. Also, when the standard context package is used, the errors of
not calling cancel() are detected/reported by go vet:
> the cancel function returned by context.WithTimeout should be called,
> not discarded, to avoid a context leak (vet)
This essentially asks to call cancel() as otherwise a stray timer
is leaked. Fix a few such issues, mostly in _test.go files.
4. Since in func (*session).start (see agent/session.go) we deliberately
do not want to cancel the context, govet gives us a couple of errors:
> agent/session.go:140: the cancelSession function is not used
> on all paths (possible context leak)
> agent/session.go:163: this return statement may be reached
> without using the cancelSession var defined on line 140
To silence these, use `// nolint: vet` mark in a couple of places
(this is a feature of gometalinter).
Oh, the conversion (items 1 and 2 above) was performed by this
shell script:
```sh
FILES=$*
test -z "$FILES" && FILES=$(git ls-files \*.go | grep -v ^vendor/ | grep -v .pb.go$)
for f in $FILES; do
sed -i 's|"golang.org/x/net/context"|"context"|' $f
goimports -w $f
for i in 1 2; do
awk '/^$/ {e=1; next;}
/\t"context"$/ {e=0;}
{if (e) {print ""; e=0}; print;}' < $f > $f.new && mv $f.new $f
goimports -w $f
done
echo -n .
done
echo
```
Multiple `goimports` calls and some awk trickery is needed to iron out
incorrect formatting (excessive empty lines) from the import statements.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
...and fix the following initial bunch of warnings: agent/session.go:18:1:warning: errSessionDisconnect is unused (deadcode) agent/errors.go:7:1:warning: errTaskNoController is unused (deadcode) agent/errors.go:7:1:warning: errTaskStatusUpdateNoChange is unused (deadcode) agent/errors.go:7:1:warning: errTaskNotAssigned is unused (deadcode) agent/errors.go:7:1:warning: errTaskInvalid is unused (deadcode) ca/transport.go:21:1:warning: timeoutError is unused (deadcode) ca/config.go:29:1:warning: nodeCSRFilename is unused (deadcode) cmd/swarmctl/node/common.go:58:1:warning: changeNodeMembership is unused (deadcode) integration/cluster.go:44:1:warning: newTestCluster is unused (deadcode) manager/orchestrator/global/global.go:590:1:warning: isTaskCompleted is unused (deadcode) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... and fix some warnings from unconvert: ca/config.go:627:58:warning: unnecessary conversion (unconvert) manager/allocator/cnmallocator/portallocator.go:410:38:warning: unnecessary conversion (unconvert) manager/allocator/cnmallocator/portallocator.go:415:50:warning: unnecessary conversion (unconvert) manager/controlapi/service.go:200:47:warning: unnecessary conversion (unconvert) manager/controlapi/service.go:210:45:warning: unnecessary conversion (unconvert) manager/controlapi/service.go:220:35:warning: unnecessary conversion (unconvert) manager/dispatcher/nodes.go:159:33:warning: unnecessary conversion (unconvert) manager/state/store/memory.go:692:91:warning: unnecessary conversion (unconvert) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
... and fix warnings reported by it:
agent/exec/dockerapi/adapter.go:147:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
agent/testutils/fakes.go:143:2:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple)
agent/testutils/fakes.go:151:2:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple)
ca/certificates_test.go:708:2:warning: should use for range instead of for { select {} } (S1000) (gosimple)
ca/config.go:630:12:warning: should use time.Until instead of t.Sub(time.Now()) (S1024) (gosimple)
ca/config_test.go:790:3:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple)
ca/external_test.go:116:3:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple)
cmd/swarm-bench/collector.go:26:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
cmd/swarmctl/node/common.go:51:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
cmd/swarmctl/node/common.go:89:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
cmd/swarmctl/node/common.go:172:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
ioutils/ioutils_test.go:28:5:warning: should use !bytes.Equal(actual, expected) instead (S1004) (gosimple)
manager/allocator/cnmallocator/networkallocator.go:818:3:warning: should merge variable declaration with assignment on next line (S1021) (gosimple)
manager/constraint/constraint.go:59:7:warning: should omit comparison to bool constant, can be simplified to !matched (S1002) (gosimple)
manager/constraint/constraint.go:67:7:warning: should omit comparison to bool constant, can be simplified to !matched (S1002) (gosimple)
manager/dispatcher/dispatcher.go:1095:5:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
manager/dispatcher/dispatcher.go:1095:5:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
manager/dispatcher/dispatcher.go:1095:5:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
manager/dispatcher/dispatcher_test.go:2090:2:warning: redundant return statement (S1023) (gosimple)
manager/manager.go:1005:4:warning: should replace loop with m.config.NetworkConfig.DefaultAddrPool = append(m.config.NetworkConfig.DefaultAddrPool, cluster.DefaultAddressPool...) (S1011) (gosimple)
manager/metrics/collector.go:191:2:warning: redundant return statement (S1023) (gosimple)
manager/metrics/collector.go:222:2:warning: redundant return statement (S1023) (gosimple)
manager/orchestrator/replicated/update_test.go:53:3:warning: should use for range instead of for { select {} } (S1000) (gosimple)
manager/orchestrator/taskinit/init.go:83:32:warning: should use time.Until instead of t.Sub(time.Now()) (S1024) (gosimple)
manager/state/raft/raft.go:1185:2:warning: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008) (gosimple)
manager/state/raft/raft.go:1594:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
node/node.go:1209:2:warning: redundant return statement (S1023) (gosimple)
node/node.go:1219:2:warning: redundant return statement (S1023) (gosimple)
watch/sinks_test.go:42:2:warning: should merge variable declaration with assignment on next line (S1021) (gosimple)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When the network allocator starts, it performs two passes of allocation. The first, with existingAddressesOnly set to "true", simply re-allocates any already reserved addresses, which make the local driver state consistent with the state in swarmkit's object store. The second pass then performs any outstanding new allocations, from when the allocator last stopped. Since moby#2725, nodes only have attachments allocated for them if they have a task currently scheduled which requires those networks. This happens after a task is allocated and scheduled. Before this change, it was possible that, if a Task was correctly allocated, but the allocator stopped before the Node was also allocated, during the restore phase, an empty api.NetworkAttachment object was added to the Node's attachments. Then, in the new allocations phase, when trying to process all attachments, we were unconditionally looking at the NetworkAttachment object's Network field, which was nil. This caused a segfault and crash. With this change, we no longer add these errant NetworkAttachment objects to nodes. Signed-off-by: Drew Erny <drew.erny@docker.com>
wk8
approved these changes
Oct 19, 2018
While at it, let's simplify installation
Use unzip options to unpack directly to /usr/local/{include,bin},
to avoid unnecessary I/O.
Unfortunately unzip does not support unpacking from a pipe (due
to a limitation of .zip format -- the index is at EOF) so we still
have to save the .zip to disk, read it back, and remove. If they
could only provide tarballs... *sigh*
[v2: update circleci config as well]
[v3: add chmod a+r to circleci]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Codecov Report
@@ Coverage Diff @@
## bump_v18.09 #2769 +/- ##
===============================================
- Coverage 61.85% 61.83% -0.03%
===============================================
Files 134 134
Lines 21876 21859 -17
===============================================
- Hits 13532 13517 -15
+ Misses 6882 6880 -2
Partials 1462 1462 |
|
looks like you forgot to use the |
looks like I did. My bad @thaJeztah |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backports #2764 & #2750 to the newly-named bump_v18.09 branch.