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 domain label from VMI metrics #3500

Closed
wants to merge 24 commits into from

Conversation

ArthurSens
Copy link
Contributor

What this PR does / why we need it:
This PR removes domain labels from VMI metrics. They are being removed because they originate from the original metrics implementation where the more useful labels like name and namespace did not exist and now can be removed

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3477

Special notes for your reviewer:

Release note:

This change will possibly break dashboards that use the domain label to identify VMIs. 

action required
Use name and namespace labels instead

@kubevirt-bot kubevirt-bot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Jun 1, 2020
@dhiller
Copy link
Contributor

dhiller commented Jun 2, 2020

@ArthurSens
Copy link
Contributor Author

/retest

2 similar comments
@ArthurSens
Copy link
Contributor Author

/retest

@ArthurSens
Copy link
Contributor Author

/retest

@openshift-ci-robot
Copy link
Collaborator

@ArthurSens: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-azure-nested-virt 1d3f694 link /test e2e-azure-nested-virt

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@xpivarc
Copy link
Member

xpivarc commented Jul 2, 2020

@ArthurSens Can you squash commits? Anything else you want to do about this?

@ArthurSens
Copy link
Contributor Author

After discussing with @fabiand and @danielBelenky, I think we might want to send a deprecation note about this label before applying this patch, I don't know if it is possible to merge without adding this patch to the next release though

I don't know the procedure for this case 😬

@kubevirt-bot
Copy link
Contributor

@ArthurSens: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2020
awels and others added 14 commits July 17, 2020 10:45
…s debug issues.

Signed-off-by: Alexander Wels <awels@redhat.com>
Ginkgo containers body are evaluated at definition, i.e. at a pre-main
stage. Running code at this stage is not recommended as some
initializations are occurring only in the test `main` (which is
auto-generated by the test framework).

An example of such a scenario is the command line flag parsing.
Libraries, tested code and the test infra is expected to declare the
flags and the parsing is expected to be execute only once (at main
usually).

This change moves all code that executes in the containers body to the
Before* setup bodies, while leaving on the containers body the variable
declarations.

Signed-off-by: Edward Haas <edwardh@redhat.com>
Current command line flags are setup at `init` but parsed multiple
times by each test file.

As the flags are bound to global variables and flag parsing should only
be executed once (consequent calls are no-op), there is no point to execute
the parse more than once.
The test framework is expected to execute the parsing in its main
function for its own flags, therefore, there should be no need to
explicitly parse the flags in the tests.

Signed-off-by: Edward Haas <edwardh@redhat.com>
Signed-off-by: Edward Haas <edwardh@redhat.com>
Basic implementation that adds all k8s metadata.labels and metadata.annotations into a metric labels, separated by commas(',')

Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
The labels slice was repeating elements after the second scrape

Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Also correction of tryToPushMetric function which was always using memoryAvailableDesc to log errors

Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
ArthurSens and others added 10 commits July 17, 2020 10:45
Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Also, metrics' labelset will only be populated when the domain stat is set

Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
This reverts commit bf4b94d, reversing
changes made to fcec63f.

Signed-off-by: Or Shoval <oshoval@redhat.com>
This reverts commit ec0b542.

Signed-off-by: Or Shoval <oshoval@redhat.com>
Changing emphemeral disk perm to 640.
Changing container disk perm to 444.

Signed-off-by: L. Pivarc <lpivarc@redhat.com>
Signed-off-by: arthursens <arthursens2005@gmail.com>
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign danielbelenky
You can assign the PR to them by writing /assign @danielbelenky in a comment when ready.

The full list of commands accepted by this bot can be found 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

@kubevirt-bot
Copy link
Contributor

@ArthurSens: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubevirt-e2e-kind-k8s-1.17.0-ipv6 8d2d7c5 link /test pull-kubevirt-e2e-kind-k8s-1.17.0-ipv6
pull-kubevirt-e2e-k8s-1.16 8d2d7c5 link /test pull-kubevirt-e2e-k8s-1.16
pull-kubevirt-e2e-k8s-cnao-1.17 8d2d7c5 link /test pull-kubevirt-e2e-k8s-cnao-1.17
pull-kubevirt-e2e-k8s-1.17 8d2d7c5 link /test pull-kubevirt-e2e-k8s-1.17
pull-kubevirt-e2e-k8s-1.18 8d2d7c5 link /test pull-kubevirt-e2e-k8s-1.18
pull-kubevirt-e2e-windows2016 8d2d7c5 link /test pull-kubevirt-e2e-windows2016
pull-kubevirt-check-tests-for-flakes 8d2d7c5 link /test pull-kubevirt-check-tests-for-flakes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ArthurSens
Copy link
Contributor Author

Oh god I just messed up my rebase real bad
I'll just open a new PR for this

@ArthurSens ArthurSens closed this Jul 17, 2020
@ArthurSens ArthurSens deleted the remove_domain_label branch July 17, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove domain label from Metrics
8 participants