Common flag env metadata whitelist #2921

Colstuwjx · 2021-08-16T09:17:06Z

This is a follow-up PR for #2330, after I proposed that PR, I've seen a contributor submitted PR #2857 in order to introduce containerd whitelist env flag.

Now I merged the containerd and docker env whitelist flags, and just a simple common flag: env_metadata_whitelist, the other two are deprecated while this PR merged.

BTW, I'm not sure about the state of the Mesos container, just keep the same interface signature as well :(

k8s-ci-robot · 2021-08-16T09:17:14Z

Hi @Colstuwjx. 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.

Colstuwjx · 2021-08-16T09:23:43Z

cmd/internal/container/mesos/handler_test.go

@@ -22,7 +22,6 @@ import (
 	containerlibcontainer "github.com/google/cadvisor/container/libcontainer"
 	"github.com/google/cadvisor/fs"
 	info "github.com/google/cadvisor/info/v1"
-	"github.com/mesos/mesos-go/api/v1/lib"


I found CI error after removed this line:

Error: vet: internal/container/mesos/handler_test.go:31:21: undeclared name: mesos Error: make: *** [Makefile:71: vet] Error 2 Error: Process completed with exit code 2.

But I have no idea about the reason. Does it only use by our tests?

Colstuwjx · 2021-08-16T09:37:49Z

/cc @dashpole @bobbypage

It seems no meaningful message to debug :(

+ echo 'Deleting /tmp/tmp.DrWtfAH0Ul...'
+ [[ 1001 -ne 0 ]]
+ sudo rm -rf /tmp/tmp.DrWtfAH0Ul
make: *** [Makefile:54: docker-test-integration] Error 143
Error: Process completed with exit code 2.

eero-t · 2021-08-16T15:52:39Z

It seems no meaningful message to debug :(

This is the actual problem:

 Waiting for cAdvisor to start ...
!! cAdvisor exited unexpectedly with Exit 0
+ EXIT_VALUE=143

As there's no output, this could be early at startup?

Colstuwjx · 2021-08-17T01:51:20Z

As there's no output, this could be early at startup?

I found an issue that the integration tests are still using the deprecated flag docker_env_metadata_whitelist, I've been fixed them.

BTW, the integration tests should output the logs if there are errors, I'll try to propose another PR for fixing it.

bobbypage · 2021-08-17T02:31:22Z

This looks good overall. One thing I'm a bit worried about is removing the existing --containerd_env_metadata_whitelist and --docker_env_metadata_whitelist since it is a breaking change for users. Perhaps we can keep these flags around and just set them equal to --env_metadata_whitelist and leave a note that they are deprecated. Then in a release or two we can fully remove them. What do you think?

Colstuwjx · 2021-08-17T02:34:11Z

Thanks @Colstuwjx for the PR. Do you mind splitting the fix to integration test out into a separate PR so we can unblock this and other PRs in parallel? Thanks!

What do you mean? The integration test-related changes in this PR just changed the flag docker_env_metadata_whitelist to env_metadata_whitelist. I think it's reasonable for including these changes inside this PR, otherwise, it would panic our integration tests as we've seen.

Perhaps we can keep these flags around and just set them equal to --env_metadata_whitelist and leave a note that they are deprecated. Then in a release or two we can fully remove them.

I've been considered this path, it's ok to me, I'll add these deprecated flags back soon.

Colstuwjx · 2021-08-17T03:25:37Z

Ping @bobbypage I've been added the deprecated flags back! Please take a review :)

bobbypage · 2021-08-20T02:10:18Z

/retest

bobbypage · 2021-08-20T03:59:38Z

container/factory.go

@@ -27,7 +27,7 @@ import (

 type ContainerHandlerFactory interface {
 	// Create a new ContainerHandler using this factory. CanHandleAndAccept() must have returned true.
-	NewContainerHandler(name string, inHostNamespace bool) (c ContainerHandler, err error)
+	NewContainerHandler(name string, metadataEnvs []string, inHostNamespace bool) (c ContainerHandler, err error)


can we rename metadataEnvs to metadataEnvAllowList or something like that? From the name, it's not clear that we are only going envs in metadataEnvs

bobbypage · 2021-08-20T04:02:20Z

metrics/prometheus.go

@@ -1856,6 +1856,9 @@ func BaseContainerLabels(whiteList []string) func(container *info.ContainerInfo)
 				set[ContainerLabelPrefix+k] = v
 			}
 		}
+		for k, v := range container.Spec.Envs {


Is this change needed? It seems like this is change in behavior of adding env to Prometheus labels. I'm not sure how this change is relevant here?

It seems like this is change in behavior of adding env to Prometheus labels.

Yes. You could get to know the background from PR 2330 #issue-comment . PR #2330 is original to add container whitelisted envs to prom labels, and @dashpole think it'd better to unify the flags, so I proposed this PR righ now!

If you think it'd better to separate these changes, and make the env changes an individual PR, I'm ok with it :)

Got it, thanks for the explanation.

bobbypage · 2021-08-20T04:03:25Z

Thanks for updating @Colstuwjx. Overall LGTM, minor nit about naming and also question around why we need to change the labels provided in Prometheus (which seems unrelated to refactoring done here). Thanks!

bobbypage · 2021-08-24T00:55:54Z

Please rebase to fix conflicts and update as per #2921 (comment). Thanks!

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

Colstuwjx · 2021-08-24T01:58:10Z

Hi @bobbypage

Now I revised the PR changes according to your suggestions! Please take a look again.
Once this PR gets merged, I will take another PR for adding container envs as labels in the prom metrics.

bobbypage · 2021-08-25T22:13:53Z

LGTM, thanks!

bobbypage · 2021-08-25T22:13:59Z

/retest

Waiting for pull-cadvisor-e2e to go green

Colstuwjx · 2021-08-26T00:27:30Z

The error logs shown below:

W0825 22:19:17.344] subprocess.CalledProcessError: Command '('kubetest', '--dump=/workspace/_artifacts', '--gcp-service-account=/etc/service-account/service-account.json', '--up', '--down', '--test', '--deployment=node', '--provider=gce', '--cluster=bootstrap-e2e', '--gcp-network=bootstrap-e2e', '--gcp-project=cadvisor-e2e', '--gcp-zone=us-central1-f', '--node-args=--image-config-file=/workspace/test-infra/jobs/e2e_node/image-config.yaml --test-suite=cadvisor', '--node-tests=true', '--test_args=--nodes=1', '--timeout=10m')' returned non-zero exit status 1

I'm not sure if it's related to this PR.

bobbypage · 2021-08-26T20:47:45Z

I think CI was failing due to lack of go1.17 upgrade (since the pull-cadvisor-e2e runs in prow with k/k). It should be fixed now after #2929

bobbypage · 2021-08-26T20:47:51Z

/retest

bobbypage · 2021-08-26T20:58:28Z

Thanks, LGTM!

Colstuwjx · 2021-08-27T01:44:45Z

I'm not sure why this PR wasn't been squashed, maybe we could let bot merge the PR, just like k8s projects did?

google-cla bot added the cla: yes label Aug 16, 2021

k8s-ci-robot added the needs-ok-to-test label Aug 16, 2021

Colstuwjx commented Aug 16, 2021

View reviewed changes

bobbypage reviewed Aug 20, 2021

View reviewed changes

Colstuwjx added 9 commits August 24, 2021 09:32

Unified the env metadata whitelist flag for containerd and docker.

49d243e

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

Carry env labels within metric if added whitelist envs.

f87334e

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

fix ci: add the mesos lib import back.

54bb15a

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

chore: no need to add this sentence.

0bab947

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

fix: fixed integration tests and revise cmd flag doc.

1518474

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

chore: revise the doc.

73376cb

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

Add deprecated flags back and add deprecated warnings.

dcf13bc

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

chore: change metadataEnvs to metadataEnvAllowList.

aa0afbe

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

revert: rollback prom metrics container_env changes.

f448e00

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

Colstuwjx force-pushed the common-flag-env-metadata-whitelist branch from 44619ec to f448e00 Compare August 24, 2021 01:47

Minor fix.

058347d

Signed-off-by: colstuwjx <colstuwjx@gmail.com>

bobbypage approved these changes Aug 26, 2021

View reviewed changes

bobbypage merged commit 2dd0cd9 into google:master Aug 26, 2021

Colstuwjx deleted the common-flag-env-metadata-whitelist branch August 27, 2021 01:42

Colstuwjx mentioned this pull request Aug 27, 2021

feat: add container whitelisted envs as metrics labels. #2930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common flag env metadata whitelist #2921

Common flag env metadata whitelist #2921

Colstuwjx commented Aug 16, 2021

k8s-ci-robot commented Aug 16, 2021

Colstuwjx Aug 16, 2021 •

edited

Loading

Colstuwjx commented Aug 16, 2021

eero-t commented Aug 16, 2021

Colstuwjx commented Aug 17, 2021

bobbypage commented Aug 17, 2021

Colstuwjx commented Aug 17, 2021

Colstuwjx commented Aug 17, 2021

bobbypage commented Aug 20, 2021

bobbypage Aug 20, 2021

Colstuwjx Aug 22, 2021

bobbypage Aug 20, 2021

Colstuwjx Aug 22, 2021

bobbypage Aug 24, 2021

bobbypage commented Aug 20, 2021 •

edited

Loading

bobbypage commented Aug 24, 2021

Colstuwjx commented Aug 24, 2021

bobbypage commented Aug 25, 2021

bobbypage commented Aug 25, 2021 •

edited

Loading

Colstuwjx commented Aug 26, 2021

bobbypage commented Aug 26, 2021 •

edited

Loading

bobbypage commented Aug 26, 2021

bobbypage commented Aug 26, 2021

Colstuwjx commented Aug 27, 2021

Common flag env metadata whitelist #2921

Common flag env metadata whitelist #2921

Conversation

Colstuwjx commented Aug 16, 2021

k8s-ci-robot commented Aug 16, 2021

Colstuwjx Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Colstuwjx commented Aug 16, 2021

eero-t commented Aug 16, 2021

Colstuwjx commented Aug 17, 2021

bobbypage commented Aug 17, 2021

Colstuwjx commented Aug 17, 2021

Colstuwjx commented Aug 17, 2021

bobbypage commented Aug 20, 2021

bobbypage Aug 20, 2021

Choose a reason for hiding this comment

Colstuwjx Aug 22, 2021

Choose a reason for hiding this comment

bobbypage Aug 20, 2021

Choose a reason for hiding this comment

Colstuwjx Aug 22, 2021

Choose a reason for hiding this comment

bobbypage Aug 24, 2021

Choose a reason for hiding this comment

bobbypage commented Aug 20, 2021 • edited Loading

bobbypage commented Aug 24, 2021

Colstuwjx commented Aug 24, 2021

bobbypage commented Aug 25, 2021

bobbypage commented Aug 25, 2021 • edited Loading

Colstuwjx commented Aug 26, 2021

bobbypage commented Aug 26, 2021 • edited Loading

bobbypage commented Aug 26, 2021

bobbypage commented Aug 26, 2021

Colstuwjx commented Aug 27, 2021

Colstuwjx Aug 16, 2021 •

edited

Loading

bobbypage commented Aug 20, 2021 •

edited

Loading

bobbypage commented Aug 25, 2021 •

edited

Loading

bobbypage commented Aug 26, 2021 •

edited

Loading