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

🐛 pass the cluster in to the get targets #4367

Merged
merged 1 commit into from
Mar 25, 2021
Merged

🐛 pass the cluster in to the get targets #4367

merged 1 commit into from
Mar 25, 2021

Conversation

nader-ziada
Copy link
Contributor

What this PR does / why we need it:

  • the e2e tests for mhc are failing for not being able to get the cluster using the remote client, but we already have the cluster in the calling func

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 24, 2021
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 24, 2021
@nader-ziada nader-ziada changed the title [WIP] 🐛 pass the cluster in to the get targets 🐛 pass the cluster in to the get targets [WIP] Mar 24, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2021
@nader-ziada
Copy link
Contributor Author

/test ls

@k8s-ci-robot
Copy link
Contributor

@nader-ziada: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test pull-cluster-api-build-main
  • /test pull-cluster-api-make-main
  • /test pull-cluster-api-apidiff-main
  • /test pull-cluster-api-verify
  • /test pull-cluster-api-test-main
  • /test pull-cluster-api-e2e-main
  • /test pull-cluster-api-e2e-full-main
  • /test pull-cluster-api-e2e-k8s-latest-main

Use /test all to run the following jobs:

  • pull-cluster-api-build-main
  • pull-cluster-api-make-main
  • pull-cluster-api-apidiff-main
  • pull-cluster-api-verify
  • pull-cluster-api-test-main
  • pull-cluster-api-e2e-main

In response to this:

/test ls

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.

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main

vincepri
vincepri reviewed Mar 24, 2021
View reviewed changes
controllers/machinehealthcheck_targets.go
Comment on lines -188 to -191
var cluster clusterv1.Cluster
if err := clusterClient.Get(ctx, client.ObjectKey{Namespace: mhc.Namespace, Name: mhc.Spec.ClusterName}, &cluster); err != nil {
return nil, errors.Wrapf(err, "error getting Cluster %s/%s for MachineHealthCheck %s", mhc.Namespace, mhc.Spec.ClusterName, mhc.Name)
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this client be r.Client and not the target Cluster client?

Copy link
Member

Choose a reason for hiding this comment

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

Regardless of this, +1 passing the object in if we already have it

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I got it correctly:

  • the issue was that we were trying to get the cluster from the target cluster instead of the management cluster
  • but we still need the clusterClient (as target cluster client) in l.211

@vincepri
Copy link
Member

/retest

@fabriziopandini
Copy link
Member

Uhh, it seems that #4354 introduced a regression

Panic: interface conversion: *framework.clusterProxy is not e2e.ClusterProxy: missing method ApplyWithArgs

cc @shysank

@fabriziopandini fabriziopandini added this to the v0.4 milestone Mar 24, 2021
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main

sbueringer
sbueringer reviewed Mar 24, 2021
View reviewed changes
test/e2e/kcp_adoption.go Outdated
@@ -52,7 +52,7 @@ type KCPAdoptionSpecInput struct {
type ClusterProxy interface {
framework.ClusterProxy

ApplyWithArgs(context.Context, []byte, ...string) error
Apply(context.Context, []byte, ...string) error
Copy link
Member

Choose a reason for hiding this comment

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

This change seems to be unrelated, was it intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a previous PR changed this, so was getting a panic, check Fabrizio comment above

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see it now, didn't see the the connection the first time.

@nader-ziada
Copy link
Contributor Author

the e2e tests all passed, once #4371 merges, will rebase and this will be good to go

@nader-ziada nader-ziada changed the title 🐛 pass the cluster in to the get targets [WIP] 🐛 pass the cluster in to the get targets Mar 24, 2021
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main

@sbueringer
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 24, 2021
@nader-ziada
Copy link
Contributor Author

failed on a time out, will try again

/test pull-cluster-api-e2e-full-main

@sbueringer
Copy link
Member

Probably just stating the obvious but:
Timeout again. Looks to me like one of the machines does not come up because of a timeout when trying to create the DockerMachine:

E0324 20:21:46.852841       1 controller.go:302] controller-runtime/manager/controller/dockermachine "msg"="Reconciler error" "error"="failed to create worker DockerMachine: timed out waiting for the condition" "name"="k8s-conformance-nu20ky-md-0-njj9v" "namespace"="k8s-conformance-23gfsn" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="DockerMachine" 
I0324 20:21:48.423611       1 machine.go:357] controller-runtime/manager/controller/dockermachine "msg"="Setting Kubernetes node providerID" "name"="machine-pool-3h8bin-control-plane-q6gcw" "namespace"="machine-pool-2t0lry" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="DockerMachine" 
I0324 20:21:49.146323       1 machine.go:312] controller-runtime/manager/controller/dockermachine "msg"="Failed running command" "name"="k8s-conformance-nu20ky-md-0-njj9v" "namespace"="k8s-conformance-23gfsn" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="DockerMachine" "bootstrap data"="<user-data>==" "command"={"Cmd":"mkdir","Args":["-p","/run/kubeadm"],"Stdin":""} "stderr"="Error response from daemon: Container 317b0f95e3f6073f59a914837e124548924b45abd0545e909c2b68fc3f47318f is not running\n" "stdout"=""

(https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/4367/pull-cluster-api-e2e-full-main/1374815742700032000/artifacts/clusters/bootstrap/controllers/capi-controller-manager/capi-controller-manager-7c6fc9fcbf-6dnxx/manager.log)

I'm not that familiar (yet) with CAPD and its e2e test but looks to me like we don't have enough logs (at the moment) to find out why we get those timeouts.

/test pull-cluster-api-e2e-full-main

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-e2e-full-main

@nader-ziada
Copy link
Contributor Author

@CecileRobertMichon ready for a a look

@nader-ziada nader-ziada mentioned this pull request Mar 25, 2021
Merged
3 tasks
@CecileRobertMichon
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 Mar 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7124a65 into kubernetes-sigs:master Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@vincepri vincepri vincepri left review comments

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@sbueringer sbueringer

@CecileRobertMichon CecileRobertMichon

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

MHC E2E tests are failing
6 participants
@nader-ziada @k8s-ci-robot @vincepri @fabriziopandini @sbueringer @CecileRobertMichon