🌱 Refactor Tester to e2e framework pkg. #565

xuezhaojun · 2024-07-10T16:13:10Z

Summary

In this PR, we refactor the Tester into a e2e framework pkg. The pkg includes:

2 main abstraction Hub and Spoke, they holds the all clients that need to interact with a hub or a spoke, the metadata, and the runtime data.
Files that contains functions of checking, creating, and etc of a specific aspect. For example, under the managedcluster.go file, there are GetManagedCluster and CheckManagedClusterStatus.

The default e2e has 1 hub and 1 spoke, the hub and spoke are global varible so developer can easily use them, for example, if in your case, you want to do something on the ManagedCluster, you can do:

mc, err := hub.ClusterClient.ClusterV1().ManagedClusters().Get(context.TODO(),
		clusterName, metav1.GetOptions{})

You can also check the file test/e2e/framework/managedcluster.go to see if there is already a function implement your intention. All functions are seperated by different aspect so it's easier to find.

codecov · 2024-07-10T16:19:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.47%. Comparing base (9a7efae) to head (c7a262c).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
- Coverage   62.56%   62.47%   -0.10%     
==========================================
  Files         178      178              
  Lines       13968    13968              
==========================================
- Hits         8739     8726      -13     
- Misses       4347     4358      +11     
- Partials      882      884       +2

