-
Notifications
You must be signed in to change notification settings - Fork 229
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
Add support to run Fluentd as DaemonSet #839
Add support to run Fluentd as DaemonSet #839
Conversation
@benjaminhuo @wanjunlei @wenchajun @jjsiv can you please share your feedback/review on this? |
DisableService bool `json:"disableService,omitempty"` | ||
// Numbers of the Fluentd instance | ||
Replicas *int32 `json:"replicas,omitempty"` | ||
DisableService bool `json:"disableService,omitempty"` |
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.
It is not a good idea to delete the replicas
field directly, we should set it to deprecated and delete it in the future.
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 have reverted this change.
// Numbers of the Fluentd instance | ||
Replicas *int32 `json:"replicas,omitempty"` | ||
DisableService bool `json:"disableService,omitempty"` | ||
DaemonSetSpec DaemonSetSpec `json:"daemonSetSpec,omitempty"` |
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 suggest keeping all the original fields as they were as before while adding a switch like mode: agent
and mode: collector
to control whether it should be deployed as DaemonSet or StatefulSet.:
- Default to
mode: collector
to be consistent with the original behaviour - Ignore StatefulSet specific fields when creating DaemonSet and vice versa
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.
Incorporated these changes. Although it would be confusing for customer to see replicas in values.yaml. As it is not generic property for fluentd. It is specific for the Fluend as statefulset...
Let me know, what do you think with the latest changes.
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.
Although it would be confusing for customer to see replicas in values.yaml. As it is not generic property for fluentd. It is specific for the Fluend as statefulset...
@karan56625 Thanks for the changes. I think we can add more comments to make this more clear:
// Numbers of the Fluentd instance
// Applicable when the mode is "collector", and will be ignored when the mode is "agent"
Replicas *int32 `json:"replicas,omitempty"`
// volumeClaimTemplates is a list of claims that pods are allowed to reference.
// The StatefulSet controller is responsible for mapping network identities to
// claims in a way that maintains the identity of a pod. Every claim in
// this list must have at least one matching (by name) volumeMount in one
// container in the template.
// Applicable when the mode is "collector", and will be ignored when the mode is "agent"
VolumeClaimTemplates []corev1.PersistentVolumeClaim `json:"volumeClaimTemplates,omitempty"`
// Storage for position db. You will use it if tail input is enabled.
// Applicable when the mode is "agent", and will be ignored when the mode is "collector"
PositionDB corev1.VolumeSource `json:"positionDB,omitempty"`
// Applicable when the mode is "collector", and will be ignored when the mode is "agent"
replicas: 1
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.
updated
e1c5e31
to
3896ce2
Compare
@@ -268,6 +268,7 @@ fluentd: | |||
crdsEnable: true | |||
enable: false | |||
name: fluentd | |||
mode: "collector" |
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.
mode: "collector" | |
# Valid modes include "collector" and "agent" | |
# The "collector" mode will deploy Fluentd as a StatefulSet as before | |
# The new "agent" mode will deploy Fluentd as a DaemonSet | |
mode: "collector" |
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.
@karan56625 we'd better add this comments to values as well :)
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.
done
0a86296
to
bf7ba93
Compare
// ContainerSecurityContext represents the security context for the fluentd container. | ||
ContainerSecurityContext *corev1.SecurityContext `json:"containerSecurityContext,omitempty"` | ||
// Storage for position db. You will use it if tail input is enabled. | ||
// Applicable when the mode is "collector", and will be ignored when the mode is "agent" |
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.
// Applicable when the mode is "collector", and will be ignored when the mode is "agent" | |
// Applicable when the mode is "agent", and will be ignored when the mode is "collector" |
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.
my bad. fixed
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.
Thanks for this great enhancement @karan56625
Signed-off-by: karan k <karan.k@oracle.com>
bf7ba93
to
42023be
Compare
What this PR does / why we need it:
As of now, Fluentd runs as Sts. But we want to run Fluentd as a complete log process that needs some input plugins to include(tail,systemd) in Fluentd. To make that working, we need to run Fluentd as Daemonset.
In this PR, we introduce support to run fluentd as daemonset optionally. By default, Fluentd will run as Sts. But customer can run the Fluend as Daemonset too, by enabling the flag in
DaemonsetSpec
. In future, if there is some daemonset related specification needs to be provided, that can come underDaemonsetSpec
and if there are some statefulset specification that can be defined understatefulsetSpec
.Also Fluend can be run as either Daemonset or StatefulSet not both. If we enable the Daemonset, StatefulSet will be deleted and Fluentd will be run as Daemonset.
PR is open to receive the feedback from other folks if they have something better to introduce Fluentd as Daemonset. So marked the PR as WIP.
Which issue(s) this PR fixes:
Fixes #755
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: