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: standardize machine filter functions and improve testing #4207

Merged
merged 1 commit into from
Mar 22, 2021
Merged

🌱 refactor: standardize machine filter functions and improve testing #4207

merged 1 commit into from
Mar 22, 2021

Conversation

CecileRobertMichon
Copy link
Contributor

What this PR does / why we need it: The util, controllers, kcp internal packages had overlapping functions that fetch or filter many machines. Some functions use the clusterv1.MachineList type, others use []*clusterv1.Machine, others use collections.Machines. This PR standardizes the use of filter functions in util/collections across the codebase.

(Issue came out this discussion: #2037 (comment))

In addition, I found a few "bugs" in tests of those functions - tests that weren't actually testing what they said they were testing so I fixed those.

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 #2062

@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 19, 2021
CecileRobertMichon
CecileRobertMichon commented Feb 19, 2021
View reviewed changes
controllers/machine_controller_test.go
@@ -1377,12 +1303,20 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
expectedError: errNilNodeRef,
},
{
name: "no control plane members",
cluster: &clusterv1.Cluster{},
name: "no control plane members",
Copy link
Contributor Author

@CecileRobertMichon CecileRobertMichon Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this test was not fully correct as the machines were being excluded from the filter by not having the right cluster name label and/or namespace. Added all the required object metadata so it's actually checking that the described missing piece (eg. no control plane members) is what causes the error.

CecileRobertMichon
CecileRobertMichon commented Feb 19, 2021
View reviewed changes
controlplane/kubeadm/controllers/status_test.go
@@ -46,6 +46,10 @@ func TestKubeadmControlPlaneReconciler_updateStatusNoMachines(t *testing.T) {
}

kcp := &controlplanev1.KubeadmControlPlane{
TypeMeta: metav1.TypeMeta{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one got me debugging the test for a while 😬

the test was actually a no-op because of https://github.com/kubernetes-sigs/cluster-api/pull/4207/files#diff-a6639fd91ffb9cff50ef1f3af76ec2371aa5c41e0d28b98b651cb9193eeaab62L52

Since the fake management cluster Management was nil, it wasn't actually listing machines and filtering them, it was just returning ALL machines that were passed into managementCluster.Machines. When I changed it to actually using the filter, it was failing because the Machines weren't "owned" by KCP since the KCP TypeMeta was missing here but not in the owner ref, it was comparing empty string to the ref's Kind, Group, and Version.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2021
@CecileRobertMichon
Copy link
Contributor Author 

sigs.k8s.io/cluster-api/util
  Incompatible changes:
  - HasPausedAnnotation: removed
  - ImageTagIsValid: removed
  - IsPaused: removed
  - MachineListFormatDeprecationMessage: removed
  - ModifyImageRepository: removed
  - ModifyImageTag: removed
  - PointsTo: removed
  - SemverToOCIImageTag: removed
  - WatchOnClusterPaused: removed

All of should be removed in #4078 already

@CecileRobertMichon
Copy link
Contributor Author

/milestone v0.4.0

@k8s-ci-robot k8s-ci-robot added this to the v0.4.0 milestone Feb 19, 2021
vincepri
vincepri reviewed Feb 19, 2021
View reviewed changes
controllers/cluster_controller.go Outdated Show resolved Hide resolved
util/collections/machine_filters.go Outdated Show resolved Hide resolved
MarcelMue
MarcelMue reviewed Feb 23, 2021
View reviewed changes
controllers/machine_controller.go Outdated Show resolved Hide resolved
util/collections/machine_collection.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2021
@CecileRobertMichon
Copy link
Contributor Author 

sigs.k8s.io/cluster-api/util
  Incompatible changes:
  - ImageTagIsValid: removed
  - MachineListFormatDeprecationMessage: removed
  - ModifyImageRepository: removed
  - ModifyImageTag: removed

All 4 are deprecated in alpha3: https://github.com/kubernetes-sigs/cluster-api/blob/release-0.3/util/util.go

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2021
@CecileRobertMichon
Copy link
Contributor Author

@MarcelMue @fabriziopandini thanks for the reviews. Now that #4078 has merged I've rebased this PR and addressed your comments, PTAL.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 15, 2021 •
edited
Loading

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-apidiff-main b1c8248 link /test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@fabriziopandini
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2021
@CecileRobertMichon
Copy link
Contributor Author

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2021
vincepri
vincepri approved these changes Mar 22, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, vincepri

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:
  • OWNERS [CecileRobertMichon,vincepri]

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 merged commit f310fe9 into kubernetes-sigs:master Mar 22, 2021
@k8s-ci-robot k8s-ci-robot modified the milestones: v0.4.0, v0.4.x Mar 22, 2021
@CecileRobertMichon CecileRobertMichon modified the milestones: v0.4.x, v0.4 Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@MarcelMue MarcelMue MarcelMue left review comments

@vincepri vincepri vincepri approved these changes

@detiber detiber Awaiting requested review from detiber

Assignees

@vincepri vincepri

@fabriziopandini fabriziopandini

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
v0.4
Development

Successfully merging this pull request may close these issues.

util package: Make functions that fetch/filter many Machines consistent with one another
5 participants
@CecileRobertMichon @k8s-ci-robot @fabriziopandini @vincepri @MarcelMue