Skip to content

operator: Make CRD availability timeout configurable#12177

Merged
aanm merged 3 commits intocilium:masterfrom
vadorovsky:operator-crd-followup
Jun 26, 2020
Merged

operator: Make CRD availability timeout configurable#12177
aanm merged 3 commits intocilium:masterfrom
vadorovsky:operator-crd-followup

Conversation

@vadorovsky
Copy link
Copy Markdown
Member

@vadorovsky vadorovsky commented Jun 18, 2020

This change allows to override the default 5m timeout with the
--crd-wait-timeout option.

Signed-off-by: Michal Rostecki mrostecki@opensuse.org

Follow up to #10899

@vadorovsky vadorovsky requested review from a team as code owners June 18, 2020 11:25
@vadorovsky vadorovsky requested a review from a team June 18, 2020 11:25
@vadorovsky vadorovsky requested a review from a team as a code owner June 18, 2020 11:25
@maintainer-s-little-helper
Copy link
Copy Markdown

Please set the appropriate release note label.

@vadorovsky vadorovsky changed the title Operator crd followup operator: Make CRD availability timeout configurable Jun 18, 2020
@vadorovsky vadorovsky added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/operator Impacts the cilium-operator component release-note/misc This PR makes changes that have no direct user impact. labels Jun 18, 2020
@vadorovsky vadorovsky added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/helm Impacts helm charts and user deployment experience labels Jun 18, 2020
@vadorovsky vadorovsky requested a review from aanm June 18, 2020 11:26
@vadorovsky
Copy link
Copy Markdown
Member Author

test-me-please

Comment thread install/kubernetes/cilium/values.yaml Outdated
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 18, 2020

Coverage Status

Coverage increased (+0.007%) to 37.175% when pulling 0f3f1b8107aeb883d2739030a9ec4fbfe52a5a0e on mrostecki:operator-crd-followup into 783982c on cilium:master.

Comment thread install/kubernetes/cilium/values.yaml Outdated
Comment thread install/kubernetes/cilium/charts/config/templates/configmap.yaml Outdated
@vadorovsky vadorovsky force-pushed the operator-crd-followup branch 2 times, most recently from 9c749cf to 4110b22 Compare June 18, 2020 14:52
Comment on lines 576 to 595
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we are almost there. There was another PR with similar changes and I think this is the right steps to add new options into the config map:
#12185 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've done it more similar way to how defaultBpfCtAnyMax variable is handled, since it might contain a custom value, not a true/false bool. Please look if you are fine with that.

Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Minor nit on the docs side to clarify what it means by "CRDs have to be available".

Comment thread operator/flags.go Outdated
@vadorovsky vadorovsky force-pushed the operator-crd-followup branch 2 times, most recently from 3c925ad to 45e240b Compare June 19, 2020 09:43
@vadorovsky
Copy link
Copy Markdown
Member Author

test-me-please

Comment thread install/kubernetes/cilium/charts/config/templates/configmap.yaml Outdated
Comment thread install/kubernetes/cilium/charts/config/templates/configmap.yaml Outdated
@vadorovsky vadorovsky force-pushed the operator-crd-followup branch 3 times, most recently from 5702ef6 to d7d08c6 Compare June 24, 2020 07:48
@vadorovsky
Copy link
Copy Markdown
Member Author

test-me-please

@vadorovsky vadorovsky force-pushed the operator-crd-followup branch from d7d08c6 to a4cedaa Compare June 24, 2020 09:59
@vadorovsky
Copy link
Copy Markdown
Member Author

test-me-please

@aanm aanm removed the area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Jun 24, 2020
Copy link
Copy Markdown
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Discussed with Michal offline. It's not there yet, mostly due my fault.

@vadorovsky vadorovsky force-pushed the operator-crd-followup branch from a4cedaa to 0f3f1b8 Compare June 25, 2020 15:17
@vadorovsky
Copy link
Copy Markdown
Member Author

test-me-please

Copy link
Copy Markdown
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits below

Comment thread operator/flags.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
flags.Duration(operatorOption.CRDWaitTimeout, 5*time.Minute, "Operator will exit if CRDs are not available within this duration from startup")
flags.Duration(operatorOption.CRDWaitTimeout, 5*time.Minute, "Operator will exit if CRDs are not available within this duration upon startup")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: s/from/upon; there are many occurrences of this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: here as well

This change allows to override the default 5m timeout with the
--crd-wait-timeout option.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky vadorovsky force-pushed the operator-crd-followup branch from 0f3f1b8 to efa8d1f Compare June 26, 2020 17:52
@aanm
Copy link
Copy Markdown
Member

aanm commented Jun 26, 2020

The CI was previously green, the only changes addressed were typos point it out by Chris. Merging

@aanm aanm merged commit 76df134 into cilium:master Jun 26, 2020
@vadorovsky vadorovsky deleted the operator-crd-followup branch June 26, 2020 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/helm Impacts helm charts and user deployment experience area/operator Impacts the cilium-operator component release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants