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

Add testing for minimal profile with knative #19675

Closed

Conversation

howardjohn
Copy link
Member

@howardjohn howardjohn commented Dec 18, 2019

This is a step towards merging installer/operator into istio repo. In
order to do this, we parity on test coverage. The installer has
substantial testing of minimal profile, so its important we port this
over (including Knative minimal profile testing!).

This does make a change to the old helm templates to allow Ingress
controller using Pilot (not sure how this was not possible before...).
This is because we don't have all tests running using the new installer
yet, and ideally we want the same set of tests to run on old and new, so
I think it makes sense to make this change since its minimal and
undocumented.

I've also added some additional debug logging here, since its pretty useful to figure out why the test is sitting there doing nothing for 10 minutes.

@howardjohn howardjohn requested review from a team as code owners December 18, 2019 19:34
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 18, 2019
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 18, 2019
@howardjohn howardjohn changed the title Run ingress test using minimal profile WIP Run ingress test using minimal profile Dec 19, 2019
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Dec 19, 2019
@howardjohn howardjohn changed the title WIP Run ingress test using minimal profile Run ingress test using minimal profile, add knative smoke test Dec 19, 2019
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Dec 19, 2019
@howardjohn howardjohn changed the title Run ingress test using minimal profile, add knative smoke test Add testing for minimal profile with knative Dec 19, 2019
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2019
Copy link
Member

@sdake sdake left a comment

Choose a reason for hiding this comment

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

In theory, generated code or manifests should be generated by make gen.

@@ -0,0 +1,396 @@
apiVersion: apiextensions.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

@howardjohn this file (to me) appears to be misnamed. Would you consider knative-crds.yaml?

@@ -0,0 +1,1023 @@
apiVersion: v1
Copy link
Member

Choose a reason for hiding this comment

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

knative-serving.yaml would be a better name here.

A make target to generate this testdata would be helpful as well. It isn't clear how this testdata was generated. If knative revs, how do we keep track of the new application-specific manifest?

e.g. make gen-test-app-manifests which is tied into make gen. Perhaps there could be a better target name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed and added a comment where it comes from.

I don't think we want/need/can this to be a part of gen, it is manually updated when required

Copy link
Contributor

@nmittler nmittler left a comment

Choose a reason for hiding this comment

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

lgtm overall ... will approve after you address comments from @sdake

metadata:
annotations:
cluster-autoscaler.kubernetes.io/safe-to-evict: "false"
sidecar.istio.io/inject: "true"
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, this annotation is dropped in knative serving as we control it by namespace's.

https://github.com/knative/serving/pull/6413/files#diff-6daded100a43545c2331ebe741ea6a92L31

@nak3
Copy link
Member

nak3 commented Jan 7, 2020

This test looks good. But is it possible to expand the timeout (30sec as per logs)? I am wondering that deploying knative's system containers + application within 30sec is not enough.

2019-12-20T17:53:18.230177Z	info	CI	Applying YAML: kubectl apply --kubeconfig=/home/.kube/config -n knative-1-8102 -f /logs/artifacts/minimal-test-52926969dbae4b1b86/_suite_context/env-kube-768482011/istio-kube-accessor-987922885/accessor_206457236
2019-12-20T17:53:48.570496Z	info	CI	Deleting YAML file: /logs/artifacts/minimal-test-52926969dbae4b1b86/_suite_context/env-kube-768482011/istio-kube-accessor-987922885/accessor_860902883

@howardjohn
Copy link
Member Author

This test looks good. But is it possible to expand the timeout (30sec as per logs)? I am wondering that deploying knative's system containers + application within 30sec is not enough.

Running this locally I saw when it failed it was permanent, ill try this with 3min timeout though

@howardjohn
Copy link
Member Author

@nak3 with long timeout this still fails

@nak3
Copy link
Member

nak3 commented Jan 15, 2020

@howardjohn Thank you. Timeout is no problem, then I am suspecting the test image (istio-testing/app:istio-testing).

Knative resolves image tags to a digest when creating a revision. You can refer to the docs or this slide for more detail. In fact, the image istio-testing/app:istio-testing cannot be resolved on my local env by this error:

$ kubectl create -f accessor_738217051
$ kubectl get event
  ...
1s          Warning   InternalError         revision/knative-service-mpqg2                           failed to resolve image to digest: failed to fetch image information: MANIFEST_UNKNOWN: Failed to fetch "istio-testing" from request "/v2/istio-testing/app/manifests/istio-testing".

For the solution, I could find gcr.io/istio-testing/app:latest in public gcr.io, so it worked fine by replacing the image as:

cat <<EOF | kubectl apply -f -
apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: knative-service
spec:
  runLatest:
    configuration:
      revisionTemplate:
        spec:
          container:
            image: gcr.io/istio-testing/app:latest
EOF

Is it possible to test with image: gcr.io/istio-testing/app:latest? Otherwise, I think we need to check more pod's logs, event logs or yamls. (I could find some info in this directory but maybe that's all?)

@howardjohn
Copy link
Member Author

howardjohn commented Jan 15, 2020 via email

@nak3
Copy link
Member

nak3 commented Jan 15, 2020

Ah! it is a local image, then yes, it fails to deploy knative apps by default. You can refer to this thread especially this comment knative/serving#6101 (comment) for the instruction. You need additional configuration.

This is a step towards merging installer/operator into istio repo. In
order to do this, we parity on test coverage. The installer has
substantial testing of minimal profile, so its important we port this
over (including Knative minimal profile testing!).
@howardjohn
Copy link
Member Author

@nak3 I think i configured things correctly here but failing the same. Thanks for the reference though

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 13, 2020
@istio-testing
Copy link
Collaborator

@howardjohn: PR needs rebase.

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.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Feb 15, 2020
@istio-testing
Copy link
Collaborator

@howardjohn: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
gencheck_istio 8c25f4c link /test gencheck_istio
integ-galley-k8s-tests_istio 8c25f4c link /test integ-galley-k8s-tests_istio
integ-pilot-k8s-tests_istio 8c25f4c link /test integ-pilot-k8s-tests_istio
integ-conformance-local-tests_istio 8c25f4c link /test integ-conformance-local-tests_istio
integ-conformance-k8s-tests_istio 8c25f4c link /test integ-conformance-k8s-tests_istio
integ-distroless-k8s-tests_istio 8c25f4c link /test integ-distroless-k8s-tests_istio
integ-local-tests_istio 8c25f4c link /test integ-local-tests_istio

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.

@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-01-16. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge/hold Block automatic merging of a PR. lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants