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.

Sign up for GitHub

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

Jump to bottom

Add e2e test on k8s 1.25.2 #13358

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Add e2e test on k8s 1.25.2 #13358

merged 1 commit into from
Oct 11, 2022

Conversation

nader-ziada
Copy link
Member

Fixes #

Proposed Changes

  • add testing on k8s 1.25.2 to the e2e action

Release Note

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 5, 2022
@codecov
Copy link

codecov bot commented Oct 5, 2022 •
edited
Loading

Codecov Report

Base: 86.54% // Head: 86.45% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (898bb1d) compared to base (636e121).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #13358      +/-   ##
==========================================
- Coverage   86.54%   86.45%   -0.09%     
==========================================
  Files         196      196              
  Lines       14549    14549              
==========================================
- Hits        12591    12578      -13     
- Misses       1660     1671      +11     
- Partials      298      300       +2
Impacted Files Coverage Δ
pkg/http/handler/timeout.go 84.76% <0.00%> (-6.63%) ⬇️
pkg/autoscaler/statserver/server.go 77.77% <0.00%> (-1.49%) ⬇️
pkg/reconciler/configuration/configuration.go 82.93% <0.00%> (-1.43%) ⬇️
pkg/reconciler/revision/background.go 90.90% <0.00%> (ø)
pkg/activator/net/revision_backends.go 92.77% <0.00%> (+0.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@psschwei
Copy link
Contributor

psschwei commented Oct 6, 2022

We'll need to bump metallb to the latest version for it to work with k8s v1.25, which also requires a little tweak in how we do the setup, something like:

# Install metallb
kubectl apply -f https://raw.githubusercontent.com/metallb/metallb/v0.13.5/config/manifests/metallb-native.yaml

# Add Layer 2 config
network=$(docker network inspect kind -f "{{(index .IPAM.Config 0).Subnet}}" | cut -d '.' -f1,2)
cat <<EOF | kubectl apply -f -
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
  name: first-pool
  namespace: metallb-system
spec:
  addresses:
  - $network.255.1-$network.255.250
---
apiVersion: metallb.io/v1beta1
kind: L2Advertisement
metadata:
  name: example
  namespace: metallb-system
EOF

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
jwcesign
jwcesign reviewed Oct 6, 2022
View reviewed changes
.github/workflows/kind-e2e.yaml Outdated
@@ -109,17 +109,17 @@ jobs:
# This is attempting to make it a bit clearer what's being tested.
# See: https://github.com/kubernetes-sigs/kind/releases
# https://hub.docker.com/r/kindest/node/tags
- k8s-version: v1.22.9
- k8s-version: v1.23.12
kind-version: v0.14.0
Copy link
Member

Choose a reason for hiding this comment

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

Should we update all kind version to v0.16.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

will try that

@jwcesign
Copy link
Member

jwcesign commented Oct 6, 2022 •
edited
Loading

We'll need to bump metallb to the latest version for it to work with k8s v1.25, which also requires a little tweak in how we do the setup, something like:

We may need to disable the webhook-mode because there is a bug:metallb/metallb#1597 like https://github.com/karmada-io/karmada/pull/2560/files, it happens sometimes

@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 6, 2022
@jwcesign
Copy link
Member

jwcesign commented Oct 6, 2022 •
edited
Loading

The error is exactly what I meet before:

Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "ipaddresspoolvalidationwebhook.metallb.io": failed to call webhook: Post "[https://webhook-service.metallb-system.svc:443/validate-metallb-io-v1beta1-ipaddresspool?timeout=10s](https://webhook-service.metallb-system.svc/validate-metallb-io-v1beta1-ipaddresspool?timeout=10s)": dial tcp 10.96.116.26:443: connect: connection refused
Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "l2advertisementvalidationwebhook.metallb.io": failed to call webhook: Post "[https://webhook-service.metallb-system.svc:443/validate-metallb-io-v1beta1-l2advertisement?timeout=10s](https://webhook-service.metallb-system.svc/validate-metallb-io-v1beta1-l2advertisement?timeout=10s)": dial tcp 10.96.116.26:443: connect: connection refused

Should disable the webhook mode, we are sure the yaml is right, so disable the webhook mode is ok.

@nader-ziada
Copy link
Member Author

The error is exactly what I meet before:

Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "ipaddresspoolvalidationwebhook.metallb.io": failed to call webhook: Post "[https://webhook-service.metallb-system.svc:443/validate-metallb-io-v1beta1-ipaddresspool?timeout=10s](https://webhook-service.metallb-system.svc/validate-metallb-io-v1beta1-ipaddresspool?timeout=10s)": dial tcp 10.96.116.26:443: connect: connection refused
Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "l2advertisementvalidationwebhook.metallb.io": failed to call webhook: Post "[https://webhook-service.metallb-system.svc:443/validate-metallb-io-v1beta1-l2advertisement?timeout=10s](https://webhook-service.metallb-system.svc/validate-metallb-io-v1beta1-l2advertisement?timeout=10s)": dial tcp 10.96.116.26:443: connect: connection refused

Should disable the webhook mode, we are sure the yaml is right, so disable the webhook mode is ok.

yes, I was trying to do that, but I guess the disable code was wrong, trying again now

@nader-ziada
Copy link
Member Author

something is up with net-istio and 1.25, @nak3 I think net-isito might need a to bump the version of isito?

@nak3
Copy link
Contributor

nak3 commented Oct 7, 2022

Sorry it is an istioctl manifest generate command issue. I opened knative-extensions/net-istio#993 but need some time to fix it (or we need some workaround.).
cc @AngeloDanducci

@nak3
Copy link
Contributor

nak3 commented Oct 7, 2022

I sent the workaround knative-extensions/net-istio#994 for now.

@nader-ziada
Copy link
Member Author

/retest

@psschwei
Copy link
Contributor

psschwei commented Oct 7, 2022

/easycla

@nader-ziada nader-ziada changed the title [WIP] Add e2e test on k8s 1.25.2 Add e2e test on k8s 1.25.2 Oct 7, 2022
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2022
@nader-ziada
Copy link
Member Author

/retest

1 similar comment
@nader-ziada
Copy link
Member Author

/retest

@nak3
Copy link
Contributor

nak3 commented Oct 10, 2022

I think #13375 needs to be merged.

@nader-ziada
Copy link
Member Author

lets try again now that 13375 (net-isitio update) is merged

/retest

@psschwei
Copy link
Contributor

looks like we're running into carvel-dev/kapp#620

@nak3
Copy link
Contributor

nak3 commented Oct 11, 2022 •
edited
Loading

The latest log still displays old sha 99e1aa7ff2b76a63234b1cf8d20f0c799e59c5e0.
The script is seeing this line https://github.com/knative/serving/blob/main/third_party/istio-latest/net-istio.yaml#L1 but the sha is still before #13375
It is worth to try to rebase this PR?

2022-10-10T16:12:18.4394004Z + local sha=99e1aa7ff2b76a63234b1cf8d20f0c799e59c5e0
2022-10-10T16:12:18.4395075Z Got NET_ISTIO_COMMIT from /home/runner/work/serving/serving/third_party/istio-latest/net-istio.yaml: 99e1aa7ff2b76a63234b1cf8d20f0c799e59c5e0
2022-10-10T16:12:18.4397336Z + [[ -z 99e1aa7ff2b76a63234b1cf8d20f0c799e59c5e0 ]]
2022-10-10T16:12:18.4399004Z + echo 'Got NET_ISTIO_COMMIT from /home/runner/work/serving/serving/third_party/istio-latest/net-istio.yaml: 99e1aa7ff2b76a63234b1cf8d20f0c799e59c5e0'

@nader-ziada
Copy link
Member Author

just did a rebase now

psschwei
psschwei reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2022
@psschwei psschwei mentioned this pull request Oct 11, 2022
Closed
@knative-prow
Copy link

knative-prow bot commented Oct 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada, psschwei

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2022
@knative-prow knative-prow bot merged commit 9f882f1 into knative:main Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psschwei psschwei psschwei left review comments

@jwcesign jwcesign jwcesign left review comments

Assignees

@psschwei psschwei

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nader-ziada @psschwei @jwcesign @nak3 @knative-prow-robot