Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export node latency query api define. #6392

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

IRONICBo
Copy link
Contributor

@IRONICBo IRONICBo commented Jun 3, 2024

We have collected all agents latencies in PR: #6120, then we need to export data to api server and report it to antrea controller.

In this PR, we try to define a REST api in antrea, and we will separate the REST api inner in other PR.

The commits base on PR #6120.

Design Doc: https://docs.google.com/document/d/1EdKJ8iQ3KwVBQAHaPisqHP7cgpq4RW8mD9KPWtYETbE/edit

@IRONICBo IRONICBo force-pushed the feat/node-latency-query-api branch from 487b01b to fc9b23c Compare June 3, 2024 17:52
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve all build issues and make sure make golangci succeeds locally.

multicluster/hack/update-codegen-dockerized.sh Outdated Show resolved Hide resolved
build/charts/antrea/templates/agent/clusterrole.yaml Outdated Show resolved Hide resolved
build/charts/antrea/templates/agent/clusterrole.yaml Outdated Show resolved Hide resolved
build/charts/antrea/templates/antctl/clusterrole.yaml Outdated Show resolved Hide resolved
build/charts/antrea/templates/antctl/clusterrole.yaml Outdated Show resolved Hide resolved
pkg/apiserver/registry/stats/nodelatencystat/rest.go Outdated Show resolved Hide resolved
pkg/apiserver/registry/stats/nodelatencystat/rest.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apiserver/registry/stats/networkpolicystats/rest.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Dyanngg Dyanngg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be quite some import errors. Have you rebased the patch onto main?

pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still build issues that need to be addressed. It makes the PR harder to review. Once again, please make sure you can run make golangci successfully before pushing.

build/yamls/antrea.yml Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I started a new round of review but it looks like some key comments haven't been addressed yet, so I will hold off on reviewing again.

pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
@IRONICBo IRONICBo requested a review from antoninbas June 11, 2024 02:13
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a couple of small comments remaining from me.
@Dyanngg @tnqn do you want to take a look?

pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/apis/stats/v1alpha1/types.go Outdated Show resolved Hide resolved
@tnqn
Copy link
Member

tnqn commented Jun 12, 2024

Only a couple of small comments remaining from me.
@Dyanngg @tnqn do you want to take a look?

I could take a look tomorrow, could you wait for another day before merging it?

@antoninbas
Copy link
Contributor

Only a couple of small comments remaining from me.
@Dyanngg @tnqn do you want to take a look?

I could take a look tomorrow, could you wait for another day before merging it?

Yes, no problem

antoninbas
antoninbas previously approved these changes Jun 12, 2024
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Show resolved Hide resolved
pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved
// +genclient
// +genclient:nonNamespaced
// +resourceName=nodelatencystats
// +genclient:onlyVerbs=create,delete,get,list,watch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't support delete and watch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked @IRONICBo to include these verbs. I think we can add their implementation at the same time as the implementation for create, get and list, in the follow-up PR. But maybe we should include the 2 missing stubs in pkg/apiserver/registry/stats/nodelatencystats/rest.go as part of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wrote the comment because the server only supports create, get, and list, so had the impression they are the methods we are going to implement. Besides, do we need clients to delete a stats? and I remember supporting standard watch is somewhat difficult due to the resource version stuff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, do we need clients to delete a stats?

I think we may want Agents to delete their resource if the feature gets disabled. So IMO it makes sense to have delete.

I remember supporting standard watch is somewhat difficult due to the resource version stuff.

That makes sense. We should remove watch then.

@IRONICBo do you think you can:

  1. remove watch from the list of verbs, and regenerate the code
  2. add a stub Delete method in the REST server

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it sounds good to me.

pkg/apiserver/apiserver.go Outdated Show resolved Hide resolved
@antoninbas
Copy link
Contributor

@IRONICBo the PR will also need to be rebased, probably because I made some small changes to the monitor code when I added unit tests

