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

update logcheck version and hack/verify-structured-logging.sh #103293

Merged
merged 2 commits into from
Oct 6, 2021
Merged

update logcheck version and hack/verify-structured-logging.sh #103293

merged 2 commits into from
Oct 6, 2021

Conversation

umangachapagain
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR updates the version of logcheck and enhances hack/verify-structured-logging.sh.
With this change we can now test quality of structured logging migration PRs that are not yet added in .structured_logging.

Which issue(s) this PR fixes:

Fixes #102439

Special notes for your reviewer:

Since checks needs to be run on entire codebase, test time is increased as compared to testing only the migrated packages listed in .structured_logging file.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 29, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @umangachapagain. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot requested review from fejta, juanvallejo and a team June 29, 2021 08:45
@k8s-ci-robot k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Jun 29, 2021
@umangachapagain
Copy link
Contributor Author

/cc @serathius

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2021
serathius
serathius reviewed Jun 30, 2021
View reviewed changes
hack/verify-structured-logging.sh Outdated
# Trim the trailing /... from given packages
ignore_packages=`cat ${KUBE_ROOT}/hack/.structured_logging | grep -oE '^[a-zA-Z0-9/]+[^/\.]'`
# Packages to test = all packages - ignored packages
packages=`go list ${KUBE_ROOT}/... | grep -v '$ignore_packages'`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this works, or at least when I tried it. Can you check this

Suggested change
packages=`go list ${KUBE_ROOT}/... | grep -v '$ignore_packages'`
packages=`grep -vf <(cat ./hack/.structured_logging | grep -oE '^[a-zA-Z0-9/]+[^/\.]') <(go list ./...) `

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it didn't work and I made the required changes.

@umangachapagain
Copy link
Contributor Author

umangachapagain commented Jun 30, 2021 •
edited

@serathius FYI

Running the script as it is generates following errors

Running structured logging static check on migrated packages
Structured logging static check passed on migrated packages :)
code in directory /home/uchapaga/go/src/k8s.io/kubernetes/staging/src/k8s.io/api/admission/v1 expects import "k8s.io/api/admission/v1"
code in directory /home/uchapaga/go/src/k8s.io/kubernetes/staging/src/k8s.io/api/admission/v1beta1 expects import "k8s.io/api/admission/v1beta1"
...

This is due to import comments in some packages. This can be easily fixed by enabling Go modules, which is turned off for some reason. Should I enable it?

When running with GO111MODULE=on

Running structured logging static check on migrated packages
Structured logging static check passed on migrated packages :)

Running structured logging static check on all other packages
pkg/generated/openapi/zz_generated.openapi.go:26:2: cannot find package "." in:
	/home/uchapaga/go/src/k8s.io/kubernetes/vendor/github.com/go-openapi/spec
/home/uchapaga/go/src/k8s.io/kubernetes/pkg/generated/openapi/zz_generated.openapi.go:26:7: could not import github.com/go-openapi/spec (invalid package name: "")
/home/uchapaga/go/src/k8s.io/kubernetes/pkg/generated/openapi/zz_generated.openapi.go:26:7: could not import github.com/go-openapi/spec (invalid package name: "")
logcheck: 3 errors during loading
pkg/generated/openapi/zz_generated.openapi.go:26:2: cannot find package "." in:
	/home/uchapaga/go/src/k8s.io/kubernetes/vendor/github.com/go-openapi/spec
/home/uchapaga/go/src/k8s.io/kubernetes/pkg/generated/openapi/zz_generated.openapi.go:26:7: could not import github.com/go-openapi/spec (invalid package name: "")
-: build constraints exclude all Go files in /home/uchapaga/go/src/k8s.io/kubernetes/pkg/volume/util/fsquota/common
/home/uchapaga/go/src/k8s.io/kubernetes/pkg/generated/openapi/zz_generated.openapi.go:26:7: could not import github.com/go-openapi/spec (invalid package name: "")
logcheck: 4 errors during loading
Please fix above failures. You can locally test via:
hack/verify-structured-logging.sh

These are loading errors on generated packages. Should we skip testing these?

@umangachapagain umangachapagain marked this pull request as ready for review June 30, 2021 16:03
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2021
@umangachapagain umangachapagain requested review from serathius and removed request for a team June 30, 2021 16:04
@BenTheElder
Copy link
Member

