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

Vendor machinehealthcheck tests #247

Merged
merged 1 commit into from
Mar 20, 2019
Merged

Vendor machinehealthcheck tests #247

merged 1 commit into from
Mar 20, 2019

Conversation

cynepco3hahue
Copy link
Contributor

@cynepco3hahue cynepco3hahue commented Mar 13, 2019 •
edited
Loading

Vendor machine health check tests from the cluster-api-actuator-pkg repository.

Relevant PR under the cluster-api-actuator-pkg repository - openshift/cluster-api-actuator-pkg#47

47c603ff Merge pull request #49 from ingvagabund/code-improvements
e734831f Detect kubemark based on presence of machineapi-kubemark-controllers deployment
5c7fd8e9 Get the latest revision of a machine object before updating it
95fed5db Use GetMachineSet to retrieve a machineset by its name
7242ac6c Set the default machineset Replicas field in constant
9d9e98cf Move util functions under utils.go
5fb1ddfe Do not use ginkgo/gomega naturalisms outside the main execution
8de61aa1 Be more verbose when failed to delete a resources
949b9a6e Improve code readibility
3ae752aa List nodes directly without any retry logic
4e07125f Create workload directly without any retry logic
c2ef4e17 Fix error message
e0e35ef7 Create CA and MA only once without any retry logic
66f31b52 Implement and apply DeleteObjectsByLabels
e4c63b7c Dereference a pointer only once
c0123276 List machineset list directly without retry logic
ff0ceade Build CA and MA resources within a function
14df399d Get machine directly without retry logic
9bb870a9 List machines directly without retry logic
c8a30176 List machinesets directly without retry logic
f59eb63b List nodes directly without retry logic
90efba3f Remove getMachineSetList in favor of getMachineSets
5963bfe5 Refactor getClusterSize to get nodes by calling getNodes
d881f928 Remove getNodeList in favor of getNodes
00598362 Makefile: build e2e
bae7e399 Remove getMachineList in favor of getMachines
6c9c0c00 Merge pull request #47 from cynepco3hahue/machine-healthcheck-tests
d1ea8713 Introduce machinehealthcheck tests
0785ce82 Vendor machine-api-operator
b418c217 Merge pull request #48 from frobware/bump-autoscaler-workload-timeout
8e7f0509 e2e/autoscaler: Bump autoscaler workload timeout
6da15dab Merge pull request #46 from frobware/fix-autoscaler-e2e-flaky-test
d7aba749 e2e/autoscaler/autoscaler: remove ActiveDeadlineSeconds from job
56b034e0 Merge pull request #44 from enxebre/taints
52e48ab3 Update machineSetsSnapShotLogs name
47bebf0f Update taints test to use dsl
c0baabd4 Merge pull request #45 from enxebre/operators-update
8af29489 Add nodes snapshot
85de4df7 Refactor deployments for operators
38ae1c23 Merge pull request #43 from enxebre/scaling-story
311ba82a Add scaling stories
502fb706 Rephrase Describe
135224d8 Merge pull request #42 from enxebre/recover-story
bf7ebeb5 Revendor for k8s.io/utils/pointer
f96f4c4f Improve 'recover from deleted worker machines' story
ea7dd9a6 Merge pull request #41 from enxebre/linked-nodes-fix
78143390 Abstract function for 'have machines resources backing nodes' story
023f28f6 Expect same number of machines and nodes

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

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.

@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 13, 2019
@cynepco3hahue
Copy link
Contributor Author

/retest

@cynepco3hahue
Copy link
Contributor Author

@ingvagabund Hi, I can see this test

Machine API operator deployment should
�[90m/data/src/github.com/openshift/machine-api-operator/vendor/github.com/openshift/cluster-api-actuator-pkg/pkg/e2e/operators/machine-api-operator.go:11�[0m
  �[91m�[1mreconcile controllers deployment [It]�[0m
  �[90m/data/src/github.com/openshift/machine-api-operator/vendor/github.com/openshift/cluster-api-actuator-pkg/pkg/e2e/operators/machine-api-operator.go:21�[0m

fails consistently under the kubemark environment, can it be related to some changes under the cluster-api-actuator-pkg repository?

@ingvagabund
Copy link
Member

fails consistently under the kubemark environment, can it be related to some changes under the cluster-api-actuator-pkg repository?

It's flaking occasionally. The rest of the tests failed due to missing machineset manifests. I have update underlying Jenkins Job.

/retest

@ingvagabund
Copy link
Member

Though, I will ask you to wait a bit and revendor again once we merge openshift/cluster-api-actuator-pkg#49. I have found a bug in the machine healtchecking tests. If the tests are run at the beginning of the test suite, the worker nodes does not have enough time to pop up. So the tests fail. I have moved the check for kubemark provider sooner in the BeforeEach/AfterEach functions so it really skips the tests right away.

@cynepco3hahue
Copy link
Contributor Author

cynepco3hahue commented Mar 14, 2019 •
edited
Loading

@ingvagabund Sure it already not so critical:) By the way maybe it better to add BeforeSuite method that will wait for some specific state of the environment before start tests. What do you think?

@ingvagabund
Copy link
Member

ingvagabund commented Mar 14, 2019 •
edited
Loading

that will wait for some specific state of the environment before start tests

That's one of our plans. We want to assert expected state of the cluster and wait for it for a while before a test starts running. Though, can't say when that happens. So feel free to go ahead :).

@cynepco3hahue
Copy link
Contributor Author

@ingvagabund Maybe I can help with this if we can describe default CI environment
Currently, I saw kubemark is different from aws by the number of nodes.

@enxebre
Copy link
Member

enxebre commented Mar 14, 2019

can you please add a link to the relevant PR on the description a git log --oneline diff of the commits e.g openshift/cluster-api-provider-aws#178

@cynepco3hahue cynepco3hahue force-pushed the vendor_machinehealthcheck_tests branch from 03f18d2 to 193aab7 Compare March 17, 2019 08:33
@cynepco3hahue
Copy link
Contributor Author

@ingvagabund I updated the cluster-api-actuator-pkg package
@enxebre Updated the description

@cynepco3hahue
Copy link
Contributor Author

/retest

1 similar comment
@cynepco3hahue
Copy link
Contributor Author

/retest

@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 20, 2019
@openshift-ci-robot
Copy link
Contributor

[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 20, 2019
@ingvagabund ingvagabund mentioned this pull request Mar 20, 2019
Merged
@ingvagabund
Copy link
Member

/test e2e-aws

@enxebre
Copy link
Member

enxebre commented Mar 20, 2019 •
edited
Loading

@cynepco3hahue the diff shows changes to the /autoscaler/autoscaler.go but I can't see anything related in the description or the counter part PR, would you mind to include the git log --oneline diff of the current commit and the one being introduced by this PR e.g openshift/cluster-api-provider-aws#178

@cynepco3hahue
Copy link
Contributor Author

/retest

@cynepco3hahue
Copy link
Contributor Author

@enxebre Done

@openshift-merge-robot openshift-merge-robot merged commit 80b680a into openshift:master Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ingvagabund ingvagabund Awaiting requested review from ingvagabund

@spangenberg spangenberg Awaiting requested review from spangenberg

Assignees

@ingvagabund ingvagabund

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cynepco3hahue @openshift-ci-robot @ingvagabund @enxebre @openshift-merge-robot