@IRONICBo
Copy link
Contributor Author

@IRONICBo the PR will also need to be rebased, probably because I made some small changes to the monitor code when I added unit tests

Ok, I have done.

@IRONICBo IRONICBo force-pushed the feat/node-latency-query-api branch 2 times, most recently from bb2924d to e9d8291 Compare June 15, 2024 03:21
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but pkg/agent/monitortool/monitor_test.go needs update

Signed-off-by: Asklv <boironic@gmail.com>
Signed-off-by: Asklv <boironic@gmail.com>
Signed-off-by: Asklv <boironic@gmail.com>
Signed-off-by: Asklv <boironic@gmail.com>
Signed-off-by: Asklv <boironic@gmail.com>
@IRONICBo IRONICBo force-pushed the feat/node-latency-query-api branch from e9d8291 to eef3783 Compare June 15, 2024 03:40
Signed-off-by: Asklv <boironic@gmail.com>
@IRONICBo IRONICBo force-pushed the feat/node-latency-query-api branch from eef3783 to 4eae08c Compare June 15, 2024 03:46
@IRONICBo
Copy link
Contributor Author

LGTM but pkg/agent/monitortool/monitor_test.go needs update

Ok, I have add NewNodeLatencyMonitor params in monitor_test.go.

@@ -160,7 +162,8 @@ func newTestMonitor(
crdClientset := fakeversioned.NewSimpleClientset(crdObjects...)
crdInformerFactory := crdinformers.NewSharedInformerFactory(crdClientset, 0)
nlmInformer := crdInformerFactory.Crd().V1alpha1().NodeLatencyMonitors()
m := NewNodeLatencyMonitor(nodeInformer, nlmInformer, nodeConfig, trafficEncapMode)
antreaClientProvider, _ := client.NewAntreaClientProvider(componentbaseconfig.ClientConnectionConfiguration{}, clientset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work. You need a fake provider that will return the fake client when called. You can check out the test code in pkg/agent/controller/egress/egress_controller_test.go

Copy link
Contributor Author

@IRONICBo IRONICBo Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas Ok, I configured gomock to generate the fake client and return the clientset using EXCEPT.

But in monitor.go->report() I need pkg/client/clientset/versioned/fake/clientset_generated.go, I need to use antrea's fakeclientset method to get the StatsV1alpha1().

Do I need to replace the clientset in k8s.io/client-go/kubernetes/fake used in test code?

clientset := fake.NewSimpleClientset(objects...)
statsClientset := statsfake.NewSimpleClientset(objects...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit overkill to generate a gomock implementation for that interface. You can just use a few lines to achieve it:

type antreaClientGetter struct {
	clientset versioned.Interface
}

func (g *antreaClientGetter) GetAntreaClient() (versioned.Interface, error) {
	return g.clientset, nil
}

antreaClientProvider := &antreaClientGetter{fakeversioned.NewSimpleClientset(objects...)}
m := NewNodeLatencyMonitor(antreaClientProvider, nodeInformer, nlmInformer, nodeConfig, trafficEncapMode)

You don't need to change other clientsets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's helpful, I've updated it.

@IRONICBo IRONICBo force-pushed the feat/node-latency-query-api branch 2 times, most recently from ec414a1 to 1f69704 Compare June 15, 2024 16:59
Signed-off-by: Asklv <boironic@gmail.com>
@IRONICBo IRONICBo force-pushed the feat/node-latency-query-api branch from 1f69704 to 101d7bb Compare June 16, 2024 03:01
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@IRONICBo do you have time to work on the next PR (REST service implementation)?

@antoninbas
Copy link
Contributor

/test-all

@IRONICBo
Copy link
Contributor Author

do you have time to work on the next PR (REST service implementation)?

Yes, I'll go ahead and create the next part after this PR is merged.

@antoninbas antoninbas merged commit 25899c3 into antrea-io:main Jun 18, 2024
51 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants