Skip to content

pkg/payload: Log manifest exclusion #712

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
Merged
openshift-merge-robot merged 1 commit into from
Feb 9, 2022
Merged

pkg/payload: Log manifest exclusion #712

openshift-merge-robot merged 1 commit into from
Feb 9, 2022

Conversation

@wking
Copy link
Member

@wking wking commented Dec 10, 2021

We have a number of different annotations all working together now, so logging exclusions with our reasoning will help folks who are debugging "why is my manifest not being reconciled?". And while this will create a number of logs for some profiles, we we already log each reconciled manifest with every sync cycle, so the volume increase isn't huge compared to our existing log rate. This is primarily a dev-time issue, which argues against log spew for production clusters, but since 72599d3 (#694), there's been the dynamic tech-preview business, so there is now more utility in production logging around this than there was before.

@wking
pkg/payload: Log manifest exclusion
5d343a5 
We have a number of different annotations all working together now, so
logging exclusions with our reasoning will help folks who are
debugging "why is my manifest not being reconciled?".  And while this
will create a number of logs for some profiles, we we already log each
reconciled manifest with every sync cycle, so the volume increase
isn't huge compared to our existing log rate.  This is primarily a
dev-time issue, which argues against log spew for production clusters,
but since 72599d3 (Exclude
featuregate.release.openshift/tech-preview=true manifests, 2021-04-13, openshift#694),
there's been the dynamic tech-preview business, so there is now more
utility in production logging around this than there was before.
@openshift-ci openshift-ci bot requested review from jottofar and vrutkovs December 10, 2021 17:34
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2021
vrutkovs
vrutkovs reviewed Dec 13, 2021
View reviewed changes
pkg/payload/payload.go
}

featureGateAnnotationValue, featureGateAnnotationExists := annotations["release.openshift.io/feature-gate"]
featureGateAnnotation := "release.openshift.io/feature-gate"
Copy link

@vrutkovs vrutkovs Dec 13, 2021

Choose a reason for hiding this comment

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

Move it to const?

Copy link
Member Author

@wking wking Dec 21, 2021

Choose a reason for hiding this comment

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

This is our one occurrence of this string outside of comments and tests. Did you want the tests to consume the constant, once we have it? They are hard-coding TechPreviewNoUpgrade today too:

$ git grep release.openshift.io/feature-gate | cat
...
pkg/payload/payload_test.go:                            "release.openshift.io/feature-gate":                           "TechPreviewNoUpgrade",
...

and there's already an openshift/api constant for that, so I expected the test-suite to prefer string literals to catch thing like "we typo'ed the constant value". And if we expect external consumers, I'd rather create the constant in openshift/api, or openshift/library-go instead of asking external consumers to import the CVO's pkg/payload. Thoughts?

vrutkovs
vrutkovs reviewed Dec 13, 2021
View reviewed changes
pkg/payload/payload.go
featureGateAnnotationValue, featureGateAnnotationExists := annotations["release.openshift.io/feature-gate"]
featureGateAnnotation := "release.openshift.io/feature-gate"
featureGateAnnotationValue, featureGateAnnotationExists := annotations[featureGateAnnotation]
if featureGateAnnotationValue == "TechPreviewNoUpgrade" && !includeTechPreview {
Copy link

@vrutkovs vrutkovs Dec 13, 2021

Choose a reason for hiding this comment

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

Same - lets define it as a const to simplify lookup / re-import if needed

vrutkovs
vrutkovs reviewed Dec 13, 2021
View reviewed changes
Copy link

@vrutkovs vrutkovs left a comment

Choose a reason for hiding this comment

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

Looks fine, few nits

@wking
Copy link
Member Author

wking commented Dec 21, 2021 •
edited
Loading

Checking one of the CI runs:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/712/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic/1469359661453938688/artifacts/e2e-agnostic/gather-extra/artifacts/pods/openshift-cluster-version_cluster-version-operator-86b869b467-t52jr_cluster-version-operator.log | grep payload.go
I1210 18:06:09.829117       1 payload.go:274] Loading updatepayload from "/"
I1210 18:06:09.980599       1 payload.go:191] excluding 0000_30_capi-operator_00_credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-cluster-api-aws: tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade
I1210 18:06:09.980635       1 payload.go:191] excluding 0000_30_capi-operator_00_credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-cluster-api-azure: tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade
I1210 18:06:09.980649       1 payload.go:191] excluding 0000_30_capi-operator_00_credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-cluster-api-gcp: tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade
I1210 18:06:10.039131       1 payload.go:191] excluding 0000_50_cluster-csi-snapshot-controller-operator_07_deployment-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-cluster-storage-operator name=csi-snapshot-controller-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.066572       1 payload.go:191] excluding 0000_50_cluster-image-registry-operator_07-operator-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-image-registry name=cluster-image-registry-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.085419       1 payload.go:191] excluding 0000_50_cluster-ingress-operator_00-ingress-credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-ingress-ibmcloud: no annotations
I1210 18:06:10.085456       1 payload.go:191] excluding 0000_50_cluster-ingress-operator_00-ingress-credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-ingress-powervs: no annotations
I1210 18:06:10.085466       1 payload.go:191] excluding 0000_50_cluster-ingress-operator_00-ingress-credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-ingress-operator name=openshift-ingress-alibabacloud: no annotations
I1210 18:06:10.088106       1 payload.go:191] excluding 0000_50_cluster-ingress-operator_02-deployment-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-ingress-operator name=ingress-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.092278       1 payload.go:191] excluding 0000_50_cluster-kube-storage-version-migrator-operator_07_deployment-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-kube-storage-version-migrator-operator name=kube-storage-version-migrator-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.348166       1 payload.go:191] excluding 0000_50_cluster-monitoring-operator_05-deployment-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-monitoring name=cluster-monitoring-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.360846       1 payload.go:191] excluding 0000_50_cluster-node-tuning-operator_50-operator-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-cluster-node-tuning-operator name=cluster-node-tuning-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.390359       1 payload.go:191] excluding 0000_50_cluster-samples-operator_06-operator-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-cluster-samples-operator name=cluster-samples-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.428949       1 payload.go:191] excluding 0000_50_cluster-storage-operator_10_deployment-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-cluster-storage-operator name=cluster-storage-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.441400       1 payload.go:191] excluding 0000_50_console-operator_07-operator-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-console-operator name=console-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.626198       1 payload.go:191] excluding 0000_50_olm_07-olm-operator.deployment.ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-operator-lifecycle-manager name=olm-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.627339       1 payload.go:191] excluding 0000_50_olm_08-catalog-operator.deployment.ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-operator-lifecycle-manager name=catalog-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.631574       1 payload.go:191] excluding 0000_50_operator-marketplace_09_operator-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-marketplace name=marketplace-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.638525       1 payload.go:191] excluding 0000_50_service-ca-operator_05_deploy-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-service-ca-operator name=service-ca-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.647749       1 payload.go:191] excluding 0000_70_cluster-network-operator_03_deployment-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-network-operator name=network-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.663588       1 payload.go:191] excluding 0000_70_dns-operator_02-deployment-ibm-cloud-managed.yaml group=apps kind=Deployment namespace=openshift-dns-operator name=dns-operator: include.release.openshift.io/self-managed-high-availability unset
I1210 18:06:10.681636       1 payload.go:191] excluding 0000_90_cluster-baremetal-operator_03_servicemonitor.yaml group=monitoring.coreos.com kind=ServiceMonitor namespace=openshift-machine-api name=cluster-baremetal-operator-servicemonitor: include.release.openshift.io/self-managed-high-availability unset
I1210 18:13:03.727643       1 payload.go:274] Loading updatepayload from "/"
I1210 18:13:04.015972       1 payload.go:191] excluding 0000_30_capi-operator_00_credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-cluster-api-aws: tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade
I1210 18:13:04.016004       1 payload.go:191] excluding 0000_30_capi-operator_00_credentials-request.yaml group=cloudcredential.openshift.io kind=CredentialsRequest namespace=openshift-cloud-credential-operator name=openshift-cluster-api-azure: tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade
...

