wip: update daemonset.yaml (--disable-cri-async)

[krisnova]

No description provided.

[poiana]

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

• 46431af Update daemonset.yaml

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[poiana]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kris-nova
You can assign the PR to them by writing /assign @kris-nova in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• falco/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[victortrac]

Instead of hard-coding it here, would it be more appropriate to let the user set this flag via Values.extraArgs (on line 66)? Seems like the change here should be a documentation change that informs the user about the flag rather than a hard-coded value.

[leogr]

More context here 👉 falcosecurity/falco#925

[nibalizer]

@kris-nova Can you look in on this when you get a second?

[leogr]

Hey @kris-nova

I agree with @victortrac about using extraArgs.

Furthermore, since that flag can have a performance penalty:

--disable-cri-async           Disable asynchronous CRI metadata fetching.
                               This is useful to let the input event wait for the container metadata fetch
                               to finish before moving forward. Async fetching, in some environments leads
                               to empty fields for container metadata when the fetch is not fast enough to be
                               completed asynchronously. This can have a performance penalty on your environment
                               depending on the number of containers and the frequency at which they are created/started/stopped

I would NOT put it on by default.

Thus I believe the issue here is mostly the documentation.

So, can we rework this PR?

I will put wip: in the title to avoid that this PR gets merged when not yet ready.

[leogr]

Since we have extraArgs here 👉

charts/falco/templates/daemonset.yaml

Lines 65 to 67 in 1769c50

	{{- if .Values.extraArgs }}
	{{ toYaml .Values.extraArgs | indent 12 }}
	{{- end }}

I believe we can close this.
/close

If you think we still need further improvement, feel free to re-open this again!

[poiana]

@leogr: Closed this PR.

In response to this:

Since we have extraArgs here 👉

charts/falco/templates/daemonset.yaml

Lines 65 to 67 in 1769c50

	{{- if .Values.extraArgs }}
	{{ toYaml .Values.extraArgs | indent 12 }}
	{{- end }}

I believe we can close this.
/close

If you think we still need further improvement, feel free to re-open this again!

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.