Fix nil pointer error when collecting agent supportbundle fails #4306

tnqn · 2022-10-17T18:01:20Z

The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection.

#2598 tried to fix it but did the opposite.

Signed-off-by: Quan Tian qtian@vmware.com

Based on #4304 and #4295's testing code.

antoninbas

LGTM, I only reviewed the last commit

antoninbas · 2022-10-17T18:04:30Z

pkg/agent/apiserver/apiserver.go

-	supportBundleStorage := supportbundle.NewAgentStorage(ovsctl.NewClient(aq.GetNodeConfig().OVSBridge), aq, npq, v4Enabled, v6Enabled)
+	iptables, err := iptables.New(v4Enabled, v6Enabled)
+	if err != nil {
+		return fmt.Errorf("failed to new iptables client: %v", err)


maybe "create" or "instantiate" since "new" is not a verb

will update

antoninbas · 2022-10-17T18:06:02Z

pkg/apiserver/registry/system/supportbundle/rest_test.go

+	require.NoError(t, err)
+
+	var collectedBundle *system.SupportBundle
+	assert.Eventually(t, func() bool {


this is neat. Is this new?
should we switch to assert.Eventually for all new unit tests instead of wait.Poll?

Added in 1.4.0. Yes, I think it can replace wait.Poll in tests.

antoninbas · 2022-10-17T18:12:15Z

pkg/apiserver/registry/system/supportbundle/rest_test.go

+	fakeAgentQuerier.EXPECT().GetAgentInfo(&clusterinformationv1beta1.AntreaAgentInfo{}, false)
+	fakeIPTables.EXPECT().Save()
+	storage := NewAgentStorage(fakeOVSCtl, fakeAgentQuerier, fakeNetworkPolicyQuerier, fakeIPTables)
+	_, err := storage.SupportBundle.Create(context.TODO(), &system.SupportBundle{


nit: I feel like it makes a bit more sense for unit test to define ctx := context.Background() at the beginning of the test case and use that context for every function that requires it. I feel like Background is more logical than TODO for tests, given that we don't intend to ever replace this context.TODO() with something else.

Some tests for rest API customize context to set namespace:

antrea/pkg/apiserver/registry/stats/networkpolicystats/rest_test.go

Line 292 in 5e96c8c

ctx := request.WithNamespace(context.TODO(), tt.npNamespace)

Some tests customize context to get a chance to cancel it:

antrea/pkg/apiserver/registry/system/supportbundle/rest_test.go

Line 70 in 5e96c8c

ctx, cancelFunc := context.WithCancel(context.Background())

You mean creating a global background and use it for all tests, and create child context from it if needed?

not global, but for a given test case if that test case requires a context object:

func MyTest(t *testing.T) { ctx := context.Bakground() obj1.req(ctx, ...) obj2.req(ctx, ...) ctx, cancelFunc := context.WithCancel(ctx) defer cancelFunc() obj3.req(ctx, ...) }

updated, thanks

codecov · 2022-10-17T18:57:20Z

Codecov Report

Merging #4306 (5e96c8c) into main (c491ead) will decrease coverage by 0.70%.
The diff coverage is n/a.

❗ Current head 5e96c8c differs from pull request most recent head b9d47cd. Consider uploading reports for the commit b9d47cd to get more accurate results

@@            Coverage Diff             @@
##             main    #4306      +/-   ##
==========================================
- Coverage   66.28%   65.57%   -0.71%     
==========================================
  Files         369      364       -5     
  Lines       53270    52925     -345     
==========================================
- Hits        35309    34706     -603     
- Misses      15369    15652     +283     
+ Partials     2592     2567      -25

Flag	Coverage Δ
integration-tests	`34.40% <0.00%> (-0.16%)`	⬇️
kind-e2e-tests	`48.86% <0.00%> (+0.39%)`	⬆️
unit-tests	`46.70% <0.00%> (-2.73%)`	⬇️

Impacted Files	Coverage Δ
...kg/apiserver/registry/stats/multicastgroup/rest.go	`11.59% <0.00%> (-79.32%)`	⬇️
...piserver/registry/stats/networkpolicystats/rest.go	`57.81% <0.00%> (-37.02%)`	⬇️
...stry/stats/antreaclusternetworkpolicystats/rest.go	`63.07% <0.00%> (-36.93%)`	⬇️
...gistry/networkpolicy/networkpolicy/subresources.go	`47.05% <0.00%> (-35.30%)`	⬇️
...er/registry/stats/antreanetworkpolicystats/rest.go	`61.42% <0.00%> (-33.89%)`	⬇️
pkg/agent/flowexporter/exporter/certificate.go	`27.77% <0.00%> (-22.23%)`	⬇️
...piserver/registry/controlplane/egressgroup/rest.go	`66.66% <0.00%> (-20.52%)`	⬇️
...server/registry/networkpolicy/addressgroup/rest.go	`66.66% <0.00%> (-20.52%)`	⬇️
...erver/registry/networkpolicy/networkpolicy/rest.go	`66.66% <0.00%> (-20.52%)`	⬇️
...rver/registry/networkpolicy/appliedtogroup/rest.go	`66.66% <0.00%> (-20.52%)`	⬇️
... and 55 more

tnqn · 2022-10-19T16:54:03Z

/test-all

antoninbas

LGTM

The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. antrea-io#2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn · 2022-10-20T06:21:37Z

/test-all

wenyingd

LGTM

…ea-io#4306) The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. antrea-io#2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Oct 17, 2022

tnqn requested review from antoninbas and liu4480 October 17, 2022 18:01

antoninbas previously approved these changes Oct 17, 2022

View reviewed changes

tnqn dismissed antoninbas’s stale review via 7ae265d October 19, 2022 16:53

tnqn force-pushed the fix-agent-supportbundle branch from 9a73afc to 7ae265d Compare October 19, 2022 16:53

tnqn requested a review from antoninbas October 19, 2022 16:54

antoninbas previously approved these changes Oct 19, 2022

View reviewed changes

tnqn dismissed antoninbas’s stale review via b9d47cd October 20, 2022 05:59

tnqn force-pushed the fix-agent-supportbundle branch from 7ae265d to b9d47cd Compare October 20, 2022 05:59

tnqn added this to the Antrea v1.9 release milestone Oct 20, 2022

tnqn requested review from wenyingd and hongliangl October 20, 2022 07:14

wenyingd approved these changes Oct 20, 2022

View reviewed changes

tnqn merged commit 5e722c2 into antrea-io:main Oct 20, 2022

tnqn deleted the fix-agent-supportbundle branch October 20, 2022 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nil pointer error when collecting agent supportbundle fails #4306

Fix nil pointer error when collecting agent supportbundle fails #4306

tnqn commented Oct 17, 2022

antoninbas left a comment

antoninbas Oct 17, 2022

tnqn Oct 18, 2022

antoninbas Oct 17, 2022

tnqn Oct 18, 2022

antoninbas Oct 17, 2022

tnqn Oct 18, 2022

antoninbas Oct 18, 2022

tnqn Oct 19, 2022

tnqn Oct 19, 2022

codecov bot commented Oct 17, 2022 •

edited

Loading

tnqn commented Oct 19, 2022

antoninbas left a comment

tnqn commented Oct 20, 2022

wenyingd left a comment

Fix nil pointer error when collecting agent supportbundle fails #4306

Fix nil pointer error when collecting agent supportbundle fails #4306

Conversation

tnqn commented Oct 17, 2022

antoninbas 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

codecov bot commented Oct 17, 2022 • edited Loading

Codecov Report

tnqn commented Oct 19, 2022

antoninbas left a comment

Choose a reason for hiding this comment

tnqn commented Oct 20, 2022

wenyingd left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2022 •

edited

Loading