Export entire smaps memory metrics instead of only referenced_memory

[JensErat]

#2495 introduced parsing smaps memory metrics, but only exposes referenced memory. As proposed in #2634, also other metrics in here are of interest, specifically LazyFree; but generally also the others seem of interest.

This commit replaces the referenced memory metrics by a generic smaps series of metrics. Runtime costs in cadvisor are not really impacted, as all the smaps parsing already happens anyway. The cardinality per container extends from 1 to 19 metrics, though. I think this is acceptable: also referenced memory scraping was disabled by default, so is scraping smaps.

Resolves #2634. It slightly extends the original proposal by simply exposing all metrics, but once I started work on the proposal it felt reasonable to do so. I'm fine with major changes, though!

While the code works (and the change is actually smaller than it looks, most of the lines only affect unit tests), I guess there is definitely more work to do.

• all unit tests run successfully, let's wait for the entire integration test suite
• do we want this approach at all?
• do we need downwards compatibility for referenced_memory?

As soon as there is agreement on the chosen approach, tasks needed to make merge-ready:

• some cleanup work, maybe extract (or get rid of) the array of smaps labels
• check whether documentation is required somewhere

This PR likely also affects #2715, which proposes optimizations to smaps data collection.

^{Jens Erat [email protected], Daimler TSS GmbH, imprint}

[k8s-ci-robot]

Hi @JensErat. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[iwankgb]

Removing ContainerStats.ReferencedMemory breaks backward compatibility. I'm afraid we will have to keep it, for a while at least.

[JensErat]

It might even be reasonable to exclude it from the smaps metrics, or keep it duplicate forever. From what I understand, all other smaps metrics do not get reset using this referenced-cycle-count-reset-logic, but I'll have to investigate further. So referenced is somewhat different from all the other metrics. Everything else has "proper" bookkeeping, the referenced counter is somewhat an estimate based on page access.

Adding the referenced metric again is a matter of minutes and adjusting some tests. I think some early feedback on the general approach (and matters like compatiblity) is very important though.

[iwankgb]

/ok-to-test

[iwankgb]

@bobbypage I think it's something worth being discussed. As far as I can see in #2634 @dashpole was fine with introducing such changes. They look reasonable to me too if we keep backward compatibility.

[JensErat]

@bobbypage @dashpole @iwankgb any news on how/whether to proceed? I was just puzzled once again because of high memory usage that boiled down to LazyFree -- even if you know about it you get surprised every now and then.

[iwankgb]

It would be great to have container_referenced_bytes still available, I think. Some folks might be already using it.
I'm just finishing very intense sprint at work and I should be able to be more active here again.

[Creatone]

Feel free to reopen this PR if you will continue work.