Refactor InterfaceStore and podConfigurator #125

jianjuns · 2019-11-21T08:21:42Z

Add a struct type - podConfigurator - to be the receiver of the Pod
configuration funcs which use the podConfigurator struct fields.

antrea-bot · 2019-11-21T08:21:44Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests. This command can only be run by members of the vmware-tanzu organization
/skip-e2e: to skip e2e tests. This command can only be run by members of the vmware-tanzu organization

antoninbas

LGTM, please merge this soon, it would make my life easier for some of the changes I have to make for Kind support.

antoninbas · 2019-11-22T18:31:37Z

/test-e2e

jianjuns · 2019-11-22T23:09:01Z

LGTM, please merge this soon, it would make my life easier for some of the changes I have to make for Kind support.

I need to do one more small cleanup. Will push a PR soon.

jianjuns · 2019-11-22T23:46:14Z

/test-e2e

pkg/agent/cniserver/pod_configuration.go

jianjuns · 2019-11-24T21:23:46Z

Also moved OVS port external ID generation and parsing from interface_cache to pod_configuration.

jianjuns · 2019-11-24T21:24:01Z

/test-e2e

tnqn · 2019-11-25T03:20:30Z

@jianjuns there is conflict need to be resolved after I checked in GARP fixes.

tnqn · 2019-11-25T05:32:31Z

pkg/agent/cniserver/interface_cache_test.go

