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

Jump to bottom

test: extended: add test checking for mismatch between Machines and Nodes #25419

Merged
merged 1 commit into from Aug 18, 2020
Merged

test: extended: add test checking for mismatch between Machines and Nodes #25419

merged 1 commit into from Aug 18, 2020

Conversation

sjenning
Copy link
Contributor

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2020
@deads2k
Copy link
Contributor

deads2k commented Aug 18, 2020

/lgtm
/hold

holding to see if this actually fails in failed fips run as expected. If so, I plan to override to merge so we can get the clear signal on fips for next time. Thanks @sjenning

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2020
@cgwalters
Copy link
Member

/approve
Looks sane to me, didn't review in detail though.

@jlebon
Copy link
Member

jlebon commented Aug 18, 2020

/approve

@deads2k deads2k removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2020
@deads2k
Copy link
Contributor

deads2k commented Aug 18, 2020

/lgtm
/override fips

@openshift-ci-robot
Copy link

@deads2k: /override requires a failed status context to operate on.
The following unknown contexts were given:

  • fips

Only the following contexts were expected:

  • ci/prow/e2e-aws-fips
  • ci/prow/e2e-aws-serial
  • ci/prow/e2e-cmd
  • ci/prow/e2e-gcp
  • ci/prow/e2e-gcp-builds
  • ci/prow/e2e-gcp-upgrade
  • ci/prow/images
  • ci/prow/verify
  • ci/prow/verify-deps
  • tide

In response to this:

/lgtm
/override fips

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
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, deads2k, jlebon, sjenning

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

@deads2k
Copy link
Contributor

deads2k commented Aug 18, 2020

/override ci/prow/e2e-aws-serial
/override ci/prow/e2e-aws-fips

@openshift-ci-robot
Copy link

@deads2k: Overrode contexts on behalf of deads2k: ci/prow/e2e-aws-fips, ci/prow/e2e-aws-serial

In response to this:

/override ci/prow/e2e-aws-serial
/override ci/prow/e2e-aws-fips

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.

@deads2k
Copy link
Contributor

deads2k commented Aug 18, 2020

/hold cancel

@deads2k
Copy link
Contributor

deads2k commented Aug 18, 2020

fips is expected fail (and it did).

This only adds a test that doesn't get added to serial overriding to improve the test coverage

@openshift-merge-robot openshift-merge-robot merged commit 9291ed4 into openshift:master Aug 18, 2020
@sdodson
Copy link
Member

sdodson commented Aug 18, 2020

This fails on e2e-metal
https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_installer/4066/pull-ci-openshift-installer-master-e2e-metal/1295843694116605952

@sdodson sdodson mentioned this pull request Aug 18, 2020
Merged
@cgwalters
Copy link
Member

Hmm right, we can only use this test in IPI/machineAPI managed scenarios. Perhaps the simplest is to skip the test if the number of machines is zero?

@sdodson
Copy link
Member

sdodson commented Aug 18, 2020

Yeah that seems reasonable. Just curious if these presubmits even exist.

/test e2e-metal
/test e2e-gcp-upi

cgwalters
cgwalters reviewed Aug 18, 2020
View reviewed changes
test/extended/machines/cluster.go
defer g.GinkgoRecover()

g.It("have same number of Machines and Nodes", func() {
if e2e.TestContext.Provider == ibmcloud.ProviderName {
Copy link
Member

Choose a reason for hiding this comment

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

This is just a special case of the "UPI/no-machineAPI" scenario that also applies in e.g. AWS UPI, so should be replaced by whatever check we use for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters left review comments

@mfojtik mfojtik Awaiting requested review from mfojtik

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@deads2k deads2k

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sjenning @deads2k @cgwalters @jlebon @openshift-ci-robot @sdodson @openshift-merge-robot