Skip to content

Commit

Permalink
Remove optional from configsync in favor of an internal enabled/disab…
Browse files Browse the repository at this point in the history
…led state

The caller should not care if configsync is enabled or not. There is no
need to expose the internal state of the component to the use since it
has no public method.
  • Loading branch information
hush-hush committed Dec 16, 2024
1 parent 35a9baf commit 5517946
Show file tree
Hide file tree
Showing 8 changed files with 31 additions and 35 deletions.
10 changes: 5 additions & 5 deletions cmd/otel-agent/subcommands/run/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,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 @@ -205,7 +205,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 @@ -223,13 +223,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 @@ -54,7 +54,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 @@ -164,9 +163,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 @@ -65,7 +65,6 @@ import (
"github.com/DataDog/datadog-agent/pkg/status/health"
"github.com/DataDog/datadog-agent/pkg/util"
"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 @@ -173,9 +172,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
24 changes: 12 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(newConfigSync),
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(newConfigSync),
)
}

Expand All @@ -63,17 +62,18 @@ type configSync struct {
ctx context.Context
}

// newOptionalConfigSync checks if the component was enabled as per the config, and returns an optional.Option
func newOptionalConfigSync(deps dependencies) optional.Option[configsync.Component] {
// newConfigSync checks if the component was enabled as per the config and return a enable/disabled configsync
func newConfigSync(deps dependencies) configsync.Component {

Check failure on line 66 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (javascript)

other declaration of newConfigSync

Check failure on line 66 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (python)

other declaration of newConfigSync

Check failure on line 66 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (cpp)

other declaration of newConfigSync
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{}

Check failure on line 72 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (javascript)

configsync is not a type

Check failure on line 72 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (python)

configsync is not a type

Check failure on line 72 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (cpp)

configsync is not a type
}

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)

Check failure on line 76 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (javascript)

too many arguments in call to newConfigSync

Check failure on line 76 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (python)

too many arguments in call to newConfigSync

Check failure on line 76 in comp/core/configsync/configsyncimpl/module.go

View workflow job for this annotation

GitHub Actions / CodeQL-Build (cpp)

too many arguments in call to newConfigSync
}

// newConfigSync creates a new configSync component.
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,10 +43,10 @@ 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(
csopt := fxutil.Test[configsync.Component](t, fx.Options(
core.MockBundle(),
fetchonlyimpl.Module(),
OptionalModule(),
Module(),
fx.Populate(&cfg),
fx.Replace(config.MockParams{Overrides: overrides}),
))
Expand Down
8 changes: 4 additions & 4 deletions comp/core/configsync/configsyncimpl/module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,28 @@ import (
pkgconfigmodel "github.com/DataDog/datadog-agent/pkg/config/model"
)

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)
optConfigSync := newConfigSync(deps)
_, ok := optConfigSync.Get()
require.True(t, ok)
})

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)
optConfigSync := newConfigSync(deps)
_, ok := optConfigSync.Get()
require.False(t, ok)
})

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)
optConfigSync := newConfigSync(deps)
_, ok := optConfigSync.Get()
require.False(t, ok)
})
Expand Down

0 comments on commit 5517946

Please sign in to comment.