Skip to content

Backport Best effort Istio probing to 0.11 - #7597

Merged
knative-prow-robot merged 1 commit into
knative:release-0.11from
JRBANCEL:cherry-pick-probing
Apr 17, 2020
Merged

Backport Best effort Istio probing to 0.11#7597
knative-prow-robot merged 1 commit into
knative:release-0.11from
JRBANCEL:cherry-pick-probing

Conversation

@JRBANCEL

Copy link
Copy Markdown
Contributor

Backport of #6962 to 0.11.
This has been asked by https://github.com/kubeflow/kubeflow.

/cc @animeshsingh

@knative-prow-robot

Copy link
Copy Markdown
Contributor

@JRBANCEL: GitHub didn't allow me to request PR reviews from the following users: animeshsingh.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

Backport of #6962 to 0.11.
This has been asked by https://github.com/kubeflow/kubeflow.

/cc @animeshsingh

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.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 15, 2020
@JRBANCEL JRBANCEL changed the title Backport Best effort Istio probing Backport Best effort Istio probing to 0.11 Apr 15, 2020
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/networking labels Apr 15, 2020
@knative-metrics-robot

Copy link
Copy Markdown

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/ingress/status.go Do not exist 92.5%

@tcnghia

tcnghia commented Apr 15, 2020

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2020
@knative-prow-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JRBANCEL, tcnghia

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

The pull request process is described here

Details 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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2020
@knative-test-reporter-robot

Copy link
Copy Markdown

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-integration-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/conformance/api/v1.TestUpdateConfigurationMetadata
test/conformance/api/v1.TestContainerErrorMsg
test/conformance/api/v1.TestContainerExitingMsg
test/conformance/api/v1.TestTranslation
test/conformance/api/v1.TestCustomResourcesLimits
test/conformance/api/v1.TestServiceAccountValidation
test/conformance/api/v1.TestService
test/conformance/api/v1.TestServiceBYOName

and 34 more.

@knative-test-reporter-robot

Copy link
Copy Markdown

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-upgrade-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-upgrade-tests:

test/upgrade.TestRunLatestServicePreUpgrade
test/upgrade.TestRunLatestServicePreUpgradeAndScaleToZero
test/upgrade.TestBYORevisionPreUpgrade

@JRBANCEL

Copy link
Copy Markdown
Contributor Author

/retest

@mattmoor mattmoor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm
/assign @tcnghia

@tcnghia

tcnghia commented Apr 15, 2020

Copy link
Copy Markdown
Contributor

I lgtmed the change, but not sure why the tests are complaining. All of them, so this may be a areal problem, just test flakes.

@JRBANCEL

Copy link
Copy Markdown
Contributor Author

The tests are failing because namespace/serving-tests is not present, and I don't see it being installed:

>> Creating test resources (test/config/)
configmap/config-logging configured

compare that with a run on master:

>> Creating test resources (/tmp/ci-2020-04-15-17-42-26-4ZeEnidLZR/test/config/)
namespace/serving-tests created
namespace/serving-tests-alt created
namespace/serving-tests-security created
configmap/conformance-test-configmap created
secret/conformance-test-secret created
configmap/config-logging configured

@coryrc @chaodaiG any ideas?

@chaodaiG

Copy link
Copy Markdown
Contributor

The tests are failing because namespace/serving-tests is not present, and I don't see it being installed:

>> Creating test resources (test/config/)
configmap/config-logging configured

compare that with a run on master:

>> Creating test resources (/tmp/ci-2020-04-15-17-42-26-4ZeEnidLZR/test/config/)
namespace/serving-tests created
namespace/serving-tests-alt created
namespace/serving-tests-security created
configmap/conformance-test-configmap created
secret/conformance-test-secret created
configmap/config-logging configured

@coryrc @chaodaiG any ideas?

0.11 release branch CI Is broken too: https://testgrid.knative.dev/knative-0.11#serving-continuous

@JRBANCEL

Copy link
Copy Markdown
Contributor Author

@chaodaiG is anyone looking at fixing it?

@chaodaiG

chaodaiG commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

I'll take a quick look. It first started on 02/22/2020: https://prow.knative.dev/view/gcs/knative-prow/logs/ci-knative-serving-0.11-continuous/1231138661567827968/

The run on 02/21/2020 passed: https://prow.knative.dev/view/gcs/knative-prow/logs/ci-knative-serving-0.11-continuous/1230776268409016320/

but these 2 built on the same commit, so there must be some env changed

@chaodaiG

Copy link
Copy Markdown
Contributor

This is very likely caused by Prow test image update happened on 02/21/2020, from this PR: knative/test-infra#1727

@chizhg , we should re-evaluate the priority of using versioned tag instead of a fixed tag for every single Prow job: knative/test-infra#1556

In short, I have a suspicion that this is due to ko update, will take a little more look to see if there is an easier fix than changing Prow jobs

@chizhg

chizhg commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

This is very likely caused by Prow test image update happened on 02/21/2020, from this PR: knative/test-infra#1727

@chizhg , we should re-evaluate the priority of using versioned tag instead of a fixed tag for every single Prow job: knative/test-infra#1556

In short, I have a suspicion that this is due to ko update, will take a little more look to see if there is an easier fix than changing Prow jobs

There are two issues here:

The first one is indeed related to ko update. When running ko apply --selector=networking.knative.dev/ingress-provider!=istio -f test/config/, the previous ko version will build and create all the resources, but ko 0.3.0 will skip most of them (in my test, only configmap/config-logging is created)

The second one is we are generating Prow jobs for all release branches, e.g. the auto-tls-test job was added in release-0.13, but we are also generating the Prow job for release-0.11 and release-0.12, and the job will always fail as the two branches does not have the script.

@chaodaiG

Copy link
Copy Markdown
Contributor

This is very likely caused by Prow test image update happened on 02/21/2020, from this PR: knative/test-infra#1727
@chizhg , we should re-evaluate the priority of using versioned tag instead of a fixed tag for every single Prow job: knative/test-infra#1556
In short, I have a suspicion that this is due to ko update, will take a little more look to see if there is an easier fix than changing Prow jobs

There are two issues here:

The first one is indeed related to ko update. When running ko apply --selector=networking.knative.dev/ingress-provider!=istio -f test/config/, the previous ko version will build and create all the resources, but ko 0.3.0 will skip most of them (in my test, only configmap/config-logging is created)

The second one is we are generating Prow jobs for all release branches, e.g. the auto-tls-test job was added in release-0.13, but we are also generating the Prow job for release-0.11 and release-0.12, and the job will always fail as the two branches does not have the script.

We can kindly ignore auto-tls test in this case, but we should fix integration test. What's mystery to me is why the same ko created namespaces in master branch, the only difference is --selector doesn't exist in master branch

@chizhg

chizhg commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

This is very likely caused by Prow test image update happened on 02/21/2020, from this PR: knative/test-infra#1727
@chizhg , we should re-evaluate the priority of using versioned tag instead of a fixed tag for every single Prow job: knative/test-infra#1556
In short, I have a suspicion that this is due to ko update, will take a little more look to see if there is an easier fix than changing Prow jobs

There are two issues here:
The first one is indeed related to ko update. When running ko apply --selector=networking.knative.dev/ingress-provider!=istio -f test/config/, the previous ko version will build and create all the resources, but ko 0.3.0 will skip most of them (in my test, only configmap/config-logging is created)
The second one is we are generating Prow jobs for all release branches, e.g. the auto-tls-test job was added in release-0.13, but we are also generating the Prow job for release-0.11 and release-0.12, and the job will always fail as the two branches does not have the script.

We can kindly ignore auto-tls test in this case, but we should fix integration test. What's mystery to me is why the same ko created namespaces in master branch, the only difference is --selector doesn't exist in master branch

--selector caused the difference. I tried ko apply -f test/config/ with ko 0.3.0, and everything was created.
I think we should also fix the autotls-tests in order to get the PR merged? It's a required job I believe.

@chaodaiG

Copy link
Copy Markdown
Contributor

--selector caused the difference. I tried ko apply -f test/config/ with ko 0.3.0, and everything was created.
I think we should also fix the autotls-tests in order to get the PR merged? It's a required job I believe.

I don't think we can just simply remove --selector in 0.11 branch, suggest using a pinned image tag for running old release branch jobs.

autotls test is easy, just add an empty script to make it pass

@chaodaiG

Copy link
Copy Markdown
Contributor

another note, I created 2 PRs for testing the theory of ko, one with latest ko #7623 and the other with ko from Nov 2019: #7625, none of them worked. So there might be something else changed in the image that's related.

@chizhg

chizhg commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

I think an easier way to fix here, as well as in the long term would be installing ko and pin the version in the script. Since we are going to release test-infra together with other knative projects, we can make sure the tests for each release branch are using the compatible version of ko. The same for other tools that do not support backward compatibility, e.g. gcloud and kubectl.

I agree we can add an empty auto-tls script to fix the issue here, but in the long term, the config-generator needs to support setting branches in the job level.

@chaodaiG

chaodaiG commented Apr 17, 2020 via email

Copy link
Copy Markdown
Contributor

@chizhg

chizhg commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

/retest

@yuzisun

yuzisun commented Apr 17, 2020

Copy link
Copy Markdown

@JRBANCEL Is this going to generate a new minor version of 0.11 ?

@tcnghia

tcnghia commented Apr 17, 2020

Copy link
Copy Markdown
Contributor

Yes, this will be in 0.11.2, released next Tuesday.

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. area/API API objects and controllers area/networking cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants