bug: mountPropagation option is not proposed for 'varlibcontainers', 'varlogs' and 'systemd' mounts

[antrema]

Describe the issue

When creating a FluentBit instance, the default volumeMounts 'varlibcontainers', 'varlogs' and 'systemd' in the daemonSet created doesn't include mountPropagation option

volumeMounts:
- mountPath: /var/log/containers
  name: varlibcontainers
  readOnly: true
- mountPath: /var/log/
  name: varlogs
  readOnly: true
- mountPath: /var/log/journal
  name: systemd
  readOnly: true

On a Centos 7 cluster, I encounter problems regarding start/stop/restart of fluent-bit pods. This issue seems linked to this bug: rook/rook#3961.
A proposed workaround is to set mountPropagation option to HostToContainer in fluent-bit daemonSet.

To Reproduce

Create a fluent-bit instance as follow:

apiVersion: fluentbit.fluent.io/v1alpha2
kind: FluentBit
metadata:
  name: fluent-bit
  namespace: fluent
  labels:
    app.kubernetes.io/name: fluent-bit
spec:
  image: kubesphere/fluent-bit:v2.0.9
  positionDB:
    hostPath:
      path: /var/lib/fluent-bit/
  fluentBitConfigName: fluent-bit-config

Expected behavior

Have a possibility to define mountPropagation option in fluent-bit daemonSet

Your Environment

- Fluent Operator version: kubesphere/fluent-operator:v2.2.0@sha256:0e90f091602746188e1102dc870943feeb569a4112489a90faa5f73bed75aeee
- Container Runtime: containerd://1.4.12
- Operating system: CentOS Linux 7 (Core)
- Kernel version: 3.10.0-1160.49.1.el7.x86_64

How did you install fluent operator?

I installed the fluent-operator as described in fluent-operator documentation

Additional context

No response

[antrema]

I propose using an spec.internalMountPropagation option in FluentBit CRD, defaulting to 'None'. This option will be applied to all internal volumeMounts:

• "varlibcontainers",
• "varlogs",
• "systemd".
  Tell me if you agree with my proposal

[benjaminhuo]

I propose using an spec.internalMountPropagation option in FluentBit CRD, defaulting to 'None'. This option will be applied to all internal volumeMounts:

• "varlibcontainers",
• "varlogs",
• "systemd".
  Tell me if you agree with my proposal

@wanjunlei @wenchajun what do you think?

[benjaminhuo]

I propose using an spec.internalMountPropagation option in FluentBit CRD, defaulting to 'None'. This option will be applied to all internal volumeMounts:

Maybe DefaultVolumesMountOptions *corev1.VolumeMount json:"defaultVolumesMountOptions,omitempty" ?
We can only apply VolumeMount.MountPropagation now

[antrema]

I don't think this is a default option
The default behaviour regarding mountPropagationis already defined in DaemonSet:
kubectl explain daemonset.spec.template.spec.containers.volumeMounts.mountPropagation

KIND:     DaemonSet
VERSION:  apps/v1

FIELD:    mountPropagation <string>

DESCRIPTION:
     mountPropagation determines how mounts are propagated from the host to
     container and the other way around. When not set, MountPropagationNone is
     used. This field is beta in 1.10.

What we are expecting to do is defining a specific value for mountPropagation in each volumeMounts.
I thought an implementation like this:
[file apis/fluentbit/v1alpha2/fluentbit_types.go

// MountPropagation option for internal mounts
// +kubebuilder:validation:Enum:=None;HostToContainer;Bidirectional
InternalMountPropagation *corev1.MountPropagationMode `json:"internalMountPropagation,omitempty"`

[file pkg/operator/daemonset.go]

internalMountPropagation := corev1.MountPropagationNone
if fb.Spec.InternalMountPropagation != nil {
    internalMountPropagation = *fb.Spec.InternalMountPropagation
}

[...]
VolumeMounts: []corev1.VolumeMount{
    {
        Name:             "varlibcontainers",
        ReadOnly:         true,
        MountPath:        logPath,
        MountPropagation: &internalMountPropagation,
    },
    {
        Name:             "varlogs",
        ReadOnly:         true,
        MountPath:        "/var/log/",
        MountPropagation: &internalMountPropagation,
    },
    {
        Name:             "systemd",
        ReadOnly:         true,
        MountPath:        "/var/log/journal",
        MountPropagation: &internalMountPropagation,
    },
}
[...]

[benjaminhuo]

@antrema your proposal is ok, adding a option only for MountPropagation is one way to go.
My proposal is more general and less specific.