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

node: skip docker ping check when using fake docker #17979

Merged
merged 1 commit into from
Jan 5, 2018

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Jan 3, 2018

When using fake docker

  • Skips ping check
  • Sets kubelet cgroup driver to default (cgroupfs)

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 3, 2018
@csrwng
Copy link
Contributor Author

csrwng commented Jan 3, 2018

@deads2k we need to be able to run the kubelet with a fake docker client (which is doable when you set DOCKER_HOST=fake://). However, we implement our own version of Ping() which doesn't use the fake client. This change makes it so we skip the Ping when using the fake Docker client

/cc @stevekuznetsov @smarterclayton

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 4, 2018
@csrwng csrwng changed the title node: use a warning instead of fatal when docker ping fails node: skip docker ping check when using fake docker Jan 4, 2018
@stevekuznetsov
Copy link
Contributor

Just the one failure now:

runner_test.go:124: FAILED TestDiagNodeConditions, retrying:

diag_nodes_test.go:42: expected no warnings with one node ready, but: []types.DiagnosticError{types.DiagnosticError{ID:"DClu0002", LogMessage:"Node 127.0.0.1 is defined but is not marked as ready.\nReady status is False because \"KubeletNotReady\"\nIf the node is not intentionally disabled, check that the master can\nreach the node hostname for a health check and the node is checking in\nto the master with the same hostname.\n\nWhile in this state, pods should not be scheduled to deploy on the node,\nand any existing scheduled pods will be considered failed and removed.\n", Cause:error(nil)}}
etcd.go:140: dumping etcd to "/tmp/openshift/test-integration/tmp-testdiagnodeconditions138363114/etcd-dump-TestDiagNodeConditions-v2.json"

}

if !isFakeDocker {
if err := dockerClient.Ping(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not adding the Ping method to the FakeDockerClient rather than introducing this hack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd still need to know whether we're using it so we can match the cgroupdriver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will open a PR upstream

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

@deads2k
Copy link
Contributor

deads2k commented Jan 4, 2018

@deads2k we need to be able to run the kubelet with a fake docker client (which is doable when you set DOCKER_HOST=fake://). However, we implement our own version of Ping() which doesn't use the fake client. This change makes it so we skip the Ping when using the fake Docker client

/cc @stevekuznetsov @smarterclayton

I'm ok with it. It's fairly likely to slip during a rebase (type assertions break all the time), so you may want a test.

@stevekuznetsov
Copy link
Contributor

As soon as this is passing our containerized tests it can't slip in a rebase since it will be required to run integration tests in CI

@deads2k
Copy link
Contributor

deads2k commented Jan 4, 2018

As soon as this is passing our containerized tests it can't slip in a rebase since it will be required to run integration tests in CI

@csrwng if that's the case, consider yourself on notice for rebase help if this borks badly :)

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 4, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your 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 Jan 4, 2018
@csrwng
Copy link
Contributor Author

csrwng commented Jan 4, 2018

This is needed to unblock integration tests. I will create a follow up issue to clean it up

@stevekuznetsov
Copy link
Contributor

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 17856, 16934, 17979, 17993, 18001).

@openshift-merge-robot openshift-merge-robot merged commit fb1e9e6 into openshift:master Jan 5, 2018
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants