Add istio-kyma-patch back #1085
Conversation
sjanota
requested review from
a-thaler,
aszecowka,
clebs,
crabtree,
derberg,
jakkab,
kazydek,
klaudiagrz,
lilitgh,
mszostok,
mwieczorek,
pbochynski,
piotrmiskiewicz,
PK85,
polskikiel,
strekm,
suleymanakbas91,
Tomasz-Smelcerz-SAP and
tomekpapiernik
as code owners
|
LGTM, tests passed on a cluster for me as well. One minor thing to cleanup is prometheus clusterrole,binding and configmap |
|
|
||
| ## Prerequisites | ||
|
|
||
| To run this application, install Istio in the` istio-system` Namespace. |
There was a problem hiding this comment.
in the 'istio-system' Namespace. - you left an unnecessary space before "istio" ;)
| ### Details | ||
|
|
||
| The application performs several steps: | ||
| 1. Check if the required CRDs are already deployed. The script reads the `required-crds` file which should contain a |
There was a problem hiding this comment.
which should contain -> which must contain
|
|
||
| The application performs several steps: | ||
| 1. Check if the required CRDs are already deployed. The script reads the `required-crds` file which should contain a | ||
| list of Istio's CRDs that Kyma requires. If CRDs are not deployed, the patch fails. |
There was a problem hiding this comment.
list of Istio's CRDs that Kyma requires. If CRDs are not deployed, the patch fails. -> list of Istio CRDs that Kyma requires. If any of these CRDs is not deployed, the patch fails.
| 1. Check if the required CRDs are already deployed. The script reads the `required-crds` file which should contain a | ||
| list of Istio's CRDs that Kyma requires. If CRDs are not deployed, the patch fails. | ||
|
|
||
| 2. Configure the sidecar injector. The following changes are introduced to the `istio-sidecar-injector` ConfigMap: |
There was a problem hiding this comment.
Configure the sidecar injector. The following changes are introduced to the istio-sidecar-injector ConfigMap: -->
Configure the sidecar injector. The istio-sidecar-injector ConfigMap has these modifications:
| * The **zipkinAddress** points to Zipkin deployed in the `kyma-system` Namespace. | ||
| * All containers have the default **resources.limits.memory** set to `128Mi` and **resources.limits.cpu** to `100m`. | ||
|
|
||
| 3. Patch Istio components. The script looks for files in the `{resource-name}.{kind}.patch.json` format which contain a |
There was a problem hiding this comment.
The script looks for files in the {resource-name}.{kind}.patch.json format which contain ->
The script looks for {resource-name}.{kind}.patch.json format files which contain
| 3. Patch Istio components. The script looks for files in the `{resource-name}.{kind}.patch.json` format which contain a | ||
| `JsonPatch`. The components are applied using the `kubectl patch` command. The modified resource may not exist, in which | ||
| case the patch is skipped. See the [job ConfigMap](../../resources/istio-kyma-patch/templates/configmap.yaml) to learn | ||
| which patches are applied by default. In case of any other failure, such as wrong patch format or network error, the |
There was a problem hiding this comment.
In case of any other failure, such as wrong patch format or network error -> In case of failure, such as wrong a patch format or a network error
| which patches are applied by default. In case of any other failure, such as wrong patch format or network error, the | ||
| application fails. | ||
|
|
||
| 4. Remove the unnecessary Istio components. The patch looks for the file named `delete` which should contain lines in |
There was a problem hiding this comment.
The patch acts on a delete file which contains lines that follow the
| application fails. | ||
|
|
||
| 4. Remove the unnecessary Istio components. The patch looks for the file named `delete` which should contain lines in | ||
| the `{kind} {resource-name}` format. Every line describes an Istio resource which should be deleted from the |
There was a problem hiding this comment.
Every line describes an Istio resource which should be deleted from the -> Every line points to an Istio resource which the patch removes from the
|
|
||
| 4. Remove the unnecessary Istio components. The patch looks for the file named `delete` which should contain lines in | ||
| the `{kind} {resource-name}` format. Every line describes an Istio resource which should be deleted from the | ||
| `istio-system` Namespace. It is not an error if the resource to delete is missing. See the |
There was a problem hiding this comment.
It is not an error if the resource to delete is missing. -> The system doesn't return an error if a resource listed in the delete file is not present in the istio-system Namespace.
components/istio-kyma-patch/patch.sh
Outdated
|
|
||
| # Set limits for sidecar. Our namespaces have resource quota set thus every container needs to have limits defined. | ||
| # Add limits to already existing resources sections | ||
| configmap=$(sed 's| resources:| resources:\'$'\n limits: { memory: 128Mi, cpu: 100m }|' <<< "$configmap") |
There was a problem hiding this comment.
why u are not specifying requests property? By default, the request will be the same as the limit.
There was a problem hiding this comment.
We are setting limits here, because our namespaces have resource-quota. In such case every container needs to have limits set. Usually it is done by limit-range, but it does not apply to injected containers. Setting requests is not necessary so we skipped that part and used what they provided.
There was a problem hiding this comment.
"Setting requests is not necessary so we skipped (..)" but why it's not necessary?. IMO you should because it will not be verbose enough and you rely on the internal implementation of k8s. Please set it as it was before.
There was a problem hiding this comment.
Previously requests.memory for istio-proxy was set to the same value as limits.memory in istio/values.yaml. According to k8s docs it is the same as not specifying requests at all. So, accidentally we have the same behaviour as previously :).
There was a problem hiding this comment.
so, as I said: "it will not be verbose enough" and you can see my first comment, "By default, the request will be the same as the limit." but this requires additional knowledge and can be changed in future so you also rely on the k8s implementation.
what's more the request for CPU was different, so it's not the same as previously
| @@ -49,14 +49,11 @@ data: | |||
| global.alertTools.credentials.slack.channel: "" | |||
| global.alertTools.credentials.victorOps.routingkey: "" | |||
| global.alertTools.credentials.victorOps.apikey: "" | |||
| gateways.istio-ingressgateway.service.externalPublicIp: "" | |||
| gateways.istio-ingressgateway.type: "NodePort" | |||
| nginx-ingress.controller.service.loadBalancerIP: "" | |||
| configurations-generator.kubeConfig.clusterName: "kyma.local" | |||
| cluster-users.users.adminGroup: "" | |||
| pilot.resources.limits.memory: 256Mi | |||
| pilot.resources.requests.memory: 128Mi | |||
There was a problem hiding this comment.
Pilot resources were moved to istio-overrides. Remove them
There was a problem hiding this comment.
But still same feeling about the bash script as previously :D (#861 (comment))
This reverts commit 9efe286.
Description
Add istio-kyma-patch again. Fixed this time.
Related issue(s)