This is due to import comments in some packages. This can be easily fixed by enabling Go modules, which is turned off for some reason. Should I enable it?

Kubernetes builds from sources in vendor/, not in module mode. So the sources should be scanned in that mode most likely.

@umangachapagain umangachapagain mentioned this pull request Sep 20, 2021
Merged
@shivanshuraj1333
Copy link
Contributor

shivanshuraj1333 commented Sep 20, 2021 •
edited

@umangachapagain
When I try to run the script on my local I get the following import errors:
(the list is long so adding a small snippet to give you an idea)

➜  kubernetes git:(logcheck-update) ./hack/verify-structured-logging.sh --allow-unstructured
Running structured logging static check on migrated packages

-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/cmd/kubelet": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/cmd/kubelet/app": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/cmd/kubelet/app/options": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/fuzzer": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/scheme": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/v1alpha1": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/v1beta1": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/validation": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/podresources": cannot import absolute path

PS: I'm using macOS

@shivanshuraj1333 shivanshuraj1333 mentioned this pull request Sep 20, 2021
Closed
@umangachapagain
Copy link
Contributor Author

@umangachapagain
When I try to run the script on my local I get the following import errors:
(the list is long so adding a small snippet to give you an idea)

➜  kubernetes git:(logcheck-update) ./hack/verify-structured-logging.sh --allow-unstructured
Running structured logging static check on migrated packages

-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/cmd/kubelet": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/cmd/kubelet/app": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/cmd/kubelet/app/options": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/fuzzer": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/scheme": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/v1alpha1": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/v1beta1": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/config/validation": cannot import absolute path
-: import "/Users/shivashr/opensource/prow/go/src/k8s.io/kubernetes/pkg/kubelet/apis/podresources": cannot import absolute path

PS: I'm using macOS

Seems like a macOS only issue that was there long before this PR.
Since I'm not using mac I am unable to reproduce it.
I tried running logcheck with absolute path on my fedora machine and it worked fine.

I'll need help on this. Should we open a new issue for it and move ahead with this PR (since CI runs fine and will unblock migration tests)? @serathius @shivanshu1333

@shivanshuraj1333
Copy link
Contributor

/test pull-kubernetes-verify

@shivanshuraj1333 shivanshuraj1333 mentioned this pull request Sep 28, 2021
Closed
@shivanshuraj1333
Copy link
Contributor

Created #105316 to unblock the PR for import errors in macOS

@shivanshuraj1333
Copy link
Contributor

/retest

@shivanshuraj1333 shivanshuraj1333 mentioned this pull request Sep 28, 2021
Closed
@umangachapagain umangachapagain mentioned this pull request Sep 29, 2021
Closed
7 tasks
@umangachapagain
Copy link
Contributor Author

/test pull-kubernetes-verify

@umangachapagain
update hack/tools to pull latest logcheck
f4ff698 
Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>
@umangachapagain
update hack/verify-structured-logging.sh
1dd30f6 
hack/verify-structured-logging.sh now tests migrated packages
for use of unstructured logging functions and all other packages
for use of correct structured logging patterns.

Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>
@serathius
Copy link
Contributor

/assign @dims @BenTheElder

@dims
Copy link
Member

dims commented Oct 6, 2021

/approve

leaving /lgtm for @serathius

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, umangachapagain

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2021
@serathius
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2021
@k8s-ci-robot k8s-ci-robot merged commit 814b68f into kubernetes:master Oct 6, 2021
WG Structured Logging - Enhancement work automation moved this from In Review to Done Oct 6, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Oct 6, 2021
@umangachapagain umangachapagain deleted the logcheck-update branch October 7, 2021 08:50
@umangachapagain umangachapagain mentioned this pull request Feb 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanvallejo juanvallejo Awaiting requested review from juanvallejo

@serathius serathius Awaiting requested review from serathius

@BenTheElder BenTheElder Awaiting requested review from BenTheElder

Assignees

@dims dims

@BenTheElder BenTheElder

@serathius serathius

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging.
Projects
WG Structured Logging - Enhancement work
Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

Correctness of InfoS usage should be checked on whole codebase
9 participants
@umangachapagain @k8s-ci-robot @BenTheElder @serathius @pacoxu @fejta @dashpole @shivanshuraj1333 @dims