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.

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

Remove optional from configsync in favor of an internal enabled/disabled state #32229

Merged
merged 1 commit into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 8 additions & 8 deletions cmd/otel-agent/subcommands/run/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,15 @@ func runOTelAgentCommand(ctx context.Context, params *subcommands.GlobalParams,
}),
logfx.Module(),
fetchonlyimpl.Module(),
// TODO: don't rely on this pattern; remove this `OptionalModuleWithParams` thing
// TODO: don't rely on this pattern; remove this `ModuleWithParams` thing
// and instead adapt OptionalModule to allow parameter passing naturally.
// See: https://github.com/DataDog/datadog-agent/pull/28386
configsyncimpl.OptionalModuleWithParams(),
configsyncimpl.ModuleWithParams(),
fx.Provide(func() configsyncimpl.Params {
return configsyncimpl.NewParams(params.SyncTimeout, params.SyncDelay, true)
}),
converterfx.Module(),
fx.Provide(func(cp converter.Component, _ optional.Option[configsync.Component]) confmap.Converter {
fx.Provide(func(cp converter.Component, _ configsync.Component) confmap.Converter {
return cp
}),
collectorcontribFx.Module(),
Expand Down Expand Up @@ -191,10 +191,10 @@ func runOTelAgentCommand(ctx context.Context, params *subcommands.GlobalParams,
fx.Invoke(func(_ collectordef.Component, _ defaultforwarder.Forwarder, _ optional.Option[logsagentpipeline.Component]) {
}),

// TODO: don't rely on this pattern; remove this `OptionalModuleWithParams` thing
// TODO: don't rely on this pattern; remove this `ModuleWithParams` thing
// and instead adapt OptionalModule to allow parameter passing naturally.
// See: https://github.com/DataDog/datadog-agent/pull/28386
configsyncimpl.OptionalModuleWithParams(),
configsyncimpl.ModuleWithParams(),
fx.Provide(func() configsyncimpl.Params {
return configsyncimpl.NewParams(params.SyncTimeout, params.SyncDelay, true)
}),
Expand All @@ -218,7 +218,7 @@ func runOTelAgentCommand(ctx context.Context, params *subcommands.GlobalParams,

// TODO: consider adding configsync.Component as an explicit dependency for traceconfig
// to avoid this sort of dependency tree hack.
fx.Provide(func(deps traceconfig.Dependencies, _ optional.Option[configsync.Component]) (traceconfig.Component, error) {
fx.Provide(func(deps traceconfig.Dependencies, _ configsync.Component) (traceconfig.Component, error) {
// TODO: this would be much better if we could leverage traceconfig.Module
// Must add a new parameter to traconfig.Module to handle this.
return traceconfig.NewConfig(deps)
Expand All @@ -236,13 +236,13 @@ func runOTelAgentCommand(ctx context.Context, params *subcommands.GlobalParams,

// ForwarderBundle returns the fx.Option for the forwarder bundle.
// TODO: cleanup the forwarder instantiation with fx.
// This is a bit of a hack because we need to enforce optional.Option[configsync.Component]
// This is a bit of a hack because we need to enforce configsync.Component
// is passed to newForwarder to enforce the correct instantiation order. Currently, the
// new forwarder.BundleWithProvider makes a few assumptions in its generic prototype, and
// this is the current workaround to leverage it.
func ForwarderBundle() fx.Option {
return defaultforwarder.ModulWithOptionTMP(
fx.Provide(func(_ optional.Option[configsync.Component]) defaultforwarder.Params {
fx.Provide(func(_ configsync.Component) defaultforwarder.Params {
return defaultforwarder.NewParams()
}))
}
5 changes: 2 additions & 3 deletions cmd/process-agent/command/main_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ import (
"github.com/DataDog/datadog-agent/pkg/util/fxutil"
"github.com/DataDog/datadog-agent/pkg/util/fxutil/logging"
"github.com/DataDog/datadog-agent/pkg/util/log"
"github.com/DataDog/datadog-agent/pkg/util/optional"
"github.com/DataDog/datadog-agent/pkg/version"
)

Expand Down Expand Up @@ -156,7 +155,7 @@ func runApp(ctx context.Context, globalParams *GlobalParams) error {
fetchonlyimpl.Module(),

// Provide configsync module
configsyncimpl.OptionalModule(),
configsyncimpl.Module(),

// Provide autoexit module
autoexitimpl.Module(),
Expand Down Expand Up @@ -215,7 +214,7 @@ func runApp(ctx context.Context, globalParams *GlobalParams) error {
_ expvars.Component,
_ apiserver.Component,
cfg config.Component,
_ optional.Option[configsync.Component],
_ configsync.Component,
// TODO: This is needed by the container-provider which is not currently a component.
// We should ensure the tagger is a dependency when converting to a component.
_ tagger.Component,
Expand Down
5 changes: 2 additions & 3 deletions cmd/security-agent/main_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ import (

"github.com/DataDog/datadog-agent/pkg/util/defaultpaths"
"github.com/DataDog/datadog-agent/pkg/util/fxutil"
"github.com/DataDog/datadog-agent/pkg/util/optional"
"github.com/DataDog/datadog-agent/pkg/util/startstop"
"github.com/DataDog/datadog-agent/pkg/util/winutil/servicemain"
)
Expand Down Expand Up @@ -166,9 +165,9 @@ func (s *service) Run(svcctx context.Context) error {
statusimpl.Module(),

fetchonlyimpl.Module(),
configsyncimpl.OptionalModule(),
configsyncimpl.Module(),
// Force the instantiation of the component
fx.Invoke(func(_ optional.Option[configsync.Component]) {}),
fx.Invoke(func(_ configsync.Component) {}),
autoexitimpl.Module(),
fx.Provide(func(c config.Component) settings.Params {
return settings.Params{
Expand Down
5 changes: 2 additions & 3 deletions cmd/security-agent/subcommands/start/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ import (
"github.com/DataDog/datadog-agent/pkg/status/health"
"github.com/DataDog/datadog-agent/pkg/util/coredump"
"github.com/DataDog/datadog-agent/pkg/util/fxutil"
"github.com/DataDog/datadog-agent/pkg/util/optional"
"github.com/DataDog/datadog-agent/pkg/util/profiling"
"github.com/DataDog/datadog-agent/pkg/util/startstop"
"github.com/DataDog/datadog-agent/pkg/version"
Expand Down Expand Up @@ -174,9 +173,9 @@ func Commands(globalParams *command.GlobalParams) []*cobra.Command {
}),
statusimpl.Module(),
fetchonlyimpl.Module(),
configsyncimpl.OptionalModule(),
configsyncimpl.Module(),
// Force the instantiation of the component
fx.Invoke(func(_ optional.Option[configsync.Component]) {}),
fx.Invoke(func(_ configsync.Component) {}),
autoexitimpl.Module(),
fx.Supply(pidimpl.NewParams(params.pidfilePath)),
fx.Provide(func(c config.Component) settings.Params {
Expand Down
4 changes: 2 additions & 2 deletions cmd/trace-agent/subcommands/run/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ func runTraceAgentProcess(ctx context.Context, cliParams *Params, defaultConfPat
zstdfx.Module(),
trace.Bundle(),
fetchonlyimpl.Module(),
configsyncimpl.OptionalModule(),
configsyncimpl.Module(),
// Force the instantiation of the components
fx.Invoke(func(_ traceagent.Component, _ optional.Option[configsync.Component], _ autoexit.Component) {}),
fx.Invoke(func(_ traceagent.Component, _ configsync.Component, _ autoexit.Component) {}),
)
if err != nil && errors.Is(err, traceagentimpl.ErrAgentDisabled) {
return nil
Expand Down
26 changes: 14 additions & 12 deletions comp/core/configsync/configsyncimpl/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
log "github.com/DataDog/datadog-agent/comp/core/log/def"
apiutil "github.com/DataDog/datadog-agent/pkg/api/util"
"github.com/DataDog/datadog-agent/pkg/util/fxutil"
"github.com/DataDog/datadog-agent/pkg/util/optional"
)

type dependencies struct {
Expand All @@ -35,20 +34,20 @@ type dependencies struct {
SyncParams Params
}

// OptionalModule defines the fx options for this component.
func OptionalModule() fxutil.Module {
// Module defines the fx options for this component.
func Module() fxutil.Module {
return fxutil.Component(
fx.Provide(newOptionalConfigSync),
fx.Provide(newComponent),
fx.Supply(Params{}),
)
}

// OptionalModuleWithParams defines the fx options for this component, but
// ModuleWithParams defines the fx options for this component, but
// requires additionally specifying custom Params from the fx App, to be
// passed to the constructor.
func OptionalModuleWithParams() fxutil.Module {
func ModuleWithParams() fxutil.Module {
return fxutil.Component(
fx.Provide(newOptionalConfigSync),
fx.Provide(newComponent),
)
}

Expand All @@ -61,19 +60,21 @@ type configSync struct {
client *http.Client
connected bool
ctx context.Context
enabled bool
}

// newOptionalConfigSync checks if the component was enabled as per the config, and returns an optional.Option
func newOptionalConfigSync(deps dependencies) optional.Option[configsync.Component] {
// newComponent checks if the component was enabled as per the config and return a enable/disabled configsync
func newComponent(deps dependencies) configsync.Component {
agentIPCPort := deps.Config.GetInt("agent_ipc.port")
configRefreshIntervalSec := deps.Config.GetInt("agent_ipc.config_refresh_interval")

if agentIPCPort <= 0 || configRefreshIntervalSec <= 0 {
return optional.NewNoneOption[configsync.Component]()
deps.Log.Infof("configsync disabled (agent_ipc.port: %d | agent_ipc.config_refresh_interval: %d)", agentIPCPort, configRefreshIntervalSec)
return configSync{}
}

configSync := newConfigSync(deps, agentIPCPort, configRefreshIntervalSec)
return optional.NewOption(configSync)
deps.Log.Infof("configsync enabled (agent_ipc '%s:%d' | agent_ipc.config_refresh_interval: %d)", deps.Config.GetString("agent_ipc.host"), agentIPCPort, configRefreshIntervalSec)
return newConfigSync(deps, agentIPCPort, configRefreshIntervalSec)
}

// newConfigSync creates a new configSync component.
Expand All @@ -98,6 +99,7 @@ func newConfigSync(deps dependencies, agentIPCPort int, configRefreshIntervalSec
url: url,
client: client,
ctx: ctx,
enabled: true,
}

if deps.SyncParams.OnInit {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"github.com/DataDog/datadog-agent/comp/core/config"
"github.com/DataDog/datadog-agent/comp/core/configsync"
"github.com/DataDog/datadog-agent/pkg/util/fxutil"
"github.com/DataDog/datadog-agent/pkg/util/optional"
)

func TestOptionalModule(t *testing.T) {
Expand All @@ -44,16 +43,14 @@ func TestOptionalModule(t *testing.T) {
"agent_ipc.port": port,
"agent_ipc.config_refresh_interval": 1,
}
csopt := fxutil.Test[optional.Option[configsync.Component]](t, fx.Options(
comp := fxutil.Test[configsync.Component](t, fx.Options(
core.MockBundle(),
fetchonlyimpl.Module(),
OptionalModule(),
Module(),
fx.Populate(&cfg),
fx.Replace(config.MockParams{Overrides: overrides}),
))

_, ok := csopt.Get()
require.True(t, ok)
require.True(t, comp.(configSync).enabled)

require.EventuallyWithT(t, func(t *assert.CollectT) {
assert.Equal(t, "value1", cfg.Get("key1"))
Expand Down
20 changes: 8 additions & 12 deletions comp/core/configsync/configsyncimpl/module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,34 +8,30 @@ package configsyncimpl
import (
"testing"

"github.com/stretchr/testify/require"

pkgconfigmodel "github.com/DataDog/datadog-agent/pkg/config/model"
"github.com/stretchr/testify/assert"
)

func TestNewOptionalConfigSync(t *testing.T) {
func TestNewConfigSync(t *testing.T) {
t.Run("enabled", func(t *testing.T) {
deps := makeDeps(t)
deps.Config.Set("agent_ipc.port", 1234, pkgconfigmodel.SourceFile)
deps.Config.Set("agent_ipc.config_refresh_interval", 30, pkgconfigmodel.SourceFile)
optConfigSync := newOptionalConfigSync(deps)
_, ok := optConfigSync.Get()
require.True(t, ok)
comp := newComponent(deps)
assert.True(t, comp.(configSync).enabled)
})

t.Run("disabled ipc port zero", func(t *testing.T) {
deps := makeDeps(t)
deps.Config.Set("agent_ipc.port", 0, pkgconfigmodel.SourceFile)
optConfigSync := newOptionalConfigSync(deps)
_, ok := optConfigSync.Get()
require.False(t, ok)
comp := newComponent(deps)
assert.False(t, comp.(configSync).enabled)
})

t.Run("disabled config refresh interval zero", func(t *testing.T) {
deps := makeDeps(t)
deps.Config.Set("agent_ipc.config_refresh_interval", 0, pkgconfigmodel.SourceFile)
optConfigSync := newOptionalConfigSync(deps)
_, ok := optConfigSync.Get()
require.False(t, ok)
comp := newComponent(deps)
assert.False(t, comp.(configSync).enabled)
})
}
Loading