-
Notifications
You must be signed in to change notification settings - Fork 111
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
chore: improving e2e testing latency #1760
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1760 +/- ##
==========================================
- Coverage 60.94% 57.23% -3.71%
==========================================
Files 232 218 -14
Lines 20014 17581 -2433
==========================================
- Hits 12197 10063 -2134
+ Misses 6959 6664 -295
+ Partials 858 854 -4 ☔ View full report in Codecov by Sentry. |
@samhith-kakarla - you might need to look into the original PR, we want to revert some changes over there:
|
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
d2ddc8c
to
4ef5619
Compare
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
@@ -171,9 +149,6 @@ cleanup-e2e: | |||
# To run just one of the e2e tests by name (i.e. 'make TestCreateSimplePipeline'): | |||
Test%: | |||
$(MAKE) cleanup-e2e | |||
$(MAKE) deploy-nats | |||
$(MAKE) deploy-kafka | |||
$(MAKE) deploy-redis | |||
$(MAKE) image e2eapi-image | |||
kubectl -n numaflow-system delete po -lapp.kubernetes.io/component=controller-manager,app.kubernetes.io/part-of=numaflow | |||
kubectl -n numaflow-system delete po e2e-api-pod --ignore-not-found=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing go generate
?
test/jetstream-e2e/jetstream_test.go
Outdated
@@ -25,6 +25,10 @@ import ( | |||
"github.com/stretchr/testify/suite" | |||
) | |||
|
|||
//go:generate kubectl -n numaflow-system delete statefulset nats --ignore-not-found=true | |||
//go:generate kubectl apply -k ../../config/apps/nats -n numaflow-system | |||
//go:generate kubectl apply -f testdata/nats-auth-fake-token.yaml -n numaflow-system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is how the resources are getting deployed when go generate is called on the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wasn't this needed before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because i think the commit where this jetstream test was added(#1723) came after the e2e re-factor pr (#1725) so at this point all the resources were getting deployed in the make file whenever any test was run. But now since we are no longer deploying all the resources in the make file, need this line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it, shouldn't this line be covered in kubectl apply -k ../../config/apps/nats -n numaflow-system
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you talking about this line //go:generate kubectl apply -f testdata/nats-auth-fake-token.yaml -n numaflow-system
if so i have removed it: i have only kept : //go:generatekubectl -n numaflow-system delete statefulset nats --ignore-not-found=true //go:generate kubectl apply -k ../../config/apps/nats -n numaflow-system
@@ -0,0 +1,9 @@ | |||
|
|||
apiVersion: v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this file used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is getting used in the following line: /go:generate kubectl apply -f testdata/nats-auth-fake-token.yaml -n numaflow-system
test/nats-e2e/nats_test.go
Outdated
@@ -27,6 +27,10 @@ import ( | |||
. "github.com/numaproj/numaflow/test/fixtures" | |||
) | |||
|
|||
//go:generate kubectl -n numaflow-system delete statefulset nats --ignore-not-found=true | |||
//go:generate kubectl apply -k ../../config/apps/nats -n numaflow-system | |||
//go:generate kubectl apply -f testdata/nats-auth-fake-token.yaml -n numaflow-system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is how the resources are getting deployed when go generate is called on the file
@@ -0,0 +1,9 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file in-use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is used here : //go:generate kubectl apply -f testdata/nats-auth-fake-token.yaml -n numaflow-system
test/e2e-api/kafka.go
Outdated
func (kh *KafkaController) CreateTopicHandler(w http.ResponseWriter, r *http.Request) { | ||
|
||
m.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstract this as a function with lock, returning a client.
test/e2e-api/http.go
Outdated
@@ -42,7 +42,20 @@ func NewHttpController() *HttpController { | |||
} | |||
} | |||
|
|||
func NewHttpController() *HttpController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make change to this file? Initialization with an HTTP client should be okay, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, we do not need to change this file. i will reverse the changes
test/e2e-api/nats.go
Outdated
} | ||
} | ||
|
||
func (n *NatsController) PumpSubject(w http.ResponseWriter, r *http.Request) { | ||
m.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, abstract this as a function with lock, returning a client.
test/e2e-api/redis.go
Outdated
func (h *RedisController) GetMsgCountContains(w http.ResponseWriter, r *http.Request) { | ||
m.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Makefile
Outdated
$(MAKE) image e2eapi-image | ||
kubectl -n numaflow-system delete po -lapp.kubernetes.io/component=controller-manager,app.kubernetes.io/part-of=numaflow | ||
kubectl -n numaflow-system delete po e2e-api-pod --ignore-not-found=true | ||
go generate $(shell find ./test/kafka-e2e -name '*.go') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't use go generate $(shell find ./test/$* -name '*.go')
as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think because that portion of the makefile is To run just one of the e2e tests by name (i.e. 'make TestCreateSimplePipeline'):
so i dont think go generate $(shell find ./test/$* -name '*.go')
works as this requires test file name as input not a test name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try to do a selective go generate here as well by trying to extract the file name, maybe this can help
shell grep $(*) -R ./test | head -1 | awk -F\/ '{print $$3}'
test/e2e-api/nats.go
Outdated
func NewNatsController(url string, token string) *NatsController { | ||
opts := []natslib.Option{natslib.Token(token)} | ||
nc, err := natslib.Connect(url, opts...) | ||
func initNewNats(n *NatsController) *NatsController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it returns a * NatsController
?
test/e2e-api/kafka.go
Outdated
func NewKafkaController() *KafkaController { | ||
// initialize Kafka handlers | ||
var brokers = []string{bootstrapServers} | ||
func initNewKafka(n *KafkaController) *KafkaController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it returns *KafkaController
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i felt it was just cleaner to do that. that way when i call the function i can just re-assign the controller like this kh = initNewKafka(kh)
. otherwise i would need to return producer, consumer and adminClient individually and re-assign those fields when the init function is called right ?
test/e2e-api/redis.go
Outdated
func NewRedisController() *RedisController { | ||
// When we use this API to validate e2e test result, we always assume a redis UDSink is used | ||
// to persist data to a redis instance listening on port 6379. | ||
func initNewReddis(n *RedisController) *RedisController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above.
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
test/e2e-api/kafka.go
Outdated
config.Producer.Partitioner = sarama.NewManualPartitioner | ||
|
||
producer, err := sarama.NewSyncProducer(brokers, config) | ||
adminClient, err := sarama.NewClusterAdmin(n.brokers, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can directly assign to n.adminClient
and return the same
test/e2e-api/kafka.go
Outdated
config.Producer.Return.Successes = true | ||
config.Producer.Partitioner = sarama.NewManualPartitioner | ||
|
||
consumer, err := sarama.NewConsumer(n.brokers, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can directly assign, same as above.
test/e2e-api/kafka.go
Outdated
config.Producer.Return.Successes = true | ||
config.Producer.Partitioner = sarama.NewManualPartitioner | ||
|
||
producer, err := sarama.NewSyncProducer(n.brokers, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can directly assign, same as above.
test/e2e-api/kafka.go
Outdated
|
||
} | ||
|
||
// func initNewKafka(n *KafkaController) *KafkaController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this
test/e2e-api/kafka.go
Outdated
// } | ||
|
||
func NewKafkaController() *KafkaController { | ||
// initialize Kafka handlers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clean up any extra commented lines
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
adminClient: adminClient, | ||
producer: producer, | ||
consumer: consumer, | ||
adminClient: nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here indicating why these are set to nil initially.
test/e2e-api/kafka.go
Outdated
func NewKafkaController() *KafkaController { | ||
// initialize Kafka handlers | ||
var brokers = []string{bootstrapServers} | ||
// getter method for lazy loading. create and return the admin client only when required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getKafkaClient is used for getting a Kafka admin client for the given
controller config. It is implemented with a lazy loading mechanism
1) A new client created only for the first request
2) Returning the current client if it exists
@samhith-kakarla Please add the latency improvement stats that you collected here |
test/e2e-api/kafka.go
Outdated
n.mLock.Lock() | ||
defer n.mLock.Unlock() | ||
if n.consumer != nil { | ||
log.Println("consumer already existed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this log
test/e2e-api/kafka.go
Outdated
n.mLock.Lock() | ||
defer n.mLock.Unlock() | ||
if n.producer != nil { | ||
log.Println("producer already existed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this log
test/e2e-api/nats.go
Outdated
@@ -149,5 +172,6 @@ func (n *NatsController) PumpJetstream(w http.ResponseWriter, r *http.Request) { | |||
} | |||
|
|||
func (n *NatsController) Close() { | |||
// no lazy clozing required becuase they check if nc is not null before they close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos
// Nil check is not required here because the client.Close() function does a nil check internally.
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
6f07f90
to
808b78d
Compare
@samhith-kakarla There are permission issues with the doc you added, can you directly add the raw data here. Might be easier |
@samhith-kakarla As discussed, Could you add the verification result for the local testing for single test function with the updated go generate to block extra resources for those cases as well. |
test/e2e-api/kafka.go
Outdated
*/ | ||
|
||
func (n *KafkaController) getKafkaClient() sarama.ClusterAdmin { | ||
n.mLock.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 lines should be moved to line 54.
test/e2e-api/kafka.go
Outdated
2) Returning the current client if it exists | ||
*/ | ||
|
||
func (n *KafkaController) getKafkaClient() sarama.ClusterAdmin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is just for testing, we don't need to separate them into multiple different get
functions, instead, do something like func (n *KafkaController) getProducerAndConsumer() (sarama.ClusterAdmin, sarama.SyncProducer, sarama.Consumer)
.
This is to avoid too much redundant code.
test/e2e-api/kafka.go
Outdated
func (n *KafkaController) getKafkaClient() sarama.ClusterAdmin { | ||
n.mLock.Lock() | ||
defer n.mLock.Unlock() | ||
if n.adminClient != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check needs to happen again after the lock is acquired.
nc, err := natslib.Connect(url, opts...) | ||
// getter method for lazy loading. creates and returns nats client only when required | ||
func (n *NatsController) getNatsClient() *natslib.Conn { | ||
n.mLock.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lock should happen after client nil check.
func (n *NatsController) getNatsClient() *natslib.Conn { | ||
n.mLock.Lock() | ||
defer n.mLock.Unlock() | ||
if n.client != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, nil check should happen twice, before and after acquiring the lock.
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
9a36fe6
to
402ec82
Compare
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
a42dc4b
to
9aaa037
Compare
Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com> Signed-off-by: Samhith Kakarla <samhith_kakarla@intuit.com>
creating the pr for testing of ci do not merge right now