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

Rootless Kubeflow part one (istio-cni) #2455

Merged
merged 32 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
c22ca62
Proof of concept
juliusvonkohout May 9, 2023
7260176
remove legacy stuff
juliusvonkohout May 9, 2023
eba9538
Update README.md
juliusvonkohout May 9, 2023
c98af03
Upgrade istio to 1.18.1
tzstoyanov Jul 26, 2023
2d5d31b
Remove legacy stuff in 1.18
juliusvonkohout Jul 26, 2023
f184921
Update istio 1.17.3 documentation and enable patches
juliusvonkohout Jul 26, 2023
64bffe9
restore 1.6 and add knative-local-gateway comment
juliusvonkohout Jul 26, 2023
e4c29a2
Update kustomization.yaml
juliusvonkohout Jul 26, 2023
b321341
Update disable-debugging.yaml
juliusvonkohout Jul 26, 2023
10b0599
Update kustomization.yaml
juliusvonkohout Jul 26, 2023
a14531a
update to istio 1.17.5
juliusvonkohout Jul 26, 2023
213ac99
fix patches
juliusvonkohout Jul 26, 2023
62bae7d
kustomize is unhappy...
juliusvonkohout Jul 26, 2023
2fd015a
one patch per file
juliusvonkohout Jul 26, 2023
11a8ed3
Update istio-ingressgateway-remove-pdb.yaml
juliusvonkohout Jul 26, 2023
773e28e
Update kustomization.yaml
juliusvonkohout Jul 26, 2023
f9d3ada
Add istio 1.17.5 CNI installation
tzstoyanov Jul 27, 2023
5751788
delete istio-cni-1-16
juliusvonkohout Jul 27, 2023
9e67d3c
Update disable-debugging.yaml
juliusvonkohout Jul 27, 2023
37e6740
add --set components.cni.enabled=true --set components.cni.namespace=…
juliusvonkohout Jul 27, 2023
4c9a896
add tests for istio-cni
juliusvonkohout Jul 27, 2023
7d1ff92
move PSP and PSS to /contrib
juliusvonkohout Jul 27, 2023
8c51604
Use default CPU resources
tzstoyanov Jul 27, 2023
e42b26b
Update README.md
juliusvonkohout Jul 27, 2023
e45c674
Do not install istio in one namespace
tzstoyanov Jul 28, 2023
bd8f55c
Add github action for Istio CNI testing
tzstoyanov Jul 28, 2023
e25b0ef
readd istiod-remove-pdb
juliusvonkohout Aug 11, 2023
124c5cc
we will do /contrib changes in a new PR with capital one
juliusvonkohout Aug 11, 2023
f01d9e7
Trigger Istio CNI github action on PR
tzstoyanov Aug 14, 2023
59f3688
Update tests/gh-actions/install_knative-cni.sh
juliusvonkohout Aug 16, 2023
a893541
Change the trigger for the Istio CNI github action
tzstoyanov Aug 17, 2023
5fd16d0
Update README.md
juliusvonkohout Aug 17, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions .github/workflows/kserve_kind_cni_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
name: Build & Apply KServe manifests in KinD, using istio CNI
on:
pull_request:
paths:
- contrib/kserve/**
- common/knative/**
workflow_dispatch:
Copy link
Member

Choose a reason for hiding this comment

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

There should be a trigger here based on changes to specific directories, not just workflow_dispatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tzstoyanov will check this on monday

Copy link
Contributor

Choose a reason for hiding this comment

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

As this Istio CNI functionality is still experimental and not enabled by default yet, I added only a manual trigger of the github action. However, added also an automatic trigger on every change that uses Istio CNI.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand the trigger correctly, only people with write access will have access to trigger the workflow based on workflow_dispatch. So, not sure if it's necessary to keep

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but this would be the easiest way to run Istio CNI tests. Lets keep it at least for one release, until Istio CNI is not enabled by default. We can remove the manual trigger at any time.

Copy link
Member

Choose a reason for hiding this comment

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

Should this test be triggered when there is a change to istio-cni-*? Can we trigger the test to ensure it runs successfully?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that makes more sense than the current logic. Changed the trigger action, thanks Anna.


jobs:
build:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v3

- name: Install KinD
run: ./tests/gh-actions/install_kind.sh

- name: Create KinD Cluster
run: kind create cluster --config tests/gh-actions/kind-cluster.yaml

- name: Install kustomize
run: ./tests/gh-actions/install_kustomize.sh

- name: Create kubeflow namespace
run: kustomize build common/kubeflow-namespace/base | kubectl apply -f -

- name: Install Istio CNI
run: ./tests/gh-actions/install_istio-cni.sh

- name: Install cert-manager
run: ./tests/gh-actions/install_cert_manager.sh

- name: Install knative CNI
run: ./tests/gh-actions/install_knative-cni.sh

- name: Build & Apply manifests
run: ./tests/gh-actions/install_kserve.sh

- name: Create test namespace
run: kubectl create ns kserve-test

- name: Setup python 3.9
uses: actions/setup-python@v4
with:
python-version: 3.9

- name: Install test dependencies
run: pip install -r ./contrib/kserve/tests/requirements.txt

- name: Port forward
run: |
INGRESS_GATEWAY_SERVICE=$(kubectl get svc --namespace istio-system --selector="app=istio-ingressgateway" --output jsonpath='{.items[0].metadata.name}')
nohup kubectl port-forward --namespace istio-system svc/${INGRESS_GATEWAY_SERVICE} 8080:80 &

- name: Run kserve tests
run: |
export KSERVE_INGRESS_HOST_PORT=localhost:8080
cd ./contrib/kserve/tests && pytest .

- name: Run kserve models webapp test
run: |
kubectl wait --for=condition=Available --timeout=300s -n kubeflow deployment/kserve-models-web-app
3 changes: 1 addition & 2 deletions common/istio-1-16/istio-install/base/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,4 @@ patchesStrategicMerge:
- patches/disable-debugging.yaml
# Disable this patch until we upgrade to kustomize to v4+
# see https://github.com/kubeflow/manifests/issues/2325#issuecomment-1323909056
# - patches/remove-pdb.yaml

# - patches/remove-pdb.yaml
47 changes: 10 additions & 37 deletions common/istio-1-17/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,22 @@ In this section, we explain how to upgrade our istio kustomize packages
by leveraging `istioctl`. Assuming the new version is `X.Y.Z` and the
old version is `X1.Y1.Z1`:

1. Make a copy of the old istio manifests tree, which will become the
1. Make a copy of the old istio manifests tree, which will become the
kustomization for the new Istio version:

$ export MANIFESTS_SRC=<path/to/manifests/repo>
$ export ISTIO_OLD=$MANIFESTS_SRC/common/istio-X1-Y1
$ export ISTIO_NEW=$MANIFESTS_SRC/common/istio-X-Y
$ cp -a $ISTIO_OLD $ISTIO_NEW

2. Download `istioctl` for version `X.Y.Z`:
2. Download `istioctl` for version `X.Y.Z`:

$ ISTIO_VERSION="X.Y.Z"
$ wget "https://github.com/istio/istio/releases/download/${ISTIO_VERSION}/istio-${ISTIO_VERSION}-linux-amd64.tar.gz"
$ tar xvfz istio-${ISTIO_VERSION}-linux-amd64.tar.gz
# sudo mv istio-${ISTIO_VERSION}/bin/istioctl /usr/local/bin/istioctl

3. Use `istioctl` to generate an `IstioOperator` resource, the
3. Use `istioctl` to generate an `IstioOperator` resource, the
CustomResource used to describe the Istio Control Plane:

$ cd $ISTIO_NEW
Expand All @@ -32,13 +32,12 @@ old version is `X1.Y1.Z1`:
---
**NOTE**

`istioctl` comes with a bunch of [predefined
profiles](https://istio.io/v1.9/docs/setup/additional-setup/config-profiles/)
`istioctl` comes with a bunch of [predefined profiles](https://istio.io/latest/docs/setup/additional-setup/config-profiles/)
(`default`, `demo`, `minimal`, etc.). The `default` profile is installed by default.

---

4. Generate manifests and add them to their respective packages. We
4. Generate manifests and add them to their respective packages. We
will generate manifests using `istioctl`, the
`profile.yaml` file from upstream and the
`profile-overlay.yaml` file that contains our desired
Expand All @@ -51,6 +50,7 @@ old version is `X1.Y1.Z1`:
$ mv $ISTIO_NEW/crd.yaml $ISTIO_NEW/istio-crds/base
$ mv $ISTIO_NEW/install.yaml $ISTIO_NEW/istio-install/base
$ mv $ISTIO_NEW/cluster-local-gateway.yaml $ISTIO_NEW/cluster-local-gateway/base
$ rm dump.yaml

---
**NOTE**
Expand All @@ -62,45 +62,17 @@ old version is `X1.Y1.Z1`:
detect default settings. Ensure you have a target cluster ready before running the above commands.
We set this flag because `istioctl manifest generate` generates manifest files with resources that are no
longer supported in Kubernetes 1.25 (`policy/v1beta1`). See: https://github.com/istio/istio/issues/41220

---

5. Remove PodDisruptionBudget from `istio-install` and `cluster-local-gateway` kustomizations.
See https://github.com/istio/istio/issues/12602 and https://github.com/istio/istio/issues/24000

Until now we have used two patches:
- `common/istio-1-17/istio-install/base/patches/remove-pdb.yaml`
- `common/istio-1-17/cluster-local-gateway/base/patches/remove-pdb.yaml`

The above patches do not work with kustomize v3.2.0 as it doesn't have the appropriate
openapi schemas for the policy/v1 API version resources. This is fixed in kustomize v4+.
See https://github.com/kubernetes-sigs/kustomize/issues/3694#issuecomment-799700607 and
https://github.com/kubernetes-sigs/kustomize/issues/4495

A temporary workaround is to use the following instructions to manually delete the PodDisruptionBudget resources with `yq`:

$ yq eval -i 'select((.kind == "PodDisruptionBudget" and .metadata.name == "cluster-local-gateway") | not)' common/istio-1-17/cluster-local-gateway/base/cluster-local-gateway.yaml
$ yq eval -i 'select((.kind == "PodDisruptionBudget" and .metadata.name == "istio-ingressgateway") | not)' common/istio-1-17/istio-install/base/install.yaml
$ yq eval -i 'select((.kind == "PodDisruptionBudget" and .metadata.name == "istiod") | not)' common/istio-1-17/istio-install/base/install.yaml

---
**NOTE**

NOTE: Make sure to remove a redundant {} at the end of the `common/istio-1-17/istio-install/base/install.yaml` and `common/istio-1-17/cluster-local-gateway/base/cluster-local-gateway.yaml` files.

---

6. Remove `dump.yaml`

## Changes to Istio's upstream manifests

### Changes to the upstream IstioOperator profile

Changes to Istio's upstream profile `default` are the following:

- Add a `cluster-local-gateway` component for KFServing.
- Disable the EgressGateway component. We don't use it and it adds
unnecessary complexity.
- Add a `cluster-local-gateway` component for Kserve. Knative-local-gateway is now obsolete https://github.com/kubeflow/manifests/pull/2355/commits/adc00b804404ea08685a044ae595be0bed9adb59.
- Disable the EgressGateway component. We do not use it and it adds unnecessary complexity.

Those changes are captured in the [profile-overlay.yaml](profile-overlay.yaml)
file.
Expand All @@ -120,3 +92,4 @@ The Istio kustomizations make the following changes:
- Configure TCP KeepAlives.
- Disable tracing as it causes DNS breakdown. See:
https://github.com/istio/istio/issues/29898
- Set ENABLE_DEBUG_ON_HTTP=false according to https://istio.io/latest/docs/ops/best-practices/security/#control-plane
Loading