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 e2e test to verify knative examples #1250

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

savitaashture
Copy link
Contributor

Changes

Fixes #1089
PR adds changes to install Knative Serving and runs custom-resource examples for both v1alpha1 and v1beta1

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Triggers now verify Knative based tests

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 25, 2021
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 25, 2021
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

1 similar comment
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

@savitaashture savitaashture changed the title Add e2e test to verify knative examples [WIP] Add e2e test to verify knative examples Oct 26, 2021
@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2021
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

@savitaashture savitaashture force-pushed the e2e-knative-test branch 4 times, most recently from 0878a6c to b706ade Compare October 26, 2021 11:50
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

1 similar comment
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

Comment on lines 32 to 48
kubectl apply -f https://github.com/knative/serving/releases/download/v0.26.0/serving-crds.yaml
kubectl apply -f https://github.com/knative/serving/releases/download/v0.26.0/serving-core.yaml

kubectl apply -f https://github.com/knative/net-kourier/releases/download/v0.26.0/kourier.yaml

kubectl patch configmap/config-network \
--namespace knative-serving \
--type merge \
--patch '{"data":{"ingress.class":"kourier.ingress.networking.knative.dev"}}'

# To configure Magic DNS to access Knative Service.
kubectl apply -f https://github.com/knative/serving/releases/download/v0.26.0/serving-default-domain.yaml

# Wait for pods to be running in the namespaces we are deploying to
wait_until_pods_running knative-serving || fail_test "Knative Serving did not come up"
# Wait for jobs to be completed in the namespaces we are deploying to
wait_until_batch_job_complete knative-serving || fail_test "Knative Serving jobs did not completed"
Copy link
Contributor Author

@savitaashture savitaashture Oct 26, 2021

Choose a reason for hiding this comment

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

@mattmoor I m installing Knative Serving following installation doc and i see one issue that default-domain job is not going to completed state am i doing something wrong here 🤔

Tried samething on GCP cluster locally and it worked (default-domain job went to completed state).

Any pointers would be grateful 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Depends on the system you are using. For instance on KinD you need metallb for this stuff to work.

Can you share more about how it is failing?

Copy link
Contributor Author

@savitaashture savitaashture Oct 27, 2021

Choose a reason for hiding this comment

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

CI is failing with below error

++ kubectl get jobs -n knative-serving --no-headers '-ocustom-columns=n:{.metadata.name},c:{.spec.completions},s:{.status.succeeded}'
+ local 'jobs=default-domain   1     <none>'
++ echo 'default-domain   1     <none>'
++ awk '{if ($2!=$3) print $0}'
++ wc -l
+ local not_complete=1
+ [[ 1 -eq 0 ]]
+ echo -n .
.+ sleep 2
+ echo -e '\n\nERROR: timeout waiting for jobs to complete\ndefault-domain   1     <none>'


ERROR: timeout waiting for jobs to complete
default-domain   1     <none>
+ return 1
+ fail_test 'Knative Serving jobs did not completed'

because i am using below function to wait until jobs are completed

wait_until_batch_job_complete knative-serving || fail_test "Knative Serving jobs did not completed"

Now i have doubt that does our CI cluster will have loadbalancer attached 🤔
/cc @dibyom

Copy link
Contributor Author

@savitaashture savitaashture Oct 27, 2021

Choose a reason for hiding this comment

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

when i install on mu local GKE cluster where loadbalancer is attached the default-domain pod goes to completed state by using the public ip

Logs:

kubectl logs -f default-domain-nq8lk -n knative-serving
W1027 06:07:37.953764       1 client_config.go:615] Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
{"level":"info","ts":1635314868.1755047,"logger":"fallback.default-domain","caller":"default-domain/main.go:225","msg":"Updated default domain to: 34.66.182.202.sslip.io"}

@savitaashture savitaashture force-pushed the e2e-knative-test branch 2 times, most recently from 47bbdd0 to 007767a Compare October 28, 2021 09:12
@savitaashture savitaashture changed the title [WIP] Add e2e test to verify knative examples Add e2e test to verify knative examples Oct 28, 2021
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2021
@savitaashture
Copy link
Contributor Author

@dibyom @wlynch i have updated PR and its ready to review please do take a look at it
Thank you

@savitaashture savitaashture force-pushed the e2e-knative-test branch 2 times, most recently from acffc83 to cd4694e Compare October 28, 2021 12:13
@savitaashture
Copy link
Contributor Author

Hi @dibyom could you take a look at this PR please

Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Thanks @savitaashture Looks good overall. My main comment is can we this logic into e2e-test-examples.sh itself instead of creating a new file and copying stuff over? i.e. we can add in a couple of new functions in that file itself (curl_knative_service, install_knative_serving) and then add a new function called run_knative_tests that is basically the main from this file?

curl -v \
-H 'X-GitHub-Event: pull_request' \
-H 'X-Hub-Signature: sha1=ba0cdc263b3492a74b601d240c27efe81c4720cb' \
-H 'Content-Type: application/json' \
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in a curl.sh file like we do for other examples.

@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 13, 2021
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

1 similar comment
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

@savitaashture
Copy link
Contributor Author

@dibyom I have addressed comments PTAL

@dibyom
Copy link
Member

dibyom commented Nov 16, 2021

Looks good. Thanks!

/approve
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2021
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2021
@tekton-robot tekton-robot merged commit a488bf7 into tektoncd:main Nov 16, 2021
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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create integration test run for knative eventlistener service
4 participants