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.

Sign up for GitHub

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

Jump to bottom

🏃 Refactor of the e2e framework #2294

Merged
merged 1 commit into from
Feb 12, 2020
Merged

🏃 Refactor of the e2e framework #2294

merged 1 commit into from
Feb 12, 2020

Conversation

chuckha
Copy link
Contributor

@chuckha chuckha commented Feb 8, 2020 •
edited
Loading

Signed-off-by: Chuck Ha [email protected]

What this PR does / why we need it:
This PR pulls apart the main test into Creation functions, Assertion functions and Deleting functions. This allows far more flexibility in various ways:

  • Can be used with serial control plane node spin up (<-- the most important one)
  • Clients can now test an exact set up, assuming the steps are provided
  • Clients can customize the timeout and polling interval of every test or globally
  • Clients are not forced into the exact set up defined in the old control plane node
  • The old test still remains but is marked as deprecated. All deprecated functions and types are moved into a deprecated.go file.

I would be happy to simply remove the deprecated.go but didn't want to force anyone into the new style until they have time to update their code.

The biggest downside with this PR is increased length of a test for clients, but the upside is that the test is much more flexible and very explicit.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2251
Fixes #2291

/assign @wfernandes @detiber
/cc @akutz @CecileRobertMichon

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2020
@akutz
Copy link
Contributor

akutz commented Feb 8, 2020

/cc @yastij

@k8s-ci-robot k8s-ci-robot requested a review from yastij February 8, 2020 18:35
detiber
detiber reviewed Feb 8, 2020
View reviewed changes
test/framework/control_plane.go Outdated Show resolved Hide resolved
test/framework/control_plane.go Outdated Show resolved Hide resolved
test/framework/control_plane.go Show resolved Hide resolved
test/framework/control_plane.go Outdated Show resolved Hide resolved
test/framework/control_plane.go Show resolved Hide resolved
test/framework/control_plane.go Show resolved Hide resolved
test/framework/control_plane.go Show resolved Hide resolved
@chuckha chuckha force-pushed the e2e-refactor branch 4 times, most recently from dda38ae to 8b7e125 Compare February 8, 2020 20:55
@chuckha chuckha mentioned this pull request Feb 10, 2020
Merged
fabriziopandini
fabriziopandini reviewed Feb 11, 2020
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

+1 for me. I like the consistency of the proposed approach

test/framework/control_plane.go Show resolved Hide resolved
@fabriziopandini fabriziopandini mentioned this pull request Feb 11, 2020
Merged
@chuckha
Copy link
Contributor Author

chuckha commented Feb 11, 2020

cc @Arvinderpal

@chuckha
Copy link
Contributor Author

chuckha commented Feb 11, 2020

@detiber @fabriziopandini anything left to address before lgtm?

@fabriziopandini
Copy link
Member

lgtm from my side

detiber
detiber reviewed Feb 11, 2020
View reviewed changes
test/framework/control_plane.go Outdated Show resolved Hide resolved
test/framework/control_plane.go Outdated Show resolved Hide resolved
test/framework/control_plane.go Outdated
if err := workloadClient.List(ctx, &nodeList); err != nil {
return nil, err
}
return nodeList.Items, nil
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be returning the full node list and not filtering just the nodes related to the MachineDeployment.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to query the related Machines and wait for them to all have noderefs instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion would definitely be an improvement. It would be more true to cluster-api's vision of the cluster.

However this is what the test was doing previous. I'll open an issue to update to improve this function.

#2309

Copy link
Member

Choose a reason for hiding this comment

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

Agree with deferring the improvement to checking Machines, however Control Plane machines will also have associated Nodes on the remote cluster, and this currently is trying to compare the number of nodes retrieved against the number of desired replicas for the MachineDeployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, ok, this requires a change then. The assumption that this PR fixes unfortunately exposed this bug. Good code reading!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) maybe i meant fortunately.

test/framework/control_plane.go Show resolved Hide resolved
test/framework/control_plane.go Show resolved Hide resolved
@chuckha chuckha force-pushed the e2e-refactor branch 2 times, most recently from ad98115 to 163a114 Compare February 11, 2020 21:33
@detiber
Copy link
Member

detiber commented Feb 12, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2020
@k8s-ci-robot k8s-ci-robot merged commit 31b1857 into kubernetes-sigs:master Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagiesek nagiesek nagiesek left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@detiber detiber detiber left review comments

@akutz akutz akutz left review comments

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@yastij yastij Awaiting requested review from yastij

Assignees

@detiber detiber

@wfernandes wfernandes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
7 participants
@chuckha @k8s-ci-robot @akutz @fabriziopandini @detiber @nagiesek @wfernandes