-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[NDM][NDMII-2748] test snmp tags on agent restart #26003
Changes from 5 commits
8c1b013
344016c
69c9d21
2fd8dfd
8644d17
b80d861
45ae6cc
d8de7c0
bcdd43c
114b9cb
0cbe423
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,4 +4,4 @@ init_config: | |
instances: | ||
- ip_address: dd-snmp | ||
port: 1161 | ||
community_string: "public" | ||
community_string: "cisco-nexus" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🥜 nitpick There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed b80d861 |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,13 +8,17 @@ package snmp | |||||||||
|
||||||||||
import ( | ||||||||||
"embed" | ||||||||||
"fmt" | ||||||||||
"path" | ||||||||||
"testing" | ||||||||||
"time" | ||||||||||
|
||||||||||
"github.com/DataDog/datadog-agent/test/fakeintake/aggregator" | ||||||||||
"github.com/DataDog/datadog-agent/test/fakeintake/client" | ||||||||||
"github.com/DataDog/datadog-agent/test/new-e2e/pkg/e2e" | ||||||||||
"github.com/DataDog/datadog-agent/test/new-e2e/pkg/environments" | ||||||||||
"github.com/stretchr/testify/assert" | ||||||||||
"github.com/stretchr/testify/require" | ||||||||||
|
||||||||||
"github.com/DataDog/test-infra-definitions/components/datadog/agent" | ||||||||||
"github.com/DataDog/test-infra-definitions/components/datadog/dockeragentparams" | ||||||||||
|
@@ -155,3 +159,40 @@ func (s *snmpDockerSuite) TestSnmp() { | |||||||||
assert.Contains(c, metrics, "snmp.sysUpTimeInstance", "metrics %v doesn't contain snmp.sysUpTimeInstance", metrics) | ||||||||||
}, 5*time.Minute, 10*time.Second) | ||||||||||
} | ||||||||||
|
||||||||||
func (s *snmpDockerSuite) TestSnmpTagsAreStoredOnRestart() { | ||||||||||
fakeintake := s.Env().FakeIntake.Client() | ||||||||||
initialMetrics, err := fakeintake.FilterMetrics("snmp.device.reachable", | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be safe from potentially flaky test failures I think FIlterMetrics() should be inside an EventuallyWithT(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed d8de7c0 |
||||||||||
client.WithTags[*aggregator.MetricSeries]([]string{}), | ||||||||||
) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed 45ae6cc |
||||||||||
require.NoError(s.T(), err) | ||||||||||
|
||||||||||
initialTags := initialMetrics[0].Tags | ||||||||||
_, err = s.Env().RemoteHost.Execute("docker stop dd-snmp") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 suggestion
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be tested: // stop
s.Env().Docker.GetClient().ContainerStop(context.Background(), "dd-snmp", container.StopOptions{})
// restart
s.Env().Docker.GetClient().ContainerRestart(context.Background(), s.Env().Agent.ContainerName, container.StopOptions{}) But I think you need to get the ID from the container name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checked and indeed you would need to get the container ID, will create a card to provide this through the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I tried before to get the list of container ids filtered by the name and then get the first one from the list but it is maybe easier like this |
||||||||||
require.NoError(s.T(), err) | ||||||||||
|
||||||||||
err = fakeintake.FlushServerAndResetAggregators() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed both before and after the agent restart? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed the first one ! 45ae6cc |
||||||||||
require.NoError(s.T(), err) | ||||||||||
|
||||||||||
_, err = s.Env().RemoteHost.Execute(fmt.Sprintf("docker restart %s", s.Env().Agent.ContainerName)) | ||||||||||
require.NoError(s.T(), err) | ||||||||||
|
||||||||||
err = fakeintake.FlushServerAndResetAggregators() | ||||||||||
require.NoError(s.T(), err) | ||||||||||
|
||||||||||
var metrics []*aggregator.MetricSeries | ||||||||||
|
||||||||||
require.EventuallyWithT(s.T(), func(t *assert.CollectT) { | ||||||||||
metrics, err = fakeintake.FilterMetrics("snmp.device.reachable", | ||||||||||
client.WithTags[*aggregator.MetricSeries]([]string{}), | ||||||||||
) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same remark as above.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed 45ae6cc |
||||||||||
require.NoError(t, err) | ||||||||||
assert.NotEmpty(t, metrics) | ||||||||||
}, 5*time.Minute, 5*time.Second) | ||||||||||
|
||||||||||
tags := metrics[0].Tags | ||||||||||
|
||||||||||
require.Zero(s.T(), metrics[0].Points[0].Value) | ||||||||||
require.ElementsMatch(s.T(), tags, initialTags) | ||||||||||
jmw51798 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
require.Len(s.T(), tags, 9) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider removing this check since the number of tags could change and it probably doesn't check anything beyond what was already checked by the previous ElementsMatch(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed 45ae6cc |
||||||||||
} |
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.
👏 praise