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

Add Prometheus metrics to count ANP and ACNP Status updates #1801

Conversation

antoninbas
Copy link
Contributor

Too frequent Status updates could generate too many versions of the CRD,
that would need to be stored in etcd until the next compaction by
kube-apiserver. Too many updates could also cause fragmentation of the
database. It is useful to have access to the number of updates over time
in production clusters.

Too frequent Status updates could generate too many versions of the CRD,
that would need to be stored in etcd until the next compaction by
kube-apiserver. Too many updates could also cause fragmentation of the
database. It is useful to have access to the number of updates over time
in production clusters.
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

I am ok with the change. Feel people might not understand what such metrics are for, but probably no harm either.

@abhiraut
Copy link
Contributor

does update to the Status sub-resource generate a new version? i thought the generation count remained same.
but maybe it still updates the resourceVersion even if it does not generate a new resource.

@antoninbas
Copy link
Contributor Author

does update to the Status sub-resource generate a new version? i thought the generation count remained same.
but maybe it still updates the resourceVersion even if it does not generate a new resource.

I believe ResourceVersion is incremented and there is definitely a write to etcd, but we can wait for @tnqn to confirm and comment on the usefulness of this PR

@tnqn
Copy link
Member

tnqn commented Feb 3, 2021

does update to the Status sub-resource generate a new version? i thought the generation count remained same.
but maybe it still updates the resourceVersion even if it does not generate a new resource.

I believe ResourceVersion is incremented and there is definitely a write to etcd, but we can wait for @tnqn to confirm and comment on the usefulness of this PR

Yes, generation is different from resourceVersion. Any update to any field will lead to a new version and a write to etcd. Although the original issue is not caused by status update, this metrics can help rule it out quickly and generally help us understand the overhead of status manager so I'm good to add them.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas
Copy link
Contributor Author

Thanks for the feedback @tnqn

@antoninbas
Copy link
Contributor Author

/test-all

@codecov-io
Copy link

codecov-io commented Feb 3, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@57dcaec). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1801   +/-   ##
=======================================
  Coverage        ?   26.49%           
=======================================
  Files           ?      179           
  Lines           ?    15169           
  Branches        ?        0           
=======================================
  Hits            ?     4019           
  Misses          ?    10625           
  Partials        ?      525           
Flag Coverage Δ
e2e-tests 26.49% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@antoninbas antoninbas merged commit 9a8939a into antrea-io:main Feb 3, 2021
@antoninbas antoninbas deleted the add-prometheus-metrics-to-count-anp-and-acnp-status-updates branch February 3, 2021 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants