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

Jump to bottom

Provide a basic test for secondary interface naming metrics #24998

Merged
merged 1 commit into from
Jun 26, 2020
Merged

Provide a basic test for secondary interface naming metrics #24998

merged 1 commit into from
Jun 26, 2020

Conversation

fedepaol
Copy link
Member

This is a follow up to openshift/cluster-network-operator#552 , testing for the presence of the basic metric produced by the new daemonset.

The test creates a new secondary network using the networks.operator.openshift.io crd and verifies the presence of the metrics for both the primary and the secondary interface.

@fedepaol
Copy link
Member Author

/hold
Depends on openshift/cluster-network-operator#552

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2020
lilic
lilic reviewed May 20, 2020
View reviewed changes
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Curious does this needs to be part of the origin tests or can you just add them to the cluster-network-operator?

@fedepaol
Copy link
Member Author

Curious does this needs to be part of the origin tests or can you just add them to the cluster-network-operator?

Yes, I was not 100% sure about this but:

  • I think I was pointed here in the past, as the right place to add new prometheus tests.
  • My reasoning is that we want to test the functionality in the platform. This is not much different as as validating the presence of openshift_sdn_ovs_flows or ingress_metrics which is being done here.
  • By putting these tests in the CNO (or possibly different, in the daemon that produces them) we'll be able to gate the changes only in the single repository. Moreover, I am not seeing e2e tests at all in the CNO repo.

@lilic
Copy link
Contributor

lilic commented May 20, 2020

Moreover, I am not seeing e2e tests at all in the CNO repo.

:) interesting, we have e2e tests in our cluster-monitoring-operator, for basic functionality.

I am not against adding it here just curious if we can just keep it for the component.

@fedepaol
Copy link
Member Author

fedepaol commented Jun 8, 2020

/hold cancel
openshift/cluster-network-operator#552 is merged

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2020
@fedepaol
Copy link
Member Author

fedepaol commented Jun 8, 2020

/retest

@fedepaol fedepaol force-pushed the networkmetricsqueries branch from bfb978d to b3209c4 Compare June 8, 2020 09:11
@fedepaol
Copy link
Member Author

fedepaol commented Jun 8, 2020

/retest

1 similar comment
@fedepaol
Copy link
Member Author

fedepaol commented Jun 8, 2020

/retest

@fedepaol
Copy link
Member Author

fedepaol commented Jun 8, 2020

@lilic feature was merged, do you mind giving a look?

@fedepaol
Copy link
Member Author

fedepaol commented Jun 8, 2020

/retest

1 similar comment
@fedepaol
Copy link
Member Author

fedepaol commented Jun 9, 2020

/retest

@fedepaol
Copy link
Member Author

fedepaol commented Jun 9, 2020

/test e2e-cmd

@fedepaol fedepaol force-pushed the networkmetricsqueries branch from b3209c4 to 5584613 Compare June 10, 2020 13:08
lilic
lilic reviewed Jun 10, 2020
View reviewed changes
test/extended/prometheus/prometheus.go Outdated
queries := map[string]bool{
`template_router_reload_seconds_count{job="router-internal-default"} >= 1`: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

were these metrics removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at all, thanks for pointing it out!

@fedepaol
Add basic tests for secondary network metrics.
0c5a057 
Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
@fedepaol fedepaol force-pushed the networkmetricsqueries branch from 5584613 to 0c5a057 Compare June 10, 2020 16:00
lilic
lilic approved these changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020
@smarterclayton
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fedepaol, lilic, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

19 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 426bf57 into openshift:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilic lilic lilic approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@lilic lilic

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fedepaol @lilic @smarterclayton @openshift-ci-robot @openshift-bot @openshift-merge-robot