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.

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

Machine healthcheck tests #47

Merged

Conversation

cynepco3hahue
Copy link
Contributor

Tests:

  • [Feature:MachineHealthCheck] MachineHealthCheck controller
    with node-unhealth-conditions configmap should delete unhealthy machine
  • [Feature:MachineHealthCheck] MachineHealthCheck controller
    should delete the unhealthy machine

Copy tests from the openshift/machine-api-operator#204

@openshift-ci-robot
Copy link

Hi @cynepco3hahue. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 12, 2019
@cynepco3hahue
Copy link
Contributor Author

@enxebre @ingvagabund Can you please run tests?

@ingvagabund
Copy link
Member

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 12, 2019
Artyom Lukianov added 2 commits March 13, 2019 09:35
Tests:
- [Feature:MachineHealthCheck] MachineHealthCheck controller
  with node-unhealth-conditions configmap should delete unhealthy machine
- [Feature:MachineHealthCheck] MachineHealthCheck controller
  should delete unhealthy machine

Copy tests from the openshift/machine-api-operator#204
@cynepco3hahue cynepco3hahue force-pushed the machine-healthcheck-tests branch from 36e70c4 to d1ea871 Compare March 13, 2019 07:35
@cynepco3hahue
Copy link
Contributor Author

/retest

@cynepco3hahue
Copy link
Contributor Author

@ingvagabund Does OpenShift CI has some problems, I can see

2019/03/13 07:45:23 Build src failed, printing logs:
Pulling image "registry.svc.ci.openshift.org/ci/clonerefs@sha256:835268c14fbcad843bc9752ba579ec3be2e7d19c94f6dfc627a3dacfeaf1afce" ...
error: error pulling image registry.svc.ci.openshift.org/ci/clonerefs@sha256:835268c14fbcad843bc9752ba579ec3be2e7d19c94f6dfc627a3dacfeaf1afce: received unexpected HTTP status: 500 Internal Server Error

@ingvagabund
Copy link
Member

/retest

@ingvagabund
Copy link
Member

@ingvagabund Does OpenShift CI has some problems, I can see

This happens from time to time

@cynepco3hahue
Copy link
Contributor Author

/retest

@ingvagabund
Copy link
Member

@cynepco3hahue the CI is currently broken. The issue is under investigation and being resolved.

@cynepco3hahue
Copy link
Contributor Author

/retest

@cynepco3hahue
Copy link
Contributor Author

@ingvagabund Great thanks for info.

@cynepco3hahue
Copy link
Contributor Author

@ingvagabund I can see that k8s-e2e fails on timeout

E0313 11:29:28.355551   16992 utils.go:27] Error querying api for Deployment object "clusterapi-manager-controllers": deployments.apps "clusterapi-manager-controllers" not found, retrying...
E0313 11:29:28.357045   16992 utils.go:27] Error querying api for Deployment object "clusterapi-manager-controllers": deployments.apps "clusterapi-manager-controllers" not found, retrying...
E0313 11:29:28.357071   16992 utils.go:51] Error getting deployment: error getting deployment "clusterapi-manager-controllers": timed out waiting for the condition
E0313 11:29:28.357081   16992 utils.go:61] Error checking isDeploymentAvailable: timed out waiting for the condition

Do you have an idea of why it happens?

@ingvagabund
Copy link
Member

I can see that k8s-e2e fails on timeout

flaky test

@ingvagabund
Copy link
Member

/retest

@@ -49,6 +49,10 @@ required = [
name = "github.com/openshift/cluster-autoscaler-operator"
branch = "master"

[[constraint]]
name = "github.com/openshift/machine-api-operator"
revision = "9650e16c98802a4b57b7551201b0973fcae2f738"
Copy link
Member

Choose a reason for hiding this comment

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

This will cause a trouble in the mao repo itself. Though, we still may override the constraint. So good to go.

@ingvagabund
Copy link
Member

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2019
@openshift-merge-robot openshift-merge-robot merged commit 6c9c0c0 into openshift:master Mar 13, 2019
@ingvagabund
Copy link
Member

@cynepco3hahue thanks for the PR!!!

err = client.List(context.TODO(), &listOptions, workers)
Expect(err).ToNot(HaveOccurred())

numberOfReadyWorkers = 0
Copy link
Member

Choose a reason for hiding this comment

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

This deserves its own function and put it into utils. There's a library meant to be reusable in infra/utils. So then ginkgo e2e user stories are abstracted and easy to read with out caring about details

err = client.List(context.TODO(), &listOptions, machineList)
Expect(err).ToNot(HaveOccurred())

for i, m := range machineList.Items {
Copy link
Member

Choose a reason for hiding this comment

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

This deserves its own function and put it into utils. There's a library meant to be reusable in infra/utils. So then ginkgo e2e user stories are abstracted and easy to read with out caring about details

}
Expect(workerMachine).ToNot(BeNil())

if strings.Contains(workerMachine.Name, "kubemark") {
Copy link
Member

Choose a reason for hiding this comment

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

this needs a TODO

@enxebre
Copy link
Member

enxebre commented Mar 14, 2019

@cynepco3hahue thanks! I didn't have the chance to review that, so dropped some comments for the record so we can translate into follow ups improvements

@cynepco3hahue
Copy link
Contributor Author

@enxebre Created the PR, for all your comments 😃
#51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants