Run ssh bastion during openshift-tests #4161

jsafrane · 2019-06-24T12:33:54Z

Run ssh bastion and set up its env. variables before running openshift-tests in openshift_installer template. Some upstream e2e tests require SSH access to nodes.

Partly fixes https://bugzilla.redhat.com/show_bug.cgi?id=1711600

cc @vrutkovs to double check restore-cluster-state and recover-from-etcd-quorum-loss changes.

Tested with:

./ci-operator \
    --artifact-dir artifacts/ \
    --config openshift-origin-master.yaml \
    --git-ref jsafrane/origin@mount-propagation-test \
    --template cluster-launch-installer-e2e.yaml \
    --target cluster-launch-installer-e2e \
    --secret-dir "$name-cluster-profile/" \
    --namespace $ns

Where jsafrane/origin@mount-propagation-test is this PR: openshift/origin#22966

Mount propagation tests (= which need ssh to nodes) succeed.

vrutkovs · 2019-06-24T12:36:18Z

ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

It needs to be removed before the tests start, otherwise e2e test would fail - we're grabbing image from outside of the release image

The whole point of this PR is to run the tests with the bastion running.

Right, in that case we have to use some other images in eparis/ssh-bastion repo. See https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_release/3595/rehearse-3595-pull-ci-openshift-installer-master-e2e-restore-cluster-state/67, test [Feature:Platform][Smoke] Managed cluster should ensure pods use images from our release image with proper ImagePullPolicy [Suite:openshift/conformance/parallel] would fail with fail [github.com/openshift/origin/test/extended/operators/images.go:112]: May 10 09:33:15.658: Pods found with invalid container images not present in release payload: openshift-ssh-bastion/ssh-bastion-f4d5bbcbd-xcx68/ssh-bastion image=quay.io/eparis/ssh:latest

Indeed, that one is going to fail. I think the test could skip openshift-ssh-bastion namespace if $KUBE_SSH_BASTION is set.

openshift/origin#23252

Later on we shoud be vendoring the manifests in bindata and run it in ssh-bastion namespace in BeforeAll

Once openshift/origin#23208 merged I could take care of updating this

vrutkovs · 2019-06-24T12:41:56Z

/hold

The PR looks good, I doubt it would make tests pass though.

Could you add a space at the end of

release/ci-operator/jobs/openshift/installer/openshift-installer-master-presubmits.yaml

Line 757 in 53e161b

restore-cluster-state

in a separate commit to trigger a rehearse of restore etcd test? It would be reverted later on

jsafrane · 2019-06-24T12:54:05Z

Could you add a space at the end of restore-cluster-state?

Not exactly empty space, but done.

abhinavdahiya · 2019-06-24T16:36:31Z

SSH access to clusters that are up should not be allowed for e2e tests.

the e2e cluster restore are given SSH access because it brings the control plane down.

/cc @crawford @eparis

jsafrane · 2019-06-25T08:13:21Z

I am a bit confused. How ssh access brings control plane down?

There are tests that simply need to access OS on nodes. Upstream has chosen ssh as the access method. We can either disable those tests and leave corresponding parts untested, or, as I am trying, to run the tests with ssh bastion.

Access to nodes is mostly required by [Disruptive] tests that we don't run anyway (*), but there are few marked as [Disabled:Broken] by OpenShift and could run if we have SSH:

`SSH`, // TRIAGE
`should implement service.kubernetes.io/service-proxy-name`, // this is an optional test that requires SSH. sig-network
`should propagate mounts to the host`,                                        // requires SSH, https://bugzilla.redhat.com/show_bug.cgi?id=1711600

*) To be honest, I don't understand why we don't run [Disruptive] tests. Speaking from sig-storage perspective, they check kubelet restart / node reboot cases that have great potential to be buggy.

derekwaynecarr · 2019-06-25T13:58:48Z

ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

do we need to verify that the project is fully removed before we continue? is it possible that we will leak a loadbalancer otherwise?

IMO, oc delete waits for the namespace to be fully deleted.

derekwaynecarr · 2019-06-25T14:01:44Z

@abhinavdahiya I am not following your comment, we should make every effort to run upstream tests.

abhinavdahiya · 2019-06-25T14:36:02Z

With openshift 4 we don't ship with ssh access to the clusters by default. And my understanding was that we stopped tests that required ssh access in the hope that we can move them off needing ssh access.
And this PR turns on ssh for all tests which in my opinion is weakening our position. All tests from now on can use ssh...

jsafrane · 2019-07-01T11:53:51Z

/retest

jsafrane · 2019-07-01T13:58:01Z

I am updating Eric's ssh bastion to have configurable namespace: eparis/ssh-bastion#10

SSH access works well (Mount propagation should propagate mounts to the host passes)

This one fails reliably, but it looks unrelated to the template change:

The default cluster RBAC policy should only allow the system:authenticated group to access certain policy rules [Suite:openshift/conformance/parallel]

fail [github.com/openshift/origin/test/extended/authorization/rbac/groups_default_rules.go:196]: Jul  1 13:08:13.102: system:authenticated has extra permissions in namespace "":
{APIGroups:["console.openshift.io"], Resources:["consoleclidownloads"], Verbs:["get" "list" "watch"]}
{APIGroups:["console.openshift.io"], Resources:["consolelinks"], Verbs:["get" "list" "watch"]}
{APIGroups:["console.openshift.io"], Resources:["consolenotifications"], Verbs:["get" "list" "watch"]}

vrutkovs · 2019-07-03T09:59:11Z

/hold cancel
/approve

Looks fine to me

jsafrane · 2019-07-03T09:59:55Z

Removed WIP patches for rehearsals.

openshift-ci-robot · 2019-07-26T19:54:24Z

@jsafrane: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/rehearse/openshift/installer/master/e2e-restore-cluster-state	e8ecee4669ea87de455e4eb10887edd661b5aaec	link	`/test pj-rehearse`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

jsafrane · 2019-07-29T14:02:45Z

/assign @vrutkovs

jsafrane · 2019-09-03T14:57:56Z

/test pj-rehearse

vrutkovs

LGTM

vrutkovs · 2019-09-03T15:03:35Z

/lgtm
/hold

waiting for rehearses to pass

openshift-ci-robot · 2019-09-03T15:03:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jsafrane, vrutkovs
To complete the pull request process, please assign crawford
You can assign the PR to them by writing /assign @crawford in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

ci-operator/templates/openshift/installer/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jsafrane · 2019-09-04T07:34:31Z

Rehearsal went OK.

/assign @wking @smarterclayton
for approval

vrutkovs · 2019-09-04T15:45:38Z

/hold cancel

smarterclayton · 2019-09-04T16:01:04Z

/hold

I don't want this on for everyone. Let me review.

smarterclayton · 2019-09-04T16:56:47Z

ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

          queue /tmp/artifacts/must-gather/must-gather.log oc --insecure-skip-tls-verify adm must-gather --dest-dir /tmp/artifacts/must-gather

+          echo "Removing ssh-bastion ..."
+          queue /dev/null oc --insecure-skip-tls-verify --request-timeout=5s delete project testing-ssh-bastion


Why are you doing this? Why isn't this being torn down by the cluster tear down?

It was requested in #4161 (review)

Service load balancers aren't leaked.

smarterclayton · 2019-09-04T16:57:14Z

Upstream has chosen ssh as the access method

This statement is not true, upstream has been moving away from SSH for a long time. In general, tests that use SSH should not use SSH unless there is no other alternative. Disruptive (most of which won't work on OpenShift anyway because they encode assumptions about the shape of the OS) is the biggest place where that happens, and even most of those can be done via pods.

I would prefer to see the remaining tests moved away from SSH than turning this on globally for all test frameworks. I'm certainly ok with having this option be available for certain test suites (like disruptive) but opt-in is probably better, not on by default.

I think for this PR if you remove the automatic opt-in and leave the function there, which specific tests can use by adding it before the test runs (specifically disruptive, which needs to get enabled).

jsafrane · 2019-09-05T17:38:58Z

Upstream has been moving away from SSH for a long time

I haven't noticed any coordinated effort, either as KEP, issue or announced on kubernetes-dev. We (sig-storage) are still adding new tests that use ssh. I don't remember anyone reporting issues for that (with one exception, we use kubernetes-in-docker to tests CSI stuff and ssh does not work there; kubernetes/kubernetes#81751)

I think for this PR if you remove the automatic opt-in and leave the function there, which specific tests can use by adding it before the test runs (specifically disruptive, which needs to get enabled).

Node tests run as part of e2e-aws, I don't think it's a good idea to clutter the tests just for https://bugzilla.redhat.com/show_bug.cgi?id=1711600.

smarterclayton · 2019-09-05T17:58:22Z

You haven't noticed it but it has repeatedly been enforced at test level. You can't have conformance tests that require SSH. Large sets of tests have had SSH access removed.

jsafrane · 2019-09-05T18:01:32Z

it has repeatedly been enforced at test level

Any references? Again, no KEP, no announce, how are we supposed to know about SSH deprecation?

smarterclayton · 2019-09-05T18:04:14Z

We run almost every test in the suite. A year ago there were a hundred or more that depend on SSH. Now there are 3 or 4 in the core path.

jsafrane · 2019-09-05T18:07:58Z

I am not arguing that there tests were not reworked but the communication around. How are we supposed to know about SSH deprecation?

jsafrane · 2019-09-06T13:42:14Z

/close
Disruptive tests seem to be the only acceptable usage of SSH bastion and we don't run them, so this PR is useless.

openshift-ci-robot · 2019-09-06T13:42:15Z

@jsafrane: Closed this PR.

Details

In response to this:

/close
Disruptive tests seem to be the only acceptable usage of SSH bastion and we don't run them, so this PR is useless.

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.

smarterclayton · 2019-09-06T19:53:25Z

ci-operator/templates/openshift/installer/cluster-launch-installer-e2e.yaml

+          do
+            # AWS fills only .hostname of a service
+            BASTION_HOST=$(oc get service -n "${SSH_BASTION_NAMESPACE}" ssh-bastion -o jsonpath='{.status.loadBalancer.ingress[0].hostname}')
+            if [[ -n "${BASTION_HOST}" ]]; then break; fi


This logic should be in the script, let's get Eric to fix his bastion

smarterclayton · 2019-09-10T11:53:43Z

We’re about to start running disruptive tests from e2e and that suite does need SSH

smarterclayton · 2019-09-10T11:55:02Z

We also might end up with an “other” suite for some of the disabled tests - that’s a good spot to include SSH as wel

smarterclayton · 2019-09-25T17:33:09Z

I'm going to pull this code out and fix it up as part of getting the disruptive suite going.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 24, 2019

openshift-ci-robot requested review from tomassedovic and wking June 24, 2019 12:34

vrutkovs reviewed Jun 24, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019

jsafrane changed the title ~~Run ssh bastion during openshift-tests~~ WIP: Run ssh bastion during openshift-tests Jun 24, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2019

jsafrane force-pushed the enable-ssh-bastion branch 2 times, most recently from 9de3374 to a92564c Compare June 24, 2019 13:00

jsafrane mentioned this pull request Jun 24, 2019

Ignore ssh bastion namespace when checking for images openshift/origin#23252

Closed

openshift-ci-robot requested review from crawford and eparis June 24, 2019 16:36

derekwaynecarr reviewed Jun 25, 2019

View reviewed changes

jsafrane force-pushed the enable-ssh-bastion branch from a92564c to cab5e61 Compare July 1, 2019 08:08

jsafrane force-pushed the enable-ssh-bastion branch 3 times, most recently from 358eede to 8676108 Compare July 3, 2019 09:58

jsafrane changed the title ~~WIP: Run ssh bastion during openshift-tests~~ Run ssh bastion during openshift-tests Jul 3, 2019

openshift-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 Jul 3, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2019

openshift-ci-robot assigned vrutkovs Jul 29, 2019

vrutkovs approved these changes Sep 3, 2019

View reviewed changes

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 3, 2019

openshift-ci-robot assigned smarterclayton and wking Sep 4, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2019

smarterclayton reviewed Sep 4, 2019

View reviewed changes

openshift-ci-robot closed this Sep 6, 2019

smarterclayton reviewed Sep 6, 2019

View reviewed changes

Run ssh bastion during openshift-tests #4161

Run ssh bastion during openshift-tests #4161

Uh oh!

Conversation

jsafrane commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented Jun 24, 2019

Uh oh!

jsafrane commented Jun 24, 2019

Uh oh!

abhinavdahiya commented Jun 24, 2019

Uh oh!

jsafrane commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented Jun 25, 2019

Uh oh!

abhinavdahiya commented Jun 25, 2019

Uh oh!

jsafrane commented Jul 1, 2019

Uh oh!

jsafrane commented Jul 1, 2019

Uh oh!

vrutkovs commented Jul 3, 2019

Uh oh!

jsafrane commented Jul 3, 2019

Uh oh!

openshift-ci-robot commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsafrane commented Jul 29, 2019

Uh oh!

jsafrane commented Sep 3, 2019

Uh oh!

vrutkovs left a comment

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented Sep 3, 2019

Uh oh!

openshift-ci-robot commented Sep 3, 2019

Uh oh!

jsafrane commented Sep 4, 2019

Uh oh!

vrutkovs commented Sep 4, 2019

Uh oh!

smarterclayton commented Sep 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Sep 4, 2019

Uh oh!

jsafrane commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Sep 5, 2019

Uh oh!

jsafrane commented Sep 5, 2019

Uh oh!

smarterclayton commented Sep 5, 2019

Uh oh!

jsafrane commented Sep 5, 2019

Uh oh!

jsafrane commented Jun 24, 2019 •

edited

Loading

openshift-ci-robot commented Jul 26, 2019 •

edited

Loading

jsafrane commented Sep 5, 2019 •

edited

Loading