Flag	Coverage Δ
unit	`62.47% <ø> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xuezhaojun · 2024-07-11T00:03:18Z

/assign @qiujian16

xuezhaojun · 2024-07-11T07:03:48Z

/assign @qiujian16
PR updated, please take another look, thanks!

qiujian16 · 2024-07-11T13:19:03Z

test/framework/klusterlet.go

+// CreateAndApproveKlusterlet requires operations on both hub side and spoke side
+func CreateAndApproveKlusterlet(
+	hub *Hub, spoke *Spoke,
+	klusterletName, managedClusterName, klusterletNamespace string,


if spoke already has the klusterletName/namspace, why do we still need these variables in this func?

That's a good point.

The spoke has:

KlusterletOperatorNamespace

KlusterletOperator

Which is different from KlusterletNamespace and Klusterlet.

One represent the operator deployment, one represent a Klusterlet.

what does KlusterletOperator mean?

It's a copy of:

ocm/test/e2e/common.go

Line 73 in 7a8e444

klusterletOperator string

I think it represents the deployment of of klusterlet operator:

ocm/deploy/klusterlet/config/operator/operator.yaml

Line 6 in 7a8e444

name: klusterlet

I do not think this needs to be a variable.

That's true, maybe we can change these 2 to const values? I can submit a another smaller PR to follow up this.

test/framework/spoke.go

test/framework/featuregate.go

test/framework/hub.go

qiujian16 · 2024-07-12T02:04:18Z

test/framework/klusterlet.go

+	return realKlusterlet, nil
+}
+
+func CreatePureHostedKlusterlet(spoke *Spoke, name, clusterName string) (*operatorapiv1.Klusterlet, error) {


why this func, and why this is different from CreateKlusterlet. When should we use this?

That's a good question but may beyond this PR's scope, the function is directly copied from Tester so I will try to answer.

Comparing with the CreateKlusterlet which not only create Klusterlet but also create the related reasources like namespace, bootstrapkubeconfigsecret, externalKubeconfigSecret, the CreatePureHostedKlusterlet only create Klusterlet, no another thing.

It seems there is a case that requires only Hosted mode Klusterlet, but don't want the external managed kubeconfig, that's why we want a more Pure function.

It("Delete klusterlet CR in Hosted mode without external managed kubeconfig", func() { By(fmt.Sprintf("create klusterlet %v with managed cluster name %v in Hosted mode", klusterletName, clusterName)) _, err := CreatePureHostedKlusterlet(spoke, klusterletName, clusterName) Expect(err).ToNot(HaveOccurred())

I can add a TODO on this function and the CreateKlusterlet to try to merge them into one if we need.

test/framework/klusterlet.go

qiujian16 · 2024-07-12T02:07:27Z

test/framework/klusterlet.go

+}
+
+// CleanKlusterletRelatedResources needs both hub side and spoke side operations
+func CleanKlusterletRelatedResources(


we actually should have a clean func for spoke and hub. Something like spoke.Cleanup() and hub.Cleaup, and call these two in this func.

Agree, this is something simply copied and pasted in the PR.

But in this case, it's not like hub.Cleanup is suitable, because hub.Cleanup is like a entire hub Cleanup, in this function, we only want to clean up the managedcluster created by the klusterlet on the hub.

And also spoke != klustelret, spoke has multiple klusterlet during the e2e.

So I think we can have hub.Cleanup and spoke.Cleanup but not for this case.

this could be confusing, because to me a spoke == a klusterlet

and for hub, hub.Cleanup(cluster1) should be fine.

During the e2e we are deploying multiple klusterlet on the same spoke:

ocm/test/e2e/klusterlet_test.go

Line 38 in 7a8e444

_, err := t.CreateKlusterlet(klusterletName, clusterName, klusterletNamespace, "")

In our e2e, a spoke should be a cluster that has one klusterlet operator on it, but we can deploy multiple klusterlet on one spoke.

Should we use CleanUpManagedCluster? Because if we use CleanUp specifically represent to clean up a managedcluster, then clean up manifestwork, clustermanager and other resources will be confusing.

I think you will need:
1: just clean klusterlet CR,
2. clean klusterlet CR and klusterlet operator
3. just clean a managedcluster CR
4. clean them all

Yes, can I list it as a TODO in the future? because "clean the klusterlet operator" part feels need more discussion, it's a behavior change comparing with previous code.

qiujian16 · 2024-07-12T02:11:01Z

test/framework/managedcluster.go

+	clusterv1 "open-cluster-management.io/api/cluster/v1"
+)
+
+func GetManagedCluster(hub *Hub, clusterName string) (*clusterv1.ManagedCluster, error) {


If these are all hub side operation, shouldn't we make it as func in hub struct?

That's a good question, I think a lot on this, whether the GetManagedCluster and other similar code blocks be a method of hub or a function.

The reason 1 is to keep the defination and scope of a hub clear, hub is only:

the client

the metadata

the runtime data

It provide information for us to do things, but not do things for us.

Most functions doesn't need the entire hub, but only some of clients. This part can be improved in the future. We will make each function's input and output clearly what it need.

Just in this time, change the function one by one is kind of time consuming, if we can first set the framework down, and then we can make enhancement with other meaningful refactors together. Such as refactor the CreatePureHostedKlusterlet.

The reason 2 is, the GetManagedCluster actually doesn't need the whole hub, it only needs the clusterclient(again it's a copied-paste issue in this PR), and if we can have:

func GetManagedCluster(clusterclient clusterclient.Interface, clusterName string) (*clusterv1.ManagedCluster, error) { if clusterName == "" { return nil, fmt.Errorf("the name of managedcluster should not be null") } return clusterclient.ClusterV1().ManagedClusters().Get(context.TODO(), clusterName, metav1.GetOptions{}) }

That means we can reuse this function and this framework not only in e2e, but also the intergration test, the addon repo's e2e and integration, the downstream repos which rely on ocm.

Currently if we develop a addon or a component that rely on ocm, we write integration and e2e part(code and scripts) indenependly but if we can provide a ocm-test-framework, I think it can reduce some effort in write and maintaining these tests.

To my undstanding, hub defines a hub environment. A test case could manage hub environment with the hub struct.

Sure, I can change to hub.GetManagedCluster since currently we have no plan for a ultimate test-framework for all ocm related repos. But I still think GetManagedCluster is not limit to a hub environment, in integration cases, there is not hub just controllers and fake kube-apiserver, we can still reuse GetManagedCluster.

Updated, all functions are moved under hub and spoke as method.

Signed-off-by: xuezhaojun <zxue@redhat.com>

qiujian16

/approve

looks good in general, just some minor comments. And I would like everyone to take a look on the change. @open-cluster-management-io/core-dev

openshift-ci · 2024-07-15T14:44:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, xuezhaojun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [qiujian16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

xuezhaojun · 2024-07-22T08:23:55Z

/approve

looks good in general, just some minor comments. And I would like everyone to take a look on the change. @open-cluster-management-io/core-dev

Hi, @qiujian16 I have synced the changes with @open-cluster-management-io/core-dev in a internal weekly meeting, is it ok now to merge the PR?

qiujian16 · 2024-07-24T01:27:59Z

/lgtm

openshift-ci bot requested review from elgnay and zhujian7 July 10, 2024 16:13

xuezhaojun force-pushed the replace-tester-with-hubtesthelper-and-agenttesthelper branch 4 times, most recently from 35bcd8e to 0e4915b Compare July 10, 2024 23:26

openshift-ci bot assigned qiujian16 Jul 11, 2024

xuezhaojun changed the title ~~🌱 Sperate Tester to HubTester and SpokeTester.~~ WIP 🌱 Sperate Tester to HubTester and SpokeTester. Jul 11, 2024

openshift-ci bot added the do-not-merge/work-in-progress label Jul 11, 2024

xuezhaojun force-pushed the replace-tester-with-hubtesthelper-and-agenttesthelper branch 2 times, most recently from 65f9102 to 4979dc2 Compare July 11, 2024 06:50

xuezhaojun changed the title ~~WIP 🌱 Sperate Tester to HubTester and SpokeTester.~~ 🌱 Refactor Tester to e2e framework pkg. Jul 11, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 11, 2024

xuezhaojun force-pushed the replace-tester-with-hubtesthelper-and-agenttesthelper branch from 4979dc2 to 2fc636d Compare July 11, 2024 07:01

xuezhaojun force-pushed the replace-tester-with-hubtesthelper-and-agenttesthelper branch from 2fc636d to c5d063a Compare July 12, 2024 01:54

qiujian16 reviewed Jul 12, 2024

View reviewed changes

xuezhaojun requested a review from qiujian16 July 12, 2024 05:09

xuezhaojun force-pushed the replace-tester-with-hubtesthelper-and-agenttesthelper branch from c5d063a to eb60ecf Compare July 12, 2024 05:09

Refactor tester to e2e framework.

c7a262c

Signed-off-by: xuezhaojun <zxue@redhat.com>

xuezhaojun force-pushed the replace-tester-with-hubtesthelper-and-agenttesthelper branch from eb60ecf to c7a262c Compare July 12, 2024 08:45

qiujian16 reviewed Jul 15, 2024

View reviewed changes

openshift-ci bot added the approved label Jul 15, 2024

xuezhaojun requested a review from qiujian16 July 17, 2024 06:58

openshift-ci bot added the lgtm label Jul 24, 2024

openshift-merge-bot bot merged commit 6c4292b into open-cluster-management-io:main Jul 24, 2024
15 checks passed

xuezhaojun deleted the replace-tester-with-hubtesthelper-and-agenttesthelper branch July 24, 2024 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Refactor Tester to e2e framework pkg. #565

🌱 Refactor Tester to e2e framework pkg. #565

xuezhaojun commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

xuezhaojun commented Jul 11, 2024

xuezhaojun commented Jul 11, 2024

qiujian16 Jul 11, 2024

xuezhaojun Jul 12, 2024

qiujian16 Jul 12, 2024

xuezhaojun Jul 12, 2024 •

edited

Loading

qiujian16 Jul 15, 2024

xuezhaojun Jul 15, 2024 •

edited

Loading

qiujian16 Jul 12, 2024

xuezhaojun Jul 12, 2024 •

edited

Loading

qiujian16 Jul 12, 2024

xuezhaojun Jul 12, 2024 •

edited

Loading

qiujian16 Jul 12, 2024

qiujian16 Jul 12, 2024

xuezhaojun Jul 12, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024

qiujian16 Jul 15, 2024

xuezhaojun Jul 15, 2024 •

edited

Loading

qiujian16 Jul 12, 2024

xuezhaojun Jul 12, 2024 •

edited

Loading

qiujian16 Jul 12, 2024

xuezhaojun Jul 12, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024

qiujian16 left a comment

openshift-ci bot commented Jul 15, 2024

xuezhaojun commented Jul 22, 2024

qiujian16 commented Jul 24, 2024

🌱 Refactor Tester to e2e framework pkg. #565

🌱 Refactor Tester to e2e framework pkg. #565

Conversation

xuezhaojun commented Jul 10, 2024 • edited Loading

Summary

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

xuezhaojun commented Jul 11, 2024

xuezhaojun commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuezhaojun Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiujian16 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 15, 2024

xuezhaojun commented Jul 22, 2024

qiujian16 commented Jul 24, 2024

xuezhaojun commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024 •

edited

Loading

xuezhaojun Jul 15, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024 •

edited

Loading

xuezhaojun Jul 15, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024 •

edited

Loading

xuezhaojun Jul 12, 2024 •

edited

Loading