@@ -33,7 +34,7 @@ func TestInitCache(t *testing.T) {

 	mockOVSBridgeClient.EXPECT().GetPortList().Return(nil, ovsconfig.NewTransactionError(fmt.Errorf("Failed to list OVS ports"), true))

-	cache := NewInterfaceStore()
+	cache := agent.NewInterfaceStore(ParseOVSPortInterfaceConfig)


@jianjuns interface_cache_test.go is under pkg/agent/cniserver while interface_cache.go is under pkg/agent, is it by intention?

Yes. But the thing is the test actually tests mostly Pod interface configuration, and requires pod_configuration internal funds and structs after my changes. Let me know what you think.

BTW I just rename it to interface_store_test.go as it just references InterfaceStore but interfaceCache which is an internal struct.

In my understanding, it's not common that go source file and its unit test file are not put together, guess people would still question where is interface_store.go after the renaming.

The test code in interface_store_test.go actually have two different scopes: one is for NewInterfaceStore.Initialize and the other is for buildOVSPortExternalIDs.
I think the former should be moved to pkg/agent/interface_cache_test.go and the required constants need to be public or maybe use raw strings as part of test inputs.
The later test case name is a little misleading, it's actually testing buildOVSPortExternalIDs, should we call it TestBuildOVSPortExternalIDs and keep it in pkg/agent/cniserver/pod_configuation_test.go?

The problem is the test for NewInterfaceStore.Initialize - uses pod_configuration internal types to generate OVS ports (and the OVS port logic in my mind should belong to pod_configuration and should not be exposed outside), and requires ParseOVSPortInterfaceConfig from cniserver (and I could not import cniserver due to import cycle).

Let me know if you have suggestions on how to resolve the above.

Maybe InterfaceStore.Initialize mixes too much business code, it needs to hold ovsBridgeClient and know how to list Port, and hold containerInterfaceParser to know how to parse OVSPort while its other methods are all pure cache access operations taking the constructed InterfaceConfig.
As a cache struct, maybe we can just let Initialize take the parsed InterfaceConfigs like AddInterface, the listing and parsing can be done in setupOVSBridge before calling InterfaceStore.Initialize, this could make the cache face InterfaceConfig only, getting rid of OVS list and parse logic , also make unit test easier as we don't need to mock ovsBridgeClient any more.

I could change InterfaceStore.Initialize (or just move the func to agent.go), but then the question is still what we want to test in TestInitStore? You just want to test inserting a set of InterfaceConfigs to the store and checking they are added?

Isn't a good thing that the cache can be got rid of those dependencies and be so clean that we even think UT is not necessary? If we want to keep the original test code, they could be moved to pkg/agent/agent_test.go by testing setupOVSBridge or another method extracted from it.

Sure. I agree it is cleaner to move OVS operations to agent.go (feel better to do another PR though if no other reasons for that as my current one adds more and more changes).

My question is on the test - do we want to keep a test that generates OVS port data from OVS mock, and initializes the InterfaceStore from that - if so the test might need to be in the cniserver package as it uses pod_configuration funcs and types (unless you have a way to resolve this esp. the import cycle). Or we just write a test only for inserting InterfaceConfig to the store and checking they are indeed inserted? Or maybe you want both?

I think the problem is that currently pkg/agent mixes too many stuff (there are types needed by itself and its sub-package, which makes it can't even depend on its sub-package, and its sub-package depends on agent only because of these types).
Maybe we could move types like NodeConfig, Gateway, OVSPortConfig to pkg/agent/types as we already have it.
Also move interface_cache to separate package pkg/agent/cache as it's depended by both agent and cniserver. Then agent can depend on cniserver as it's supposed to do.
The layout would be:

type ^ | cache ^ ^ | | cniserver <- agent

I created another commit to refactor InterfaceStore and types. Please check.

pkg/agent/cniserver/pod_configuration.go

pkg/agent/cniserver/server_test.go

tnqn

LGTM, just a comment about how to organize the test code in Go style. Let me if it makes sense to you.

jianjuns · 2019-11-25T18:36:05Z

/test-e2e

Add a struct type - podConfigurator - to be the receiver of the Pod configuration funcs which use the podConfigurator struct fields.

Move InterfaceStore to a separate package. This could avoid sub-packages of agent depends on the agent package which could lead to import cycle. For the same reason, move NodeConfig and GatewayConfig from agent to agent/types. Move OVS port data initialization to agent.Initializer. Move OVS port external ID generation and parsing to podConfigurator. Change "MTU" in structs and func arguments to "mtu".

jianjuns · 2019-11-27T04:54:57Z

/test-e2e

tnqn

LGTM

Before aggregating, validate data records if all the records have proper values after decoding.

jianjuns requested a review from wenyingd November 21, 2019 08:21

vmwclabot added the cla-not-required label Nov 21, 2019

jianjuns force-pushed the pod-config branch from 5623884 to 4ba3061 Compare November 22, 2019 06:55

antoninbas previously approved these changes Nov 22, 2019

View reviewed changes

jianjuns dismissed antoninbas’s stale review via 944c9db November 22, 2019 23:11

jianjuns force-pushed the pod-config branch from 4ba3061 to 944c9db Compare November 22, 2019 23:11

tnqn reviewed Nov 23, 2019

View reviewed changes

pkg/agent/cniserver/pod_configuration.go Outdated Show resolved Hide resolved

pkg/agent/cniserver/pod_configuration.go Outdated Show resolved Hide resolved

jianjuns force-pushed the pod-config branch 2 times, most recently from b4a39d4 to 79ac262 Compare November 24, 2019 21:20

jianjuns force-pushed the pod-config branch 2 times, most recently from 6dd006a to 3ab5e12 Compare November 25, 2019 01:03

jianjuns force-pushed the pod-config branch from 3ab5e12 to a50d95f Compare November 25, 2019 05:04

tnqn reviewed Nov 25, 2019

View reviewed changes

jianjuns force-pushed the pod-config branch from a50d95f to cfd80a6 Compare November 25, 2019 05:56

tnqn reviewed Nov 25, 2019

View reviewed changes

pkg/agent/cniserver/pod_configuration.go Outdated Show resolved Hide resolved

pkg/agent/cniserver/server_test.go Outdated Show resolved Hide resolved

pkg/agent/cniserver/server_test.go Outdated Show resolved Hide resolved

jianjuns force-pushed the pod-config branch from cfd80a6 to 48507dd Compare November 25, 2019 06:06

tnqn reviewed Nov 25, 2019

View reviewed changes

antoninbas mentioned this pull request Nov 25, 2019

Fix Kind support (Linux hosts only) #137

Merged

jianjuns force-pushed the pod-config branch 3 times, most recently from a859156 to 9ceaf9e Compare November 27, 2019 02:57

jianjuns changed the title ~~Make Pod configuration funcs methods of podConfigurator~~ Refactor InterfaceStore and podConfigurator Nov 27, 2019

jianjuns force-pushed the pod-config branch from 9ceaf9e to 5af186f Compare November 27, 2019 03:04

jianjuns added 2 commits November 26, 2019 20:40

Make Pod configuration funcs methods of podConfigurator

ce33091

Add a struct type - podConfigurator - to be the receiver of the Pod configuration funcs which use the podConfigurator struct fields.

jianjuns force-pushed the pod-config branch from 5af186f to 92b2f08 Compare November 27, 2019 04:44

tnqn approved these changes Nov 27, 2019

View reviewed changes

jianjuns merged commit e34cc0e into master Nov 27, 2019

jianjuns deleted the pod-config branch November 27, 2019 18:28

zyiou pushed a commit to zyiou/antrea that referenced this pull request Jul 2, 2021

Validating data records in aggregation process (antrea-io#125)

29807f8

Before aggregating, validate data records if all the records have proper values after decoding.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor InterfaceStore and podConfigurator #125

Refactor InterfaceStore and podConfigurator #125

jianjuns commented Nov 21, 2019

antrea-bot commented Nov 21, 2019

antoninbas left a comment

antoninbas commented Nov 22, 2019

jianjuns commented Nov 22, 2019

jianjuns commented Nov 22, 2019

jianjuns commented Nov 24, 2019

jianjuns commented Nov 24, 2019

tnqn commented Nov 25, 2019

tnqn Nov 25, 2019

jianjuns Nov 25, 2019

tnqn Nov 25, 2019

jianjuns Nov 25, 2019

tnqn Nov 25, 2019

jianjuns Nov 26, 2019

tnqn Nov 26, 2019

jianjuns Nov 26, 2019

tnqn Nov 26, 2019

jianjuns Nov 27, 2019

tnqn left a comment

jianjuns commented Nov 25, 2019

jianjuns commented Nov 27, 2019

tnqn left a comment

Refactor InterfaceStore and podConfigurator #125

Refactor InterfaceStore and podConfigurator #125

Conversation

jianjuns commented Nov 21, 2019

antrea-bot commented Nov 21, 2019

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Nov 22, 2019

jianjuns commented Nov 22, 2019

jianjuns commented Nov 22, 2019

jianjuns commented Nov 24, 2019

jianjuns commented Nov 24, 2019

tnqn commented Nov 25, 2019

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

tnqn left a comment

Choose a reason for hiding this comment

jianjuns commented Nov 25, 2019

jianjuns commented Nov 27, 2019

tnqn left a comment

Choose a reason for hiding this comment