I haven't dug into why it reloads the payload at 18:13, but the new log lines look good to me. Counting by type:

$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-version-operator/712/pull-ci-openshift-cluster-version-operator-master-e2e-agnostic/1469359661453938688/artifacts/e2e-agnostic/gather-extra/artifacts/pods/openshift-cluster-version_cluster-version-operator-86b869b467-t52jr_cluster-version-operator.log | sed -n 's/.*payload.go.*://p' | sort | uniq -c | sort -n
      2 274] Loading updatepayload from "/"
      6  no annotations
      6  tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade
     32  include.release.openshift.io/self-managed-high-availability unset

I'll go follow up on those annotation-less manifests in openshift/cluster-ingress-operator#692.

@vrutkovs
Copy link

vrutkovs commented Feb 8, 2022

/lgtm
/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 8, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrutkovs, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 8, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 8, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 8, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 8, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 9, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 9, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 9, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 9, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 9, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@wking
Copy link
Member Author

wking commented Feb 9, 2022

LB connectivity is unrelated.

/override ci/prow/e2e-agnostic-upgrade

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 9, 2022

@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-upgrade

Details

In response to this:

LB connectivity is unrelated.

/override ci/prow/e2e-agnostic-upgrade

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.

@wking
Copy link
Member Author

wking commented Feb 9, 2022

I am extremely confident that this logging is not going to have adverse interactions with any recently-landed, parallel pull requests.

/override ci/prow/e2e-agnostic
/override ci/prow/e2e-agnostic-operator

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 9, 2022

@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic, ci/prow/e2e-agnostic-operator

Details

In response to this:

I am extremely confident that this logging is not going to have adverse interactions with any recently-landed, parallel pull requests.

/override ci/prow/e2e-agnostic
/override ci/prow/e2e-agnostic-operator

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.

@openshift-merge-robot openshift-merge-robot merged commit 505f382 into openshift:master Feb 9, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 9, 2022

@wking: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@wking wking deleted the log-manifest-exclusion branch February 9, 2022 07:01
wking added a commit to wking/cluster-version-operator that referenced this pull request Mar 15, 2022
@wking
pkg/payload: Log load-time manifest exclusion at V(2)
c0fc6d8 
We moved from --v=5 to --v=2 with c4a1004 (*: Use --v=2 logging to
drop client-side throttling noise, 2022-01-04, openshift#721).  But this V(5)
logging slipped through via the parallel 5d343a5 (pkg/payload: Log
manifest exclusion, 2021-12-10, openshift#712).  Catch it up, to restore
exclusion logging.
@wking wking mentioned this pull request Mar 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jottofar jottofar Awaiting requested review from jottofar

1 more reviewer

@vrutkovs vrutkovs vrutkovs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vrutkovs vrutkovs

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wking @vrutkovs @openshift-bot @openshift-merge-robot