-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Update olm addon to v0.17.0 #10947
Update olm addon to v0.17.0 #10947
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kadel The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the crds.yaml.tmpl diff says
11,302 additions, 10,357 deletions not shown because the diff is too large. Please use a local Git client to view these changes.
this doesnt sound right, do u mind clarifying what is inside that file?
@@ -78,9 +78,9 @@ spec: | |||
command: | |||
- /bin/olm | |||
args: | |||
- -namespace | |||
- --namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason for the additional -- ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old OLM versions were using flags
at some point they switched to github.com/spf13/pflag
library, hence move from -
to --
This is correct. It contains all CustomeResourceDefinitions used by OLM. This is taken from the official v0.17.0 release artifcats. You can check it at https://github.com/operator-framework/operator-lifecycle-manager/releases/tag/v0.17.0 |
f62cf49
to
63ed2c5
Compare
Hey @kadel, we have a test (in test/integration/addons_test.go) that currently skips testing the OLM addon. Could you remove the skip so that we can properly test this PR? Thanks! |
oh, I haven't noticed the integration test. |
/ok-to-test |
kvm2 Driver |
@@ -326,8 +328,16 @@ spec: | |||
requests: | |||
cpu: 10m | |||
memory: 50Mi | |||
securityContext: | |||
runAsUser: 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required ? this seems to give Root Access to the addon,
do u mind clarifying why we need to run with more special access ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't work on OLM directly I'm just a user and I did not write this definition. This comes from the official OLM release artifacts.
But If I'm not mistaken root has uid 0
. This is not giving any special access to the container, it is doing quite the opposite. It ensures that the container can't run anything as root.
1000
is the first non-root user. Even if the image is built in a way that it runs as root inside the container, this will force it to be non-root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense ! thanks for the clarifying, I mistakenly thought it was user 1
@@ -308,7 +310,7 @@ spec: | |||
- --global-namespace | |||
- olm | |||
image: {{.CustomRegistries.OLM | default .ImageRepository | default .Registries.OLM }}{{.Images.OLM}} | |||
imagePullPolicy: IfNotPresent | |||
imagePullPolicy: Always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change related to bumping the version ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
63af485
to
1e8a841
Compare
kvm2 Driver Times for minikube ingress: 43.3s 36.4s 46.3s 43.8s 36.4s Times for minikube (PR 10947) start: 47.8s 51.8s 52.5s 48.9s 48.7s Times for minikube (PR 10947) ingress: 43.3s 43.3s 45.0s 36.7s 34.8s Averages Time Per Log
docker Driver Times for minikube ingress: 37.0s 35.0s 37.0s 31.0s 34.0s Times for minikube (PR 10947) start: 21.5s 22.5s 21.0s 21.8s 22.0s Times for minikube (PR 10947) ingress: 32.5s 35.0s 33.5s 33.5s 38.0s Averages Time Per Log
|
1e8a841
to
a3be004
Compare
/ok-to-test |
kvm2 driver with docker runtime
Times for minikube start: 57.7s 58.2s 54.4s 58.7s 53.4s Times for minikube ingress: 36.9s 43.5s 38.5s 36.4s 39.4s docker driver with docker runtime
Times for minikube ingress: 32.1s 32.6s 91.1s 32.6s 33.6s Times for minikube start: 23.5s 23.0s 23.6s 23.5s 23.2s docker driver with containerd runtime
Times for minikube start: 44.8s 48.5s 43.8s 45.6s 44.4s |
Looks good to me @kadel , Thank you for this PR and improving olm addon look forward for more PRs for you |
fixes #10768