[Helm chart] Refactoring scheduling methods #4706

nlamirault · 2022-05-02T07:10:37Z

Description: Currently we can't customize affinity, nodeSelectors or tolerations.
This makes it impossible to deploy OSM on an ARM64 cluster.

#4702

Testing done:

Affected area:

Functional Area
New Functionality	[ ]
CI System	[ ]
CLI Tool	[ ]
Certificate Management	[ ]
Control Plane	[ ]
Demo	[ ]
Documentation	[ ]
Egress	[ ]
Ingress	[ ]
Install	[ ]
Networking	[ ]
Observability	[ ]
Performance	[ ]
SMI Policy	[ ]
Security	[ ]
Sidecar Injection	[ ]
Tests	[ ]
Upgrade	[ ]
Other	[x ]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project?
- Did you notify the maintainers and provide attribution?
Is this a breaking change?
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?
Yes into the chart README.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

codecov-commenter · 2022-05-02T16:23:42Z

Codecov Report

Merging #4706 (15c86d8) into main (102baf5) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4706      +/-   ##
==========================================
- Coverage   69.56%   69.53%   -0.04%     
==========================================
  Files         220      223       +3     
  Lines       15800    15941     +141     
==========================================
+ Hits        10992    11085      +93     
- Misses       4758     4805      +47     
- Partials       50       51       +1

Flag	Coverage Δ
unittests	`69.53% <ø> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/certificate/manager.go	`83.33% <0.00%> (-1.59%)`	⬇️
pkg/configurator/client.go	`88.97% <0.00%> (-1.14%)`	⬇️
cmd/osm-bootstrap/osm-bootstrap.go	`43.60% <0.00%> (-0.21%)`	⬇️
cmd/osm-controller/osm-controller.go	`14.78% <0.00%> (-0.07%)`	⬇️
cmd/cli/verify.go	`100.00% <0.00%> (ø)`
cmd/cli/verify_control_plane.go	`36.11% <0.00%> (ø)`
pkg/cli/verifier/control_plane.go	`61.11% <0.00%> (ø)`
pkg/cli/verifier/pod.go	`79.48% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 102baf5...15c86d8. Read the comment docs.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nshankar13 · 2022-05-03T16:42:03Z

charts/osm/values.yaml

@@ -166,13 +229,16 @@ osm:
  enableReconciler: false

  # -- Deploy Prometheus with OSM installation
-  deployPrometheus: false
+  deployPrometheus: true


What's the reasoning for changing the default values for deploying Prometheus, Grafana, Jaeger, and fluentbit?

An error. Fixed

nshankar13 · 2022-05-03T16:48:50Z

@nlamirault can you also share the full osm install command and CLI output on ARM64 cluster/environment, and if possible also on AMD64?

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault · 2022-05-04T09:00:54Z

@nshankar13 can i only generate manifests from CLI ?

shashankram

Thanks @nlamirault for this change. Could you address the following comments:

Describe in the PR description what additional testing was done
Update the release notes to reflect the breaking changes to existing Helm chart values.

nshankar13 · 2022-05-04T16:51:44Z

@nlamirault manifests/configs not needed, just need to ensure that installation succeeded and that all pods in OSM release namespace are ready.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault · 2022-05-05T06:48:59Z

My tests was :

NAME                              READY   STATUS    RESTARTS   AGE
osm-injector-7ccd8c9d98-xdmqg     0/1     Pending   0          8d
osm-controller-687f75c598-l4lsd   0/1     Pending   0          8d
osm-bootstrap-5df4b86cc7-wpsbp    0/1     Pending   0          8d

kubectl edit to change amd64 to arm64 :

❯ kubectl -n osm-system get pod
NAME                              READY   STATUS    RESTARTS   AGE
osm-bootstrap-74fdcc5589-58f9q    1/1     Running   0          76s
osm-controller-7fc5c87bf4-jd5zp   1/1     Running   0          43s
osm-injector-6dc5fd7658-bqm6q     1/1     Running   0          61s

Then update the chart, and check differences :

with https://github.com/Wilfred/difftastic :

❯ difft amd64.yaml arm64.yaml
arm64.yaml --- YAML
No syntactic changes.

with diff :

✖ diff amd64.yaml arm64.yaml
275c275
<                 - amd64
---
>                 - arm64
396c396
<                 - amd64
---
>                 - arm64
515c515
<                 - amd64
---
>                 - arm64
757c757
<         kubernetes.io/arch: amd64
---
>         kubernetes.io/arch: arm64

I will try to delete my setup and install the chart with arm64 values.

nlamirault · 2022-05-05T07:03:21Z

❯ helm install test-osm -f ~/values-arm64.yaml -n test-osm Forks/osm/charts/osm/
NAME: test-osm
LAST DEPLOYED: Thu May  5 09:01:04 2022
NAMESPACE: test-osm
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
Congratulations! The OSM control plane has been installed in your Kubernetes cluster!

❯ kubectl -n test-osm get pods
NAME                              READY   STATUS    RESTARTS   AGE
osm-bootstrap-7db9ccf4b6-d7sl4    1/1     Running   0          2m
osm-controller-7fbf694685-t6t4n   1/1     Running   0          2m1s
osm-injector-c9795d495-wxlc9      1/1     Running   0          2m

shashankram · 2022-05-10T16:30:55Z

Thanks @nlamirault for this change. Could you address the following comments:

Describe in the PR description what additional testing was done

Update the release notes to reflect the breaking changes to existing Helm chart values.

@nlamirault The release notes aren't updated as requested. This is important because this PR is a breaking change to install options.

nlamirault · 2022-05-11T14:59:13Z

@shashankram how i update the release note ?
Last release is on v1.1.0 (https://github.com/openservicemesh/osm/blob/main/docs/release_notes.md#release-v110=), but there is a v1.1.1
Do i have to create a v1.2.0 on the release note document ?

shashankram · 2022-05-11T16:34:42Z

@shashankram how i update the release note ? Last release is on v1.1.0 (https://github.com/openservicemesh/osm/blob/main/docs/release_notes.md#release-v110=), but there is a v1.1.1 Do i have to create a v1.2.0 on the release note document ?

@nlamirault You need to make changes to that file by adding a section for v1.2.0 and listing the breaking changes.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nojnhuh

Thanks for this change @nlamirault! Left a few comments inline.

nojnhuh · 2022-05-19T19:10:54Z

charts/osm/values.yaml

+            - key: kubernetes.io/arch
+              operator: In
+              values:
+              - amd64


I'd say let's add arm64 here since all of the OSM images we publish will also work on arm64.

Done. I add arm64 in other podAntiAffinity also.

nojnhuh · 2022-05-19T19:12:38Z

charts/osm/values.yaml

@@ -88,6 +88,41 @@ osm:
        # -- Average target memory utilization (%)
        targetAverageUtilization: 80

+    ## Node labels for pod assignment
+    ## Ref: https://kubernetes.io/docs/user-guide/node-selection/
+    nodeSelector: {}


For the sake of not having multiple ways of doing the same thing, I would vote to only expose affinity in the values here and remove nodeSelector.

charts/osm/values.yaml

nojnhuh · 2022-05-19T19:16:44Z

charts/osm/values.yaml

+
+    ## Affinity settings for pod assignment
+    ## Ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
+    affinity: {}


Since the Promtheus image we're using by default has support for several different architectures, let's define all of them as acceptable here instead of just amd64 in nodeSelector: https://hub.docker.com/layers/prometheus/prom/prometheus/v2.34.0/images/sha256-cb42332b66ac51a05c52f255e48a4496c0a172676093123bf28b37762009e78a?context=explore

So only :

nodeSelector: kubernetes.io/arch: amd64

I think Prometheus images are multi arch now ?

I mean let's define all of the architectures supported by the Prometheus image in the nodeAffinity here and remove the nodeSelector.

charts/osm/templates/cleanup-hook.yaml

nojnhuh · 2022-05-19T20:01:10Z

charts/osm/values.schema.json

+                    "title": "The preinstall schema",
+                    "description": "Preinstall configurations",
+                    "required": [],
+                    "properties": {


Let's also add "additionalProperties": false to these to match everything else.

…on arm64 Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nojnhuh · 2022-05-20T15:37:32Z

charts/osm/values.yaml

+
+    ## Affinity settings for pod assignment
+    ## Ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
+    affinity: {}


I mean let's define all of the architectures supported by the Prometheus image in the nodeAffinity here and remove the nodeSelector.

nojnhuh · 2022-05-20T15:42:21Z

charts/osm/values.yaml

+
+    ## Affinity settings for pod assignment
+    ## Ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
+    affinity: {}


Let's do the same as Prometheus here and define all of the architectures supported by the image here in nodeAffinity.

nojnhuh · 2022-05-20T15:51:28Z

charts/osm/values.yaml

+    ## Node labels for pod assignment
+    ## Ref: https://kubernetes.io/docs/user-guide/node-selection/
+    nodeSelector:
+      kubernetes.io/arch: amd64


@shashankram The envoy-alpine image is only built for amd64. Do you see any issues if we specify envoyproxy/envoy as the default sidecar image instead which also supports arm64?

nojnhuh · 2022-05-20T15:53:17Z

charts/osm/values.yaml

+    ## Node labels for pod assignment
+    ## Ref: https://kubernetes.io/docs/user-guide/node-selection/
+    nodeSelector:
+      kubernetes.io/arch: amd64


This image also supports arm64, so let's define both amd64 and arm64 as acceptable in the nodeAffinity.

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

shalier · 2022-06-09T19:22:11Z

PR also addresses #4496

shalier · 2022-06-09T20:25:52Z

@nlamirault Please let us know if we can provide more information on the outstanding comments!

shalier · 2022-06-23T19:04:48Z

Created #4842 to address comments in this PR - closing this PR

Update: Refactoring scheduling methods

2773ad0

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault requested review from shashankram, snehachhabria, nojnhuh, draychev, jaellio and trstringer as code owners May 2, 2022 07:10

nlamirault mentioned this pull request May 3, 2022

Service Mesh portefaix/portefaix-kubernetes#2203

Closed

Update: documentation

56a1555

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault changed the title ~~Refactoring scheduling methods~~ [Helm chart] Refactoring scheduling methods May 3, 2022

nshankar13 reviewed May 3, 2022

View reviewed changes

nshankar13 mentioned this pull request May 3, 2022

OSM ARM 64 support #4716

Closed

Fix: disable addons

0fa851e

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

shashankram reviewed May 4, 2022

View reviewed changes

Update: move affinity like original chart

e69c01a

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Merge branch 'openservicemesh:main' into feat/selectors

6d17522

nlamirault added 2 commits May 12, 2022 17:50

Add: changes for new release

15c86d8

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Fix: chart documentation

d145499

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nojnhuh suggested changes May 19, 2022

View reviewed changes

nlamirault added 2 commits May 20, 2022 09:01

Add: arm64 for podAntiAffinity since all of the OSM images also work …

ec3b13e

…on arm64 Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Fix: remove comment

fd6c0b0

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nlamirault added 2 commits May 20, 2022 09:08

Add: Affinity and Tolerations for cleanup job

5e904d9

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Fix: remove comment

91434fc

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

nojnhuh reviewed May 20, 2022

View reviewed changes

nlamirault added 4 commits June 3, 2022 07:41

Update: use node affinity

e5b8d93

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Merge branch 'main' into feat/selectors

8ac625b

Fix: documentation

79c6987

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>

Merge branch 'main' into feat/selectors

5f94b0f

shalier mentioned this pull request Jun 9, 2022

Allows node affinity values to be set in values.yaml #4798

Closed

shalier requested a review from shashankram June 9, 2022 19:29

shalier mentioned this pull request Jun 22, 2022

Customize affinity, nodeSelectors and tolerations in values.yaml #4842

Merged

shalier closed this Jun 23, 2022

[Helm chart] Refactoring scheduling methods #4706

[Helm chart] Refactoring scheduling methods #4706

Uh oh!

Conversation

nlamirault commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlamirault May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nshankar13 commented May 3, 2022

Uh oh!

nlamirault commented May 4, 2022

Uh oh!

shashankram left a comment

Choose a reason for hiding this comment

Uh oh!

nshankar13 commented May 4, 2022

Uh oh!

nlamirault commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nlamirault commented May 5, 2022

Uh oh!

shashankram commented May 10, 2022

Uh oh!

nlamirault commented May 11, 2022

Uh oh!

shashankram commented May 11, 2022

Uh oh!

nojnhuh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlamirault May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlamirault May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shalier commented Jun 9, 2022

Uh oh!

shalier commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shalier commented Jun 23, 2022

Uh oh!

Uh oh!

nlamirault commented May 2, 2022 •

edited

Loading

codecov-commenter commented May 2, 2022 •

edited

Loading

nlamirault May 4, 2022 •

edited

Loading

nlamirault commented May 5, 2022 •

edited

Loading

nlamirault May 20, 2022 •

edited

Loading

nlamirault May 20, 2022 •

edited

Loading

shalier commented Jun 9, 2022 •

edited

Loading