Skip to content
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.

Sign up for GitHub

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

Monitoring API: Add AlertmanagerMainConfig #2148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+5,593 −6
Open

Monitoring API: Add AlertmanagerMainConfig #2148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions config/v1/types_cluster_monitoring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1

import (
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -77,6 +78,9 @@ type ClusterMonitoringSpec struct {
// userDefined set the deployment mode for user-defined monitoring in addition to the default platform monitoring.
// +required
UserDefined UserDefinedMonitoring `json:"userDefined"`
// alertmanagerMainConfig defines settings for the Alertmanager component in the `openshift-monitoring` namespace.
Copy link

Choose a reason for hiding this comment

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

While this is a good start in stating what this field does, generally it is recommended to follow the guidelines for writing godoc on the API fields here: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc

In particular, the changes I would suggest here are:

  • Making sure there is a clear explanation as to why a user may want to use this field (i.e what does it allow them to achieve?)
  • including a reference that the field is required. Users can't see the +required marker in the generated documentation that is shown when using something like oc explain ... so being explicit in the godoc ensures that a user will see that information.

// +required
AlertmanagerMainConfig AlertmanagerMainConfig `json:"alertmanagerMainConfig"`
}

// UserDefinedMonitoring config for user-defined projects.
Expand All @@ -101,3 +105,86 @@ const (
// UserDefinedNamespaceIsolated enables monitoring for user-defined projects with namespace-scoped tenancy. This ensures that metrics, alerts, and monitoring data are isolated at the namespace level.
UserDefinedNamespaceIsolated UserDefinedMode = "NamespaceIsolated"
)

// The `AlertmanagerMainConfig` resource defines settings for the
// Alertmanager component in the `openshift-monitoring` namespace.
type AlertmanagerMainConfig struct {
// mode enables or disables the main Alertmanager instance. in the `openshift-monitoring` namespace
// Allowed values are "Enabled", "Disabled".
Comment on lines +112 to +113
Copy link

Choose a reason for hiding this comment

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

A few things:

  • It would be good to go a bit more in detail on what this actually does when set to a specific value. What actually happens when this is set to Enabled?
  • Include a snippet that this field is required, as per my previous comment about tags not being included in generated documentation
  • Generally we try to avoid using Enabled and Disabled as they can be a bit overloaded and similar to True/False. This makes it a bit more difficult to introduce additional modes in the future where it isn't necessarily a clean enabled/disabled. Are there any other names you could use that are more descriptive for the same desired state?

// +kubebuilder:validation:Enum:=Enabled;Disabled;""
Copy link

Choose a reason for hiding this comment

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

It looks like this validation includes the ability to use an empty string as a value. There is no mention that the empty value is a valid value - is it?

If it is, is this actually a required field? What happens in the case of the empty string value?

// +required
Mode AlertManagerMode `json:"mode"`
// userMode enables or disables user-defined namespaces
// to be selected for `AlertmanagerConfig` lookups. This setting only
// applies if the user workload monitoring instance of Alertmanager
// is not enabled.
Comment on lines +117 to +120
Copy link

Choose a reason for hiding this comment

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

A few things:

  • What does enable/disable mean in this context?
  • You mention that this setting only applies during a specific case, what field does a user have to set to adhere to that case? If there is a dependency between the fields, there should be some validation in place to ensure the right state
  • If this field is only valid during a certain state, is it really required?

// +required
UserMode UserAlertManagerMode `json:"userMode"`
// logLevel Defines the log level setting for Alertmanager.
// The possible values are: `Error`, `Warn`, `Info`, `Debug`.
// The default value is `Info`.
Comment on lines +123 to +125
Copy link

Choose a reason for hiding this comment

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

A few things:

  • Would be good to add more information on why a user may care about setting this field.
  • Include in the godoc that the field is optional
  • Elaborate on what each allowed value means when set. For example, what happens when logLevel is set to Debug vs Warn? Generally log levels are well understood, but it would help to highlight how verbose each log level is.

// +optional
// +kubebuilder:default=Info
Copy link

Choose a reason for hiding this comment

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

OpenAPI generation doesn't understand the kubebuilder default tag. Use +default instead here.

LogLevel string `json:"logLevel,omitempty"`
Copy link

Choose a reason for hiding this comment

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

Generally when using an enum it is recommended to use a type alias. This field is also missing an enum marker.

// nodeSelector Defines the nodes on which the Pods are scheduled.
Copy link

Choose a reason for hiding this comment

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

It would be good to explain more about what is done with the values in this field and why a user might care. Is this field passed directly to the Pod spec for node selection?

Also include that this field is optional in the godoc and what happens if it is not specified.

// +optional
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
// resources Defines resource requests and limits for the Alertmanager container.
Copy link

Choose a reason for hiding this comment

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

Would recommend expanding on the godoc a bit more here. Include that it is optional and what happens if not set. Should there be a default? Why would a user care to use this field? How is the value of this field used?

// +optional
Resources *v1.ResourceRequirements `json:"resources,omitempty"`
Copy link

Choose a reason for hiding this comment

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

This seems like it would be passed directly to the pod spec, but it is worth considering - are you okay with relying on an external API, including any and all maintenance challenges involved in that?

// secrets Defines a list of secrets that need to be mounted into the Alertmanager.
// The secrets must reside within the same namespace as the Alertmanager object.
// They will be added as volumes named secret-<secret-name> and mounted at
// /etc/alertmanager/secrets/<secret-name> within the 'alertmanager' container of
// the Alertmanager Pods.
// +optional
Copy link

Choose a reason for hiding this comment

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

Mention that the field is optional in the godoc

Secrets []string `json:"secrets,omitempty"`
Copy link

Choose a reason for hiding this comment

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

For local object references, the api convention is to create a type alias. A good example is in the OpenShift API conventions doc under https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-specific-types-for-object-references-and-omit-ref-suffix

// tolerations Defines tolerations for the pods.
// +optional
Comment on lines +142 to +143
Copy link

Choose a reason for hiding this comment

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

Include the field is optional in the godoc. Elaborate further on the field, including what it does, why a user may care, etc.

Tolerations []v1.Toleration `json:"tolerations,omitempty"`
// topologySpreadConstraints Defines a pod's topology spread constraints.
// +optional
Comment on lines +145 to +146
Copy link

Choose a reason for hiding this comment

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

Same general comments regarding godoc updates.

TopologySpreadConstraints []v1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"`
Comment on lines +129 to +147
Copy link

Choose a reason for hiding this comment

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

This generally looks like a bunch of configuration options that are going to be piped directly to a Pod spec. Would it make more sense to put these under a new type to group all of these options under a field named something like podTemplate to make it more clear that these values get stamped directly onto the Alertmanager Pod?

// volumeClaimTemplate Defines persistent storage for Alertmanager. Use this setting to
// configure the persistent volume claim, including storage class, volume
// size, and name.
// +optional
Comment on lines +148 to +151
Copy link

Choose a reason for hiding this comment

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

Mention the field is optional in the godoc. What happens when this field is unset?

VolumeClaimTemplate v1.PersistentVolumeClaim `json:"volumeClaimTemplate,omitempty"`
}

// AlertmanagerMode defines mode for AlertManager instance
// +kubebuilder:validation:Enum="";Enabled;Disabled
Copy link

Choose a reason for hiding this comment

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

This is a repeated enum validation that exists on the AlertmanagerMainConfig.Mode field. This results in two separate enum validations showing up on the generated CRD that may result in unexpected behavior. Maintaining two separate validations may also introduce drift.

Pick one location to add this validation.

type AlertManagerMode string

const (
// AlertManagerEnable enables the main Alertmanager instance. in the `openshift-monitoring` namespace
AlertManagerEnabled AlertManagerMode = "Enabled"
Copy link

Choose a reason for hiding this comment

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

Generally, it is recommended to have these constants follow a pattern of type name followed by the value. In this case it should be:

Suggested change
AlertManagerEnabled AlertManagerMode = "Enabled"
AlertManagerModeEnabled AlertManagerMode = "Enabled"

// AlertManagerDisabled enables the main Alertmanager instance. in the `openshift-monitoring` namespace
Copy link

Choose a reason for hiding this comment

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

Suggested change
// AlertManagerDisabled enables the main Alertmanager instance. in the `openshift-monitoring` namespace
// AlertManagerDisabled disables the main Alertmanager instance. in the `openshift-monitoring` namespace

?

AlertManagerDisabled AlertManagerMode = "Disabled"
Copy link

Choose a reason for hiding this comment

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

Same as above:

Suggested change
AlertManagerDisabled AlertManagerMode = "Disabled"
AlertManagerModeDisabled AlertManagerMode = "Disabled"

)

// UserAlertManagerMode defines mode for user-defines namespaced
// +kubebuilder:validation:Enum="";Enabled;Disabled
Copy link

Choose a reason for hiding this comment

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

UserMode looks to be a required field, is the empty string actually a valid value? If so, what happens in the case the empty value is provided?

type UserAlertManagerMode string

const (
// AlertManagerEnabled enables user-defined namespaces to be selected for `AlertmanagerConfig` lookups. This setting only
// applies if the user workload monitoring instance of Alertmanager is not enabled.
UserAlertManagerEnabled UserAlertManagerMode = "Enabled"
Comment on lines +171 to +173
Copy link

Choose a reason for hiding this comment

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

Suggested change
// AlertManagerEnabled enables user-defined namespaces to be selected for `AlertmanagerConfig` lookups. This setting only
// applies if the user workload monitoring instance of Alertmanager is not enabled.
UserAlertManagerEnabled UserAlertManagerMode = "Enabled"
// UserAlertManagerModeEnabled enables user-defined namespaces to be selected for `AlertmanagerConfig` lookups. This setting only
// applies if the user workload monitoring instance of Alertmanager is not enabled.
UserAlertManagerModeEnabled UserAlertManagerMode = "Enabled"

// AlertManagerDisabled disables user-defined namespaces to be selected for `AlertmanagerConfig` lookups. This setting only
// applies if the user workload monitoring instance of Alertmanager is not enabled.
UserAlertManagerDisabled UserAlertManagerMode = "Disabled"
Comment on lines +174 to +176
Copy link

Choose a reason for hiding this comment

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

Suggested change
// AlertManagerDisabled disables user-defined namespaces to be selected for `AlertmanagerConfig` lookups. This setting only
// applies if the user workload monitoring instance of Alertmanager is not enabled.
UserAlertManagerDisabled UserAlertManagerMode = "Disabled"
// UserAlertManagerModeDisabled disables user-defined namespaces to be selected for `AlertmanagerConfig` lookups. This setting only
// applies if the user workload monitoring instance of Alertmanager is not enabled.
UserAlertManagerModeDisabled UserAlertManagerMode = "Disabled"

)

// +kubebuilder:validation:Enum="";Error;Warn;Info;Debug
Copy link

Choose a reason for hiding this comment

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

Is an empty string a valid log level value? What should happen when set to the empty string value?

type LogLevel string
Copy link

Choose a reason for hiding this comment

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

There should be at least some godoc here to explain what this type is for.


var (
Copy link

Choose a reason for hiding this comment

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

Should be constants

Error LogLevel = "error"

Warn LogLevel = "warn"

Info LogLevel = "info"

Debug LogLevel = "debug"
Comment on lines +183 to +189
Copy link

Choose a reason for hiding this comment

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

The string values here should be capitalized to match the enum values.

Each should generally follow the name pattern of LogLevel{value}.

